Repository navigation
loader: reject corrupt NSO modules that fail to decompress - #863
Fernandess07 wants to merge 1 commit into
Conversation
|
Retargeted to |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Previously, when an NSO module failed to decompress, the emulator got stuck in an endless loading loop, leaving the user unable to tell whether the problem was the emulator or the dump. The two ASSERTs were replaced by a check that returns a new error, ErrorCorruptNSO, which is shown to the user and written to the log. The NSO header stores the SHA-256 of each segment, but the emulator never compared it. It now does, which helps identify the problem. A mismatch only logs a warning, so games are not prevented from running when the hash does not match. Assisted-by: Claude Opus 5.5 (diagnosis of the corrupt dump, implementation and testing) Signed-off-by: Pedro Fernandes <pedrocufe05@gmail.com>
997d1c9 to
09e8ca1
Compare
| std::span<const u8>{compressed_data}.first(nso_header.segments_compressed_size[i]) | ||
| ); | ||
| ASSERT(r > 0); | ||
| if (r <= 0) { |
There was a problem hiding this comment.
This only catches outright zstd errors. ZSTD_decompress can succeed and write fewer bytes than the segment size, and the rest of decompressed_size (left over from the previous segment) then gets copied in. Could this match the LZ4 branch?
if (r != int(nso_header.segments[i].size)) {Also, DecompressDataZBIC returns -1 when dst is empty, so a compressed 0-byte segment (e.g. an empty .data) would now fail the load, where before the assert only logged. Probably worth skipping decompression when segments[i].size == 0.
| return std::nullopt; | ||
| }; | ||
| for (std::size_t i = 0; i < nso_header.segments.size(); ++i) { | ||
| nso_file.Read(compressed_data.data(), nso_header.segments_compressed_size[i], nso_header.segments[i].offset); |
There was a problem hiding this comment.
The return value of Read is ignored, so a truncated dump (probably the most common kind of corruption) shows up as a decompression failure or as stale bytes from the previous segment. Since this PR is about corrupt dumps, could this be checked as well?
if (nso_file.Read(compressed_data.data(), nso_header.segments_compressed_size[i],
nso_header.segments[i].offset) != nso_header.segments_compressed_size[i]) {
return corrupt(i, "is truncated");
}| if (nso_header.IsSegmentHashChecked(i)) { | ||
| NSOHeader::SHA256Hash hash{}; | ||
| unsigned int hash_size = 0; | ||
| EVP_Digest(segment_data, nso_header.segments[i].size, hash.data(), &hash_size, |
There was a problem hiding this comment.
For an uncompressed segment segment_data points into compressed_data, which is sized to the largest compressed segment, but this hashes segments[i].size bytes. A corrupt header with size > segments_compressed_size[i] reads past the heap buffer here. The uncompressed memcpy below already has this bug, but this adds a second over-read. Could uncompressed segments where segments_compressed_size[i] != segments[i].size be rejected via corrupt()?
Minor: EVP_Digest's return value is ignored. That's harmless, since a failure only gives a false warning, but it's easy to check.
| "Integrity verification failed.", | ||
| "The NUP container is malformed or its TOC hash does not match.", | ||
| "The NUP container does not contain a loadable base package.", | ||
| "An ExeFS module is corrupt and could not be decompressed. The game needs to be redumped.", |
There was a problem hiding this comment.
Nit: this is also shown when a standalone .nso is loaded through AppLoader_NSO::Load, which isn't ExeFS. Something like "An executable module (NSO) is corrupt…" would cover both.
| std::vector<Core::NCE::Patcher>* patches = nullptr, | ||
| s32 patch_index = -1); | ||
| s32 patch_index = -1, | ||
| ResultStatus* out_status = nullptr); |
There was a problem hiding this comment.
Nit, not a blocker: this is a tenth parameter after several defaulted ones. Returning the status (e.g. std::expected<VAddr, ResultStatus>) would avoid the out-pointer and the ResultStatus module_status = ErrorLoadingNSO; setup at each call site.
Summary
Previously, when an NSO module failed to decompress, the emulator got
stuck in an endless loading loop, leaving the user unable to tell
whether the problem was the emulator or the dump.
The two ASSERTs were replaced by a check that returns a new error,
ErrorCorruptNSO, which is shown to the user and written to the log.
The NSO header stores the SHA-256 of each segment, but the emulator
never compared it. It now does, which helps identify the problem. A
mismatch only logs a warning, so games are not prevented from running
when the hash does not match.
Assisted-by: Claude Opus 5.5 (diagnosis of the corrupt dump, implementation and testing)
Type of change
Related issues
Related to #70
Testing
Games tested:
Corrupt Pokopia dump (0400A3A01EE4C000): previously stuck in an endless loading loop, now fails with an error (checked in both the Qt frontend and eden-cli).
Checklist
Screenshots / logs
Summary by CodeRabbit