From 0d83f8f2558b45923b072cf535bf7da9e5c24aea 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 4f18d3588879fcfdbab7dbc1f0a1ecda61ea4b0f 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 fd312abedd437bc22b34da65eb1ff42dadce7388 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 ccf0a9cb381cad75878c79b962ac61ded0466d08 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 37ee05f7c0f1fa8d94317e92795016e91f1bdfa6 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 02841052aa5e2126089268f465858fff379577d8 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 dade709f6347ca3b27a0577efc25adae8e89bcf8 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 024eec02a59d5902e3731a7120ebc97846b34991 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));