New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file_sys/archive_ncch: use NCCHs/.apps instead of .romfs files, NCCH section override #2975

Merged
merged 4 commits into from Oct 6, 2017

Conversation

Projects
None yet
4 participants
@shinyquagsire23
Copy link
Contributor

shinyquagsire23 commented Sep 30, 2017

  • archive_ncch now uses NCCHContainer instead of loading a .romfs
  • Backward compatibility with existing is maintained through RomFS override, ExeFS override has also been added which fixes #2735
  • A class for reading from and writing to TMDs has been added, which paves the way for title install via CIAs
  • archive_ncch will use title metadata to determine the content ID for the main executable. This allows for decrypted NAND contents to be used without altering the file structure.
  • The NCCH loader will also use title metadata to determine the content ID for update NCCHs, given

RomFS and ExeFS override is per-NCCH. For example, if I have a update of Smash installed as sdmc:/Nintendo 3DS/00000000000000000000000000000000/00000000000000000000000000000000/title/0004000e/000ee000/content/00000017.app, the RomFS override would be sdmc:/Nintendo 3DS/00000000000000000000000000000000/00000000000000000000000000000000/title/0004000e/000ee000/content/00000017.app.romfs. If I wanted to override an exeFS file (ie .code), I can place the file at sdmc:/Nintendo 3DS/00000000000000000000000000000000/00000000000000000000000000000000/title/0004000e/000ee000/content/00000017.app.exefsdir/.code. (For Windows-based systems, .code can be overridden as code.bin instead).

If a TMD is not found for an application, the path will default to .../00000000.app, and if .../00000000.app is not found but .../00000000.app.romfs is, the RomFS will stand-in as an NCCH even though the full contents of the NCCH are not available. For system archives, this generally works fine and archive_ncch will still work with older .romfs-based dumps.


This change is Reviewable

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:archive-ncch-container-and-override branch from dfec74c to 6464a3e Sep 30, 2017

if (ncch_header.romfs_offset != 0 && ncch_header.romfs_size != 0)
has_romfs = true;
// Check for split-off files, mark the archive as tainted if we will use them
bool has_split_romfs = false;

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

IMO this entire section could be factored into a function, something like LoadOverrides but with a better name.

Loader::ResultStatus result = Load();
if (result != Loader::ResultStatus::Success)
return result;

if (!has_exefs)
// Check if we have files that can drop-in and replace

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

This too could be made into its own function, LoadOverrideExeFSSection

Loader::ResultStatus result = Load();
if (result != Loader::ResultStatus::Success)
return result;

// Check for RomFS overrides

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

Same

static const int TMD_INDEX_MANUAL = 1;
static const int TMD_INDEX_DLP = 2;

static u32 GetSignatureSize(u32_be signature_type) {

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

The parameter can be a normal u32

return Loader::ResultStatus::Error;

// The TMD body start position is rounded to the nearest 0x40 after the signature
u32 body_start = (signature_size + sizeof(u32_be) + 0x40) & ~0x3F;

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

We have AlignUp/AlignDown functions in the Common namespace

TMD_ContentInfo contentinfo[64];
};

static_assert(sizeof(TMD_Body) == 0x9C4, "TMD body structure size is wrong");

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

I'm not a fan of the naming convention in these structures, can't you make them members of the TitleMetadata class and get rid of the TMD_ prefix?

////////////////////////////////////////////////////////////////////////////////////////////////////
/// TMD structures

#pragma pack(1)

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

#pragma pack(push, 1)
and #pragma pack(pop)

void SetSystemVersion(u64 version);
void AddContentChunk(TMD_ContentChunk& chunk);

void Print();

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

this can be const

Loader::ResultStatus Load();
Loader::ResultStatus Save();

u64 GetTitleID();

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

These Get functions can be const

@@ -247,6 +247,12 @@ Loader::ResultStatus NCCHContainer::LoadSectionExeFS(const char* name, std::vect
std::string section_override = filepath + ".exefsdir/" + name;
FileUtil::IOFile section_file = FileUtil::IOFile(section_override, "rb");

// Special override case for .code -> code.bin
if (!section_file.IsOpen() && !strcmp(name, ".code")) {

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

Should we really support both naming conventions?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Sep 30, 2017

Contributor

Hmm, probably not I suppose. Would it just be better to map all files to their 3dstool counterpart, ie .code -> code.bin, icon -> icon.icn, banner -> banner.bnr, logo -> logo.bcma.lz?

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

That sounds like a good idea

@lioncash
Copy link
Member

lioncash left a comment

Just did a quick pass over everything. Feel free to tell me if you disagree with any of the suggestions.


CryptoPP::SHA256 chunk_hash;
for (int i = 0; i < tmd_body.content_count; i++) {
u8* chunk_data = static_cast<u8*>(static_cast<void*>(&tmd_chunks[i]));

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

can't this just be a single reinterpret_cast<u8*>(&tmd_chunks[i]);?


CryptoPP::SHA256 contentinfo_hash;
for (int i = 0; i < 64; i++) {
u8* contentinfo_data = static_cast<u8*>(static_cast<void*>(&tmd_body.contentinfo[i]));

This comment has been minimized.

@lioncash
u16_be boot_content;
u8 reserved_4[2];
u8 contentinfo_hash[0x20];
TMD_ContentInfo contentinfo[64];

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

You can make this a std::array so you can just call contentinfo.size() instead of hardcoding 64 as a loop boundary in other source files.

*/
class TitleMetadata {
public:
TitleMetadata(std::string& path) : filepath(path) {}

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member
explicit TitleMetadata(std::string path) : filepath(std::move(path)) {}
void SetTitleType(u32 type);
void SetTitleVersion(u16 version);
void SetSystemVersion(u64 version);
void AddContentChunk(TMD_ContentChunk& chunk);

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member
void AddContentChunk(const TMD_ContentChunk& chunk);
return Loader::ResultStatus::Error;

// The TMD body start position is rounded to the nearest 0x40 after the signature
u32 body_start = (signature_size + sizeof(u32_be) + 0x40) & ~0x3F;

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

This should likely be a size_t, otherwise this truncates to a u32 implicitly (sizeof returns a size_t)

return Loader::ResultStatus::Error;

// The TMD body start position is rounded to the nearest 0x40 after the signature
u32 body_start = (signature_size + sizeof(u32_be) + 0x40) & ~0x3F;

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

This should likely be a size_t, otherwise this truncates to a u32 implicitly (sizeof returns a size_t). The below log formatting specifier would need to be changed to %zu as well.

if (!has_exefs)
// Check if we have files that can drop-in and replace
std::string section_override = filepath + ".exefsdir/" + name;
FileUtil::IOFile section_file = FileUtil::IOFile(section_override, "rb");

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member
auto section_file = FileUtil::IOFile(section_override, "rb");

or

FileUtil::IOFile section_file(section_override, "rb");
}
}

file.Close();

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

file will automatically close when it goes out of scope, so this isn't necessary.

return Loader::ResultStatus::Error;
}

file.Close();

This comment has been minimized.

@lioncash

lioncash Sep 30, 2017

Member

file will automatically close when it goes out of scope

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Sep 30, 2017

Review status: 0 of 7 files reviewed at latest revision, 47 unresolved discussions.


src/core/file_sys/title_metadata.cpp, line 51 at r3 (raw file):

Previously, Subv (Sebastian Valle) wrote…

Why is this a warning?

Yeah that was just a test print for myself, removed now.


src/core/file_sys/title_metadata.h, line 36 at r3 (raw file):

Previously, Subv (Sebastian Valle) wrote…

can this be an u8 array instead of a char array? also, use std::array, i'll stop pointing this out for the rest of the arrays

Had it as a char array because it's actually a readable/possibly printable string, but I doubt we'll ever really be checking certs


Comments from Reviewable

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Sep 30, 2017

Fixed nits, there was some conflicting opinions on the for loops but I ended up using a u16 since the struct field was u16, though for the Print() for I used size_t since I checked against .size().

for (int i = 0; i < 64; i++) {
u8* contentinfo_data = static_cast<u8*>(static_cast<void*>(&tmd_body.contentinfo[i]));
chunk_hash.Update(contentinfo_data, sizeof(TMD_ContentInfo));
for (int i = 0; i < tmd_body.contentinfo.size(); i++) {

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

size_t

static_cast<u16>(tmd_body.contentinfo[i].index),
static_cast<u16>(tmd_body.contentinfo[i].command_count));
LOG_DEBUG(Service_FS, " Index %04X, Command Count %04X",
static_cast<u16>(tmd_body.contentinfo[i].index),

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

These will all be promoted to int automatically, but i prefer it be explicit, static_cast<u32>

u16 index = static_cast<u16>(tmd_body.contentinfo[i].index);
u16 count = static_cast<u16>(tmd_body.contentinfo[i].command_count);

if (count == 0)
continue;

LOG_WARNING(Service_FS, "Content chunks for content info index %u:", i);
for (int j = index; j < index + count; j++) {
LOG_DEBUG(Service_FS, "Content chunks for content info index %u:", i);

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

%zu

ContentChunk chunk = tmd_chunks[j];
LOG_DEBUG(Service_FS, " ID %08X, Index %04X, Type %04x, Size %016llx",
static_cast<u32>(chunk.id), static_cast<u16>(chunk.index),
static_cast<u16>(chunk.type), static_cast<u64>(chunk.size));

This comment has been minimized.

@Subv

Subv Sep 30, 2017

Member

Cast to u32.

As for the u64 cast, it won't work as you expect, you'll have to use an 64 bit format specifier like PRIu64

// TODO(shinyquagsire23): Do TMDs with more than one contentinfo exist?
// For now we'll just adjust the first index to hold all content chunks
// and ensure that no further content info data exists.
memset(tmd_body.contentinfo.data(), 0, sizeof(tmd_body.contentinfo));

This comment has been minimized.

@lioncash

lioncash Oct 1, 2017

Member

Since you're using a std::array, you can do:

tmd_body.contentinfo = {};

to zero it out.

LOG_DEBUG(Service_FS, "ExeFS offset: 0x%08X", exefs_offset);
LOG_DEBUG(Service_FS, "ExeFS size: 0x%08X", exefs_size);
// We need at least one of these or overrides, practically
if (!(has_exefs | has_romfs | is_tainted))

This comment has been minimized.

@lioncash

lioncash Oct 1, 2017

Member

Shouldn't these be ||, so you get short-circuit evaluation, or is this intentional?

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 1, 2017

Contributor

It was kinda intentional, but in retrospect || probably compiles nicer.

if (j > tmd_body.content_count)
break;

ContentChunk chunk = tmd_chunks[j];

This comment has been minimized.

@lioncash

lioncash Oct 1, 2017

Member

Should this be a const ContentChunk& so you aren't making copies of the structs instances in a loop?

Shared = 1 << 15
};

enum TMDContentIndex { Main = 0, Manual = 1, DLP = 2 };

This comment has been minimized.

@lioncash

lioncash Oct 1, 2017

Member

This can be private within the TitleMetadata class since it's only ever used there.

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Oct 1, 2017

u16 stuff

FWIW, my reasoning for size_t is it results in slightly better codegen for no work, since size_t generally scales with the memory architecture on a platform (as it's required to be able to hold the maximum size of any theoretical possible object on a platform) so it removes the need for an extra zero or sign extension. Reasoning behind that being since it generally scales with memory arch, the compiler assumption can be made that the extra upper bits of the register won't contain junk data. Example.

That said, it doesn't really matter much here.

@shinyquagsire23

This comment has been minimized.

Copy link
Contributor

shinyquagsire23 commented Oct 1, 2017

Got those changes sorted out, and yeah I don't think the slight differences in compiler output will matter too much in this case at least. Though, there will be cases later where title scanning will have to load several TMDs but even then I don't think this code will end up being in any hot code, mostly just loading stuff.


Review status: 0 of 7 files reviewed at latest revision, 55 unresolved discussions.


Comments from Reviewable

@Subv

Subv approved these changes Oct 1, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:archive-ncch-container-and-override branch 4 times, most recently from 2dd8f3a to 6c2fecc Oct 1, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:archive-ncch-container-and-override branch from 6c2fecc to 4887d18 Oct 1, 2017

@Subv Subv merged commit 74d4050 into citra-emu:master Oct 6, 2017

2 of 3 checks passed

code-review/reviewable 7 files, 55 discussions left (lioncash, shinyquagsire23, Subv)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Oct 22, 2017

Your PR introduce 1 new warning. If you can fix it in upcoming PR, that will be great.

'Loader::AppLoader_NCCH::ReadUpdateRomFS': not all control paths return a value
src\core\loader\ncch.cpp -------------------------------------------------------------- Line 242 [core]

@Subv Subv referenced this pull request Dec 3, 2017

Closed

Progress Report 2017 October #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment