From 0b2773e2c6f663d6d71447d989f84cf7df762bb0 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:11:12 -0400 Subject: [PATCH 1/8] submission_package: Invert conditionals within NSP's constructor to reduce nesting We can use early continues here to reduce the amount of nesting. --- src/core/file_sys/submission_package.cpp | 94 ++++++++++++------------ 1 file changed, 49 insertions(+), 45 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 11264878d0..6ff43fa773 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -45,62 +45,66 @@ NSP::NSP(VirtualFile file_) Core::Crypto::KeyManager keys; for (const auto& ticket_file : files) { - if (ticket_file->GetExtension() == "tik") { - if (ticket_file == nullptr || - ticket_file->GetSize() < - Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { - continue; - } - - Core::Crypto::Key128 key{}; - ticket_file->Read(key.data(), key.size(), Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET); - std::string_view name_only(ticket_file->GetName()); - name_only.remove_suffix(4); - const auto rights_id_raw = Common::HexStringToArray<16>(name_only); - u128 rights_id; - std::memcpy(rights_id.data(), rights_id_raw.data(), sizeof(u128)); - keys.SetKey(Core::Crypto::S128KeyType::Titlekey, key, rights_id[1], rights_id[0]); + if (ticket_file->GetExtension() != "tik") { + continue; } + + if (ticket_file == nullptr || + ticket_file->GetSize() < + Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { + continue; + } + + Core::Crypto::Key128 key{}; + ticket_file->Read(key.data(), key.size(), Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET); + std::string_view name_only(ticket_file->GetName()); + name_only.remove_suffix(4); + const auto rights_id_raw = Common::HexStringToArray<16>(name_only); + u128 rights_id; + std::memcpy(rights_id.data(), rights_id_raw.data(), sizeof(u128)); + keys.SetKey(Core::Crypto::S128KeyType::Titlekey, key, rights_id[1], rights_id[0]); } for (const auto& outer_file : files) { - if (outer_file->GetName().substr(outer_file->GetName().size() - 9) == ".cnmt.nca") { - const auto nca = std::make_shared(outer_file); - if (nca->GetStatus() != Loader::ResultStatus::Success) { - program_status[nca->GetTitleId()] = nca->GetStatus(); + if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { + continue; + } + + const auto nca = std::make_shared(outer_file); + if (nca->GetStatus() != Loader::ResultStatus::Success) { + program_status[nca->GetTitleId()] = nca->GetStatus(); + continue; + } + + const auto section0 = nca->GetSubdirectories()[0]; + + for (const auto& inner_file : section0->GetFiles()) { + if (inner_file->GetExtension() != "cnmt") continue; - } - const auto section0 = nca->GetSubdirectories()[0]; + const CNMT cnmt(inner_file); + auto& ncas_title = ncas[cnmt.GetTitleID()]; - for (const auto& inner_file : section0->GetFiles()) { - if (inner_file->GetExtension() != "cnmt") + ncas_title[ContentRecordType::Meta] = nca; + for (const auto& rec : cnmt.GetContentRecords()) { + const auto id_string = Common::HexArrayToString(rec.nca_id, false); + const auto next_file = pfs->GetFile(fmt::format("{}.nca", id_string)); + if (next_file == nullptr) { + LOG_WARNING(Service_FS, + "NCA with ID {}.nca is listed in content metadata, but cannot " + "be found in PFS. NSP appears to be corrupted.", + id_string); continue; - - const CNMT cnmt(inner_file); - auto& ncas_title = ncas[cnmt.GetTitleID()]; - - ncas_title[ContentRecordType::Meta] = nca; - for (const auto& rec : cnmt.GetContentRecords()) { - const auto id_string = Common::HexArrayToString(rec.nca_id, false); - const auto next_file = pfs->GetFile(fmt::format("{}.nca", id_string)); - if (next_file == nullptr) { - LOG_WARNING(Service_FS, - "NCA with ID {}.nca is listed in content metadata, but cannot " - "be found in PFS. NSP appears to be corrupted.", - id_string); - continue; - } - - auto next_nca = std::make_shared(next_file); - if (next_nca->GetType() == NCAContentType::Program) - program_status[cnmt.GetTitleID()] = next_nca->GetStatus(); - if (next_nca->GetStatus() == Loader::ResultStatus::Success) - ncas_title[rec.type] = std::move(next_nca); } - break; + auto next_nca = std::make_shared(next_file); + if (next_nca->GetType() == NCAContentType::Program) + program_status[cnmt.GetTitleID()] = next_nca->GetStatus(); + if (next_nca->GetStatus() == Loader::ResultStatus::Success) + ncas_title[rec.type] = std::move(next_nca); } + + break; } } } From 17fb526fd9435ef0870374d67197fa8a46d46c28 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:20:54 -0400 Subject: [PATCH 2/8] submission_package: Move ticket key setting to its own function This behavior is entirely independent of the surrounding code, so it can be put in its own function to keep the behavior separate. --- src/core/file_sys/submission_package.cpp | 49 ++++++++++++++---------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 6ff43fa773..b1ebab17ff 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -18,6 +18,33 @@ #include "core/loader/loader.h" namespace FileSys { +namespace { +void SetTicketKeys(const std::vector& files) { + Core::Crypto::KeyManager keys; + + for (const auto& ticket_file : files) { + if (ticket_file->GetExtension() != "tik") { + continue; + } + + if (ticket_file == nullptr || + ticket_file->GetSize() < + Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { + continue; + } + + Core::Crypto::Key128 key{}; + ticket_file->Read(key.data(), key.size(), Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET); + std::string_view name_only(ticket_file->GetName()); + name_only.remove_suffix(4); + const auto rights_id_raw = Common::HexStringToArray<16>(name_only); + u128 rights_id; + std::memcpy(rights_id.data(), rights_id_raw.data(), sizeof(u128)); + keys.SetKey(Core::Crypto::S128KeyType::Titlekey, key, rights_id[1], rights_id[0]); + } +} +} // Anonymous namespace + NSP::NSP(VirtualFile file_) : file(std::move(file_)), status{Loader::ResultStatus::Success}, pfs(std::make_shared(file)) { @@ -43,27 +70,7 @@ NSP::NSP(VirtualFile file_) extracted = false; const auto files = pfs->GetFiles(); - Core::Crypto::KeyManager keys; - for (const auto& ticket_file : files) { - if (ticket_file->GetExtension() != "tik") { - continue; - } - - if (ticket_file == nullptr || - ticket_file->GetSize() < - Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { - continue; - } - - Core::Crypto::Key128 key{}; - ticket_file->Read(key.data(), key.size(), Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET); - std::string_view name_only(ticket_file->GetName()); - name_only.remove_suffix(4); - const auto rights_id_raw = Common::HexStringToArray<16>(name_only); - u128 rights_id; - std::memcpy(rights_id.data(), rights_id_raw.data(), sizeof(u128)); - keys.SetKey(Core::Crypto::S128KeyType::Titlekey, key, rights_id[1], rights_id[0]); - } + SetTicketKeys(files); for (const auto& outer_file : files) { if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { From eb438661e6cf86e516f56db390950d7767126880 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:35:38 -0400 Subject: [PATCH 3/8] submission_package: Move NCA reading code to its own function This too, is completely separate behavior from what is in the constructor, so we can move this to its own isolated function to keep everything self-contained. --- src/core/file_sys/submission_package.cpp | 89 ++++++++++++------------ src/core/file_sys/submission_package.h | 2 + 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index b1ebab17ff..b4d738d94e 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -71,49 +71,7 @@ NSP::NSP(VirtualFile file_) const auto files = pfs->GetFiles(); SetTicketKeys(files); - - for (const auto& outer_file : files) { - if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { - continue; - } - - const auto nca = std::make_shared(outer_file); - if (nca->GetStatus() != Loader::ResultStatus::Success) { - program_status[nca->GetTitleId()] = nca->GetStatus(); - continue; - } - - const auto section0 = nca->GetSubdirectories()[0]; - - for (const auto& inner_file : section0->GetFiles()) { - if (inner_file->GetExtension() != "cnmt") - continue; - - const CNMT cnmt(inner_file); - auto& ncas_title = ncas[cnmt.GetTitleID()]; - - ncas_title[ContentRecordType::Meta] = nca; - for (const auto& rec : cnmt.GetContentRecords()) { - const auto id_string = Common::HexArrayToString(rec.nca_id, false); - const auto next_file = pfs->GetFile(fmt::format("{}.nca", id_string)); - if (next_file == nullptr) { - LOG_WARNING(Service_FS, - "NCA with ID {}.nca is listed in content metadata, but cannot " - "be found in PFS. NSP appears to be corrupted.", - id_string); - continue; - } - - auto next_nca = std::make_shared(next_file); - if (next_nca->GetType() == NCAContentType::Program) - program_status[cnmt.GetTitleID()] = next_nca->GetStatus(); - if (next_nca->GetStatus() == Loader::ResultStatus::Success) - ncas_title[rec.type] = std::move(next_nca); - } - - break; - } - } + ReadNCAs(files); } NSP::~NSP() = default; @@ -253,4 +211,49 @@ VirtualDir NSP::GetParentDirectory() const { bool NSP::ReplaceFileWithSubdirectory(VirtualFile file, VirtualDir dir) { return false; } + +void NSP::ReadNCAs(const std::vector& files) { + for (const auto& outer_file : files) { + if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { + continue; + } + + const auto nca = std::make_shared(outer_file); + if (nca->GetStatus() != Loader::ResultStatus::Success) { + program_status[nca->GetTitleId()] = nca->GetStatus(); + continue; + } + + const auto section0 = nca->GetSubdirectories()[0]; + + for (const auto& inner_file : section0->GetFiles()) { + if (inner_file->GetExtension() != "cnmt") + continue; + + const CNMT cnmt(inner_file); + auto& ncas_title = ncas[cnmt.GetTitleID()]; + + ncas_title[ContentRecordType::Meta] = nca; + for (const auto& rec : cnmt.GetContentRecords()) { + const auto id_string = Common::HexArrayToString(rec.nca_id, false); + const auto next_file = pfs->GetFile(fmt::format("{}.nca", id_string)); + if (next_file == nullptr) { + LOG_WARNING(Service_FS, + "NCA with ID {}.nca is listed in content metadata, but cannot " + "be found in PFS. NSP appears to be corrupted.", + id_string); + continue; + } + + auto next_nca = std::make_shared(next_file); + if (next_nca->GetType() == NCAContentType::Program) + program_status[cnmt.GetTitleID()] = next_nca->GetStatus(); + if (next_nca->GetStatus() == Loader::ResultStatus::Success) + ncas_title[rec.type] = std::move(next_nca); + } + + break; + } + } +} } // namespace FileSys diff --git a/src/core/file_sys/submission_package.h b/src/core/file_sys/submission_package.h index e85a2b76eb..7c7cebf334 100644 --- a/src/core/file_sys/submission_package.h +++ b/src/core/file_sys/submission_package.h @@ -59,6 +59,8 @@ protected: bool ReplaceFileWithSubdirectory(VirtualFile file, VirtualDir dir) override; private: + void ReadNCAs(const std::vector& files); + VirtualFile file; bool extracted; From e4f994749bf6688806b2345b65ec70b26f8d027d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:43:34 -0400 Subject: [PATCH 4/8] submission_package: Move ExeFS and RomFS initialization to its own function Like the other two bits of factored out code, this can also be put within its own function. We can also modify the code so that it accepts a const reference to a std::vector of files, this way, we can deduplicate the file retrieval. Now the constructor for NSP isn't a combination of multiple behaviors in one spot. It's nice and separate. --- src/core/file_sys/submission_package.cpp | 27 +++++++++++++++--------- src/core/file_sys/submission_package.h | 1 + 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index b4d738d94e..5ebae15036 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -53,22 +53,15 @@ NSP::NSP(VirtualFile file_) return; } + const auto files = pfs->GetFiles(); + if (IsDirectoryExeFS(pfs)) { extracted = true; - exefs = pfs; - - const auto& files = pfs->GetFiles(); - const auto romfs_iter = - std::find_if(files.begin(), files.end(), [](const FileSys::VirtualFile& file) { - return file->GetName().find(".romfs") != std::string::npos; - }); - if (romfs_iter != files.end()) - romfs = *romfs_iter; + InitializeExeFSAndRomFS(files); return; } extracted = false; - const auto files = pfs->GetFiles(); SetTicketKeys(files); ReadNCAs(files); @@ -212,6 +205,20 @@ bool NSP::ReplaceFileWithSubdirectory(VirtualFile file, VirtualDir dir) { return false; } +void NSP::InitializeExeFSAndRomFS(const std::vector& files) { + exefs = pfs; + + const auto romfs_iter = std::find_if(files.begin(), files.end(), [](const VirtualFile& file) { + return file->GetName().find(".romfs") != std::string::npos; + }); + + if (romfs_iter == files.end()) { + return; + } + + romfs = *romfs_iter; +} + void NSP::ReadNCAs(const std::vector& files) { for (const auto& outer_file : files) { if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { diff --git a/src/core/file_sys/submission_package.h b/src/core/file_sys/submission_package.h index 7c7cebf334..2978215221 100644 --- a/src/core/file_sys/submission_package.h +++ b/src/core/file_sys/submission_package.h @@ -59,6 +59,7 @@ protected: bool ReplaceFileWithSubdirectory(VirtualFile file, VirtualDir dir) override; private: + void InitializeExeFSAndRomFS(const std::vector& files); void ReadNCAs(const std::vector& files); VirtualFile file; From 8f51371d7b9d29bb5599016b5a7ee9fb43756e78 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:47:32 -0400 Subject: [PATCH 5/8] submission_package: Ensure the 'extracted' member variable is always initialized If an error occurs when constructing the PartitionFilesystem instance, the constructor would be exited early, which wouldn't initialize the extracted data member, making it possible for other code to perform an uninitialized read by calling the public IsExtractedType() member function. This prevents that. --- src/core/file_sys/submission_package.cpp | 2 -- src/core/file_sys/submission_package.h | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 5ebae15036..d39b79eddf 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -61,8 +61,6 @@ NSP::NSP(VirtualFile file_) return; } - extracted = false; - SetTicketKeys(files); ReadNCAs(files); } diff --git a/src/core/file_sys/submission_package.h b/src/core/file_sys/submission_package.h index 2978215221..da3dc5e9f3 100644 --- a/src/core/file_sys/submission_package.h +++ b/src/core/file_sys/submission_package.h @@ -64,7 +64,7 @@ private: VirtualFile file; - bool extracted; + bool extracted = false; Loader::ResultStatus status; std::map program_status; From b77f6a32ff1957f88bcb521323aef62efb13d87a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:50:59 -0400 Subject: [PATCH 6/8] submission_package: Use std::string's rfind() when looking for the extension in InitializeExeFSAndRomFS() When searching for a file extension, it's generally preferable to begin the search at the end of the string rather than the beginning, as the whole string isn't going to be walked just to check for something at the end of it. --- src/core/file_sys/submission_package.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index d39b79eddf..c3848e826f 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -207,7 +207,7 @@ void NSP::InitializeExeFSAndRomFS(const std::vector& files) { exefs = pfs; const auto romfs_iter = std::find_if(files.begin(), files.end(), [](const VirtualFile& file) { - return file->GetName().find(".romfs") != std::string::npos; + return file->GetName().rfind(".romfs") != std::string::npos; }); if (romfs_iter == files.end()) { From 689010c2fa94e856f989b08e082713e0ce50ec86 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 02:06:26 -0400 Subject: [PATCH 7/8] submission_package: Correct location of null check within SetTicketKeys() If a ticket file was ever a null pointer, we'd cause a null pointer dereference, as we were calling GetExtension() on the pointer instance. --- src/core/file_sys/submission_package.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index c3848e826f..829aca06f8 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -23,13 +23,16 @@ void SetTicketKeys(const std::vector& files) { Core::Crypto::KeyManager keys; for (const auto& ticket_file : files) { + if (ticket_file == nullptr) { + continue; + } + if (ticket_file->GetExtension() != "tik") { continue; } - if (ticket_file == nullptr || - ticket_file->GetSize() < - Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { + if (ticket_file->GetSize() < + Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET + sizeof(Core::Crypto::Key128)) { continue; } From b9b8610d6dde9f8d30c77d1f002e3142eccf08ad Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 02:13:49 -0400 Subject: [PATCH 8/8] submission_package: Avoid dangling std::string_view within SetTicketKeys() GetName() returns a std::string by value, not by reference, so after the std::string_view is constructed, it's not well defined to actually execute any member functions of std::string_view that attempt to access the data, as the std::string has already been destroyed. Instead, we can just use a std::string and erase the last four characters. --- src/core/file_sys/submission_package.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 829aca06f8..09bf077cda 100644 --- a/src/core/file_sys/submission_package.cpp +++ b/src/core/file_sys/submission_package.cpp @@ -38,8 +38,11 @@ void SetTicketKeys(const std::vector& files) { Core::Crypto::Key128 key{}; ticket_file->Read(key.data(), key.size(), Core::Crypto::TICKET_FILE_TITLEKEY_OFFSET); - std::string_view name_only(ticket_file->GetName()); - name_only.remove_suffix(4); + + // We get the name without the extension in order to create the rights ID. + std::string name_only(ticket_file->GetName()); + name_only.erase(name_only.size() - 4); + const auto rights_id_raw = Common::HexStringToArray<16>(name_only); u128 rights_id; std::memcpy(rights_id.data(), rights_id_raw.data(), sizeof(u128));