From fed03c3db7ff57a56d44906a277fe6251c986baf Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:47:35 +0200 Subject: [PATCH] fix: pack each zip with the collection's bytes --- scripts/generate_pack.py | 75 ++++++++++++--------------------- tests/test_audit_regressions.py | 33 ++++++++++++--- 2 files changed, 56 insertions(+), 52 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index c43bdb81..319cb8f8 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -80,7 +80,7 @@ import slot as slot_mod import slots import split_pack from gamedata import GAME_DATA, GameDataCut, is_game_data, platform_game_data, without_game_data -from deterministic_zip import _FIXED_DATE_TIME, rebuild_zip_deterministic +from deterministic_zip import _FIXED_DATE_TIME from largefiles import asset_name, load_registered_paths, published_asset_name from nativemode import ( digest_algorithm, @@ -1259,64 +1259,45 @@ def _add_zip_to_pack( target_zf: zipfile.ZipFile, file_entry: dict | None = None, ) -> None: - """Write a source ZIP into the pack. + """Write a source ZIP into the pack, byte for byte. - A declared outer hash (md5/sha1/crc32) means the platform or profile - verifies the ZIP bytes themselves (Batocera md5sum, System.dat), so - the resolved file is copied unchanged: a rebuild would change the - outer hash and fail the native check. Entries with zipped_file - declare the hash of a ROM inside the ZIP (checkInsideZip), which a - content-preserving rebuild does not affect. + The pack carries the collection's own bytes, so every member hashes to + the sha1 the install list and the site publish. Rebuilt ZIPs gave about + 1,500 archives another hash than the list: an extracted pack then failed + `install.py --check`. The collection's bytes are fixed in git, which keeps + the pack reproducible without a rebuild. """ if isinstance(target_zf, PackRecord): target_zf.members.append(dest_path) return - if ( - file_entry - and not file_entry.get("zipped_file") - and ( - file_entry.get("md5") - or file_entry.get("sha1") - or file_entry.get("crc32") + _check_zip_members(source_zip) + _add_pack_member(target_zf, source_zip, dest_path) + + +def _check_zip_members(source_zip: str) -> None: + """Refuse an archive whose members fail their own CRC. + + Copied as-is "for verify to flag", a damaged archive went into the pack + and nothing flagged it. + """ + try: + with zipfile.ZipFile(source_zip) as zf: + bad = zf.testzip() + except zipfile.BadZipFile as exc: + raise DamagedArchiveError( + f"{source_zip}: {exc}; restore it from git or the release before packing" + ) from exc + if bad is not None: + raise DamagedArchiveError( + f"{source_zip}: member {bad} fails its CRC; " + "restore it from git or the release before packing" ) - ): - _add_pack_member(target_zf, source_zip, dest_path) - return - _normalize_zip_for_pack(source_zip, dest_path, target_zf) class DamagedArchiveError(OSError): """An archive of the collection whose members fail their own CRC.""" -def _normalize_zip_for_pack( - source_zip: str, dest_path: str, target_zf: zipfile.ZipFile -) -> None: - """Add a MAME BIOS ZIP to the pack, rebuilt from its ROMs. - - The same ROMs give the same archive bytes in every build, whatever - metadata the source ZIP carried. - """ - import tempfile as _tmp - - # tmp/ is gitignored, so a fresh clone does not carry it. WSL keeps /tmp - # on a 4 GB tmpfs that MAME sets overflow, hence the repo-local scratch. - os.makedirs("tmp", exist_ok=True) - tmp_fd, tmp_path = _tmp.mkstemp(suffix=".zip", dir="tmp") - os.close(tmp_fd) - try: - rebuild_zip_deterministic(source_zip, tmp_path) - _add_pack_member(target_zf, tmp_path, dest_path) - except zipfile.BadZipFile as exc: - # The rebuild checks every member's CRC. Copied as-is "for verify to - # flag", a damaged archive went into the pack and nothing flagged it. - raise DamagedArchiveError( - f"{source_zip}: {exc}; restore it from git or the release before packing" - ) from exc - finally: - os.unlink(tmp_path) - - # Emulator/system mode pack generation diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index f0e5f7c5..f66bf9a8 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -626,14 +626,14 @@ class StandaloneCopiesNameTheirSource(unittest.TestCase): class ADamagedArchiveIsNotPacked(unittest.TestCase): - """The deterministic rebuild raises on a member whose bytes fail their - CRC. The builder caught it and copied the damaged archive into the pack - "for verify to flag", which nothing did.""" + """A member whose bytes fail their CRC stops the build. The builder once + copied the damaged archive into the pack "for verify to flag", which + nothing did.""" def test_the_rebuild_refusal_reaches_the_run(self): import zipfile - from generate_pack import DamagedArchiveError, _normalize_zip_for_pack + from generate_pack import DamagedArchiveError, _add_zip_to_pack with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: source = Path(directory) / "set.zip" @@ -645,12 +645,35 @@ class ADamagedArchiveIsNotPacked(unittest.TestCase): pack = Path(directory) / "pack.zip" with zipfile.ZipFile(pack, "w") as target: with self.assertRaises(DamagedArchiveError) as caught: - _normalize_zip_for_pack(str(source), "set.zip", target) + _add_zip_to_pack(str(source), "set.zip", target) self.assertNotIn("set.zip", target.namelist()) self.assertIsInstance(caught.exception, OSError) self.assertIn("set.zip", str(caught.exception)) +class APackedArchiveKeepsItsBytes(unittest.TestCase): + """Packs rebuilt about 1,500 ZIPs in a fixed byte order while the install + list kept the collection's hash: an extracted pack failed --check on each.""" + + def test_the_member_is_the_source_byte_for_byte(self): + import hashlib + import zipfile + + from generate_pack import _add_zip_to_pack + + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: + source = Path(directory) / "set.zip" + with zipfile.ZipFile(source, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr(zipfile.ZipInfo("rom.bin", (1999, 1, 2, 3, 4, 5)), b"rom" * 40) + pack = Path(directory) / "pack.zip" + with zipfile.ZipFile(pack, "w") as target: + _add_zip_to_pack(str(source), "set.zip", target) + with zipfile.ZipFile(pack) as packed: + inner = packed.read("set.zip") + self.assertEqual(hashlib.sha1(inner).hexdigest(), + hashlib.sha1(source.read_bytes()).hexdigest()) + + class PackReadmesSayWhatTheInstallerDoes(unittest.TestCase): """The EmuDeck README promised keys set up automatically (they need --standalone-copies) and gave a Citra path the installer never uses, and the