Skip to content
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

Show an error if CIA contend is encrypted #5130

Merged
merged 6 commits into from Apr 16, 2020

Conversation

B3n30
Copy link
Contributor

@B3n30 B3n30 commented Mar 15, 2020

We already show an error if the cia is encrypted but not if the cia itself isn't encrypted but it's content is.
This lead to a lot of confusion where users thought they successfully installed the cia, but then nothing appeared in the game list. Now the encryption error is also shown if the cia contained any executable that Citra isn't able to decrypt (e.g. due to missing aes keys).

In addition any executable that is encrypted and Citra can't decrypt is still shown in the game list (if executables without icons are visible). Such executables will still be shown in an ugly way (no proper title, icon, region etc), since we can't access the SMDH.

As a last change in that PR I removed a debugging line we added that showed the path of each NCCH file, since it was only added to debug an issue where loading encrypted NCCHs caused a crash.


This change is Reviewable

@B3n30
Copy link
Contributor Author

B3n30 commented Mar 15, 2020

This should fix #5065 and #4117

@FearlessTobi
Copy link
Contributor

FearlessTobi commented Mar 15, 2020

The braces should be placed on the same line as the if statements.

@B3n30 B3n30 force-pushed the warn_if_cia_contend_is_encrypted branch from 2b70160 to 706a329 Compare Mar 15, 2020
@B3n30 B3n30 force-pushed the warn_if_cia_contend_is_encrypted branch from 706a329 to 391580c Compare Mar 15, 2020
src/core/hle/service/am/am.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@FearlessTobi FearlessTobi left a comment

Some minor stuff I noticed

src/core/file_sys/ncch_container.cpp Outdated Show resolved Hide resolved
@@ -133,8 +133,37 @@ Loader::ResultStatus NCCHContainer::OpenFile(const std::string& filepath, u32 nc
return Loader::ResultStatus::Success;
}

Loader::ResultStatus NCCHContainer::LoadHeader() {
if (has_header)
Copy link
Contributor

@FearlessTobi FearlessTobi Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are following the convention that all if statements should be braced. Dito below.

src/citra_qt/game_list_worker.cpp Outdated Show resolved Hide resolved
src/core/hle/service/am/am.cpp Outdated Show resolved Hide resolved
@B3n30 B3n30 force-pushed the warn_if_cia_contend_is_encrypted branch from 1a3cbf7 to a4457d8 Compare Mar 15, 2020
@B3n30
Copy link
Contributor Author

B3n30 commented Mar 15, 2020

Since some users requested it, I added a log that shows which key is missing when a Ticket gets decrypted during cia installation

const auto file_ext = FileUtil::GetExtensionFromFilename(path);
return std::find(extensions.begin(), extensions.end(), file_ext) != extensions.end();
}
} // namespace
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't see a reason for this hardcoded list of extensions, why can't we just try to load everything in the folder?

Copy link
Contributor Author

@B3n30 B3n30 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just to don't do any unnecessary stuff (on big game lists this could really be costly)

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only executed once when a CIA is installed so I don't think it's going to influence the game list performance

@@ -697,7 +728,7 @@ Loader::ResultStatus NCCHContainer::ReadOverrideRomFS(std::shared_ptr<RomFSReade
}

Loader::ResultStatus NCCHContainer::ReadProgramId(u64_le& program_id) {
Loader::ResultStatus result = Load();
Loader::ResultStatus result = LoadHeader();
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary?

Copy link
Contributor Author

@B3n30 B3n30 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensure that we get a ProgramID even if the NCCH is encrypted and the keys aren't available. It probably still requires to change the ui to show the programId instead of the filename in the game list though


has_header = true;
return Loader::ResultStatus::Success;
}
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably convert the first part of the Load() to LoadHeader() then

Copy link
Contributor Author

@B3n30 B3n30 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats not that easy because it seems the remaining part of Load() needs the file to be at the position after the header

return FileUtil::ForeachDirectoryEntry(nullptr, physical_name, callback);
}
};
if (!FileUtil::ForeachDirectoryEntry(nullptr, path, callback)) {
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? won't path point to the CIA file (instead of the installed directory)?

Copy link
Contributor Author

@B3n30 B3n30 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, thx for spotting this

const auto file_ext = FileUtil::GetExtensionFromFilename(path);
return std::find(extensions.begin(), extensions.end(), file_ext) != extensions.end();
}
} // namespace
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only executed once when a CIA is installed so I don't think it's going to influence the game list performance

@hamish-milne hamish-milne merged commit 026a63b into citra-emu:master Apr 16, 2020
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants