fix(nup): exclude digest-mismatched stand-in sources #2019

github-sync · 2026-11-10T23:50:07Z

Arkex-ai commented

2026-11-10 23:50:07 +00:00

(Migrated from github.com)

Summary

The Builder could copy a corrupted ROMFS file into another path during repair if the donor had a valid Zstd prefix. It now excludes donors that fail their SHA-256 check.

Addresses the stand-in source part of #2005.

Technical changes

Hash .zs bytes while writing fresh files. Verify retained files on resume with bounded reads. Preserve prefix checks when a digest is absent.
Carry mismatches through the existing damage list. Build the exclusion set once and apply it throughout replacement.

Validation

At 2f95040, all 24 Linux NUP path tests passed. The hosted source suite finished at 324 passed / 0 failed / 24 skipped on Windows and 338 / 0 / 10 on Linux. The integrity regression ran on both platforms.
The focused regression fails on main and passes on the candidate. It selects the intact donor during fresh and resumed extraction and preserves the original NUP. Windows also passed the stand-in, extraction, codec and disk-full checks.
In four paired cached Windows runs with a synthetic 256 MiB NUP at feb38bf, median fresh extraction went from 0.143 s to 0.254 s. Median resume went from 0.102 s to 0.303 s as retained files were read for verification. Full game dumps and gameplay remain untested.

Current-head check	Result
Linux AppImage	Passed
Windows build	Passed
Source suite	Passed

## Summary The Builder could copy a corrupted ROMFS file into another path during repair if the donor had a valid Zstd prefix. It now excludes donors that fail their SHA-256 check. Addresses the stand-in source part of #2005. ## Technical changes - Hash `.zs` bytes while writing fresh files. Verify retained files on resume with bounded reads. Preserve prefix checks when a digest is absent. - Carry mismatches through the existing damage list. Build the exclusion set once and apply it throughout replacement. ## Validation - At `2f95040`, all **24 Linux NUP path tests** passed. The hosted source suite finished at **324 passed / 0 failed / 24 skipped on Windows** and **338 / 0 / 10 on Linux**. The integrity regression ran on both platforms. - The focused regression fails on main and passes on the candidate. It selects the intact donor during fresh and resumed extraction and preserves the original NUP. Windows also passed the stand-in, extraction, codec and disk-full checks. - In four paired cached Windows runs with a synthetic 256 MiB NUP at `feb38bf`, median fresh extraction went from **0.143 s to 0.254 s**. Median resume went from **0.102 s to 0.303 s** as retained files were read for verification. Full game dumps and gameplay remain untested.  | Current-head check | Result | |---|---| | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/42931635552) | Passed | | [Windows build](https://github.com/NotRexed/Hookshot/actions/runs/42931635399) | Passed | | [Source suite](https://github.com/NotRexed/Hookshot/actions/runs/42931635529) | Passed |

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 02:20:56 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks right to me. The TOC digest is over the decoded member bytes (same as verify() checks), and HeadSink hashes what copy_to writes, so a fresh extraction compares like with like. The resume branch hashes the file on disk, which is also decoded content. Because a mismatched file now goes on the damaged list, it is excluded as a donor by the existing stand_in_for(path, excluded) check. That closes the #2005 case, where a corrupt file with a valid prefix got copied over other paths. The regression test covers it well: the near-match donor's bytes are flipped, it runs both the fresh and the resumed pass, and it checks the .nup stays untouched.

On security: the digests come from the same untrusted TOC as everything else, so this is an integrity check, not an authenticity one. That's fine for the goal here. No new parsing is added, and the reads on resume are bounded at 1 MiB blocks. CI is green, and main hasn't changed nx2/nup.py since the base.

Two things to keep in mind, neither blocking:

The cost of hashing everything on resume doesn't reach Builder users. make_game.py wipes game/ before it runs nup.py --extract and again on failure, so only a manual CLI re-run takes the resume path. There it now reads the whole retained romfs (~23 GB on a full dump), not 4 bytes a file. That's worth it for correctness, but it's minutes on an HDD rather than seconds.
The fix only covers the path where an extraction list exists. When stand_in_damaged_files falls back to damaged_game_files (a folder from an earlier builder, or a stale heads file), donors are still chosen on prefix alone, since there's no .nup to check against there. That's expected; I'm just noting the gap.

Nits:

The message in make_game.py (Your game dump is incomplete ... 23.3 GB) now also covers digest mismatches. Those more likely mean a damaged copy of the .nup than a short dump, so a line like "or copy the .nup again" would point people the right way. keep_heads' docstring ("files that do not start as zstd") is out of date for the same reason.
HeadSink.damaged() is followed by a single blank line before the module-level comment.

Looks right to me. The TOC digest is over the decoded member bytes (same as `verify()` checks), and `HeadSink` hashes what `copy_to` writes, so a fresh extraction compares like with like. The resume branch hashes the file on disk, which is also decoded content. Because a mismatched file now goes on the damaged list, it is excluded as a donor by the existing `stand_in_for(path, excluded)` check. That closes the #2005 case, where a corrupt file with a valid prefix got copied over other paths. The regression test covers it well: the near-match donor's bytes are flipped, it runs both the fresh and the resumed pass, and it checks the .nup stays untouched. On security: the digests come from the same untrusted TOC as everything else, so this is an integrity check, not an authenticity one. That's fine for the goal here. No new parsing is added, and the reads on resume are bounded at 1 MiB blocks. CI is green, and main hasn't changed `nx2/nup.py` since the base. Two things to keep in mind, neither blocking: - The cost of hashing everything on resume doesn't reach Builder users. `make_game.py` wipes `game/` before it runs `nup.py --extract` and again on failure, so only a manual CLI re-run takes the resume path. There it now reads the whole retained romfs (~23 GB on a full dump), not 4 bytes a file. That's worth it for correctness, but it's minutes on an HDD rather than seconds. - The fix only covers the path where an extraction list exists. When `stand_in_damaged_files` falls back to `damaged_game_files` (a folder from an earlier builder, or a stale heads file), donors are still chosen on prefix alone, since there's no .nup to check against there. That's expected; I'm just noting the gap. Nits: - The message in `make_game.py` (`Your game dump is incomplete ... 23.3 GB`) now also covers digest mismatches. Those more likely mean a damaged copy of the .nup than a short dump, so a line like "or copy the .nup again" would point people the right way. `keep_heads`' docstring ("files that do not start as zstd") is out of date for the same reason. - `HeadSink.damaged()` is followed by a single blank line before the module-level comment.

nx2/nup.py

					
				@ -512,0 +524,4 @@

				                        elif expected is not None:

				                            digest = hashlib.sha256(prefix)

				                            for block in iter(lambda: f.read(1 << 20), b""):

				                                digest.update(block)

Nit: an all-zero digest means "no digest" here, but verify() would report the same record as a SHA-256 mismatch. If zero really is the format's "absent" marker, a short comment saying so (or the same rule in verify()) would keep the two from disagreeing.

Nit: an all-zero digest means "no digest" here, but `verify()` would report the same record as a SHA-256 mismatch. If zero really is the format's "absent" marker, a short comment saying so (or the same rule in `verify()`) would keep the two from disagreeing.

nx2/nup.py

This now reads every retained .zs in full (~23 GB on a full dump), not 4 bytes. The Builder never reaches this path because it wipes game/ before extracting, so it's fine. I'd still keep a short comment saying that resume costs a full read, since the old comment about re-runs explained why this branch exists.

This now reads every retained .zs in full (~23 GB on a full dump), not 4 bytes. The Builder never reaches this path because it wipes `game/` before extracting, so it's fine. I'd still keep a short comment saying that resume costs a full read, since the old comment about re-runs explained why this branch exists.