From 0d83f8f2558b45923b072cf535bf7da9e5c24aea Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:11:12 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 11264878d..6ff43fa77 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; - } + if (ticket_file->GetExtension() != "tik") { + 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 == 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(); - continue; - } + if (outer_file->GetName().substr(outer_file->GetName().size() - 9) != ".cnmt.nca") { + continue; + } - const auto section0 = nca->GetSubdirectories()[0]; + const auto nca = std::make_shared(outer_file); + if (nca->GetStatus() != Loader::ResultStatus::Success) { + program_status[nca->GetTitleId()] = nca->GetStatus(); + continue; + } - 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()]; - - 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); + 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; } - 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; } } } -- cgit v1.2.3 From 4f18d3588879fcfdbab7dbc1f0a1ecda61ea4b0f Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:20:54 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 6ff43fa77..b1ebab17f 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") { -- cgit v1.2.3 From fd312abedd437bc22b34da65eb1ff42dadce7388 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:35:38 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index b1ebab17f..b4d738d94 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 e85a2b76e..7c7cebf33 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; -- cgit v1.2.3 From ccf0a9cb381cad75878c79b962ac61ded0466d08 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:43:34 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index b4d738d94..5ebae1503 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 7c7cebf33..297821522 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; -- cgit v1.2.3 From 37ee05f7c0f1fa8d94317e92795016e91f1bdfa6 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:47:32 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 5ebae1503..d39b79edd 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 297821522..da3dc5e9f 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; -- cgit v1.2.3 From 02841052aa5e2126089268f465858fff379577d8 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 01:50:59 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index d39b79edd..c3848e826 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()) { -- cgit v1.2.3 From dade709f6347ca3b27a0577efc25adae8e89bcf8 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 02:06:26 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index c3848e826..829aca06f 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; } -- cgit v1.2.3 From 024eec02a59d5902e3731a7120ebc97846b34991 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 3 Oct 2018 02:13:49 -0400 Subject: 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(-) (limited to 'src/core/file_sys') diff --git a/src/core/file_sys/submission_package.cpp b/src/core/file_sys/submission_package.cpp index 829aca06f..09bf077cd 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)); -- cgit v1.2.3