From ac47faf7f2a65ce4dbe9f45379e01cc32bd683a5 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 20 Oct 2026 02:11:53 +0200 Subject: [PATCH] fix: the pack check applies the frontend's rule The integrity step compared a member's md5 by hand and ignored hash_rule, so under RomM's rule (size, then any of md5, sha1 and crc32) it would refuse a revision the builder rightly shipped on its sha1. The rule now lives in digest_verdict, which the path check and the pack member check both call. The member's archive digest is computed without the path cache, whose key a buffer's reused address made collide. --- scripts/hashing.py | 18 ++++++++--- scripts/packverify.py | 53 +++++++++++++++++++-------------- scripts/validation.py | 52 ++++++++++++++++++++++++-------- tests/test_audit_regressions.py | 28 +++++++++++++++++ 4 files changed, 111 insertions(+), 40 deletions(-) diff --git a/scripts/hashing.py b/scripts/hashing.py index 3addb3fe..c6d89736 100644 --- a/scripts/hashing.py +++ b/scripts/hashing.py @@ -10,6 +10,7 @@ import os import zipfile import zlib from pathlib import Path +from typing import BinaryIO _ALL_ALGORITHMS = frozenset({"sha1", "md5", "sha256", "crc32", "adler32"}) @@ -102,7 +103,18 @@ def md5_composite(filepath: str | Path) -> str: cached = _md5_composite_cache.get(key) if cached is not None: return cached - with zipfile.ZipFile(filepath) as zf: + result = md5_composite_of(filepath) + _md5_composite_cache[key] = result + return result + + +def md5_composite_of(source: str | Path | BinaryIO) -> str: + """md5_composite without the cache, for an archive held in memory. + + A buffer has no stable name: keyed by its repr, a later buffer at the + same address read back another archive's digest. + """ + with zipfile.ZipFile(source) as zf: names = sorted(n for n in zf.namelist() if not n.endswith("/")) h = hashlib.md5() for name in names: @@ -110,9 +122,7 @@ def md5_composite(filepath: str | Path) -> str: if info.file_size > 512 * 1024 * 1024: continue # skip oversized entries h.update(zf.read(name)) - result = h.hexdigest() - _md5_composite_cache[key] = result - return result + return h.hexdigest() def parse_hash_list(raw: str | list | int | None) -> list[str]: """Normalize any hash field into its accepted values, lowercase. diff --git a/scripts/packverify.py b/scripts/packverify.py index 656b25bf..94c13046 100644 --- a/scripts/packverify.py +++ b/scripts/packverify.py @@ -17,12 +17,14 @@ from packpaths import _register_path from ziptools import build_zip_contents_index from ziptools import check_inside_zip from nativemode import digest_algorithm -from validation import baseline_verdict +from validation import baseline_verdict, digest_verdict from nativemode import hash_mismatch_excludes_file import functools import hashlib +import zlib +import sys from common import filter_systems_by_target -from hashing import parse_md5_list +from hashing import md5_composite_of, parse_md5_list import io from common import load_emulator_profiles from common import load_platform_config @@ -328,7 +330,7 @@ def _hash_matches(declared: str, actual: str) -> bool: return False def _check_member_hash( - zf: zipfile.ZipFile, member: str, file_entry: dict, mode: str + zf: zipfile.ZipFile, member: str, file_entry: dict, mode: str, rule: str = "" ) -> str | None: """Verify a pack member against its platform-declared hash. @@ -363,31 +365,35 @@ def _check_member_hash( ) return None - h = hashlib.md5() if mode == "md5" else hashlib.sha1() + # The frontend's own rule, as the builder and verify apply it: RomM's + # size-then-any-hash included, which a bare md5 comparison refused. + digests = {"md5": hashlib.md5(), "sha1": hashlib.sha1()} + crc = 0 with zf.open(member) as f: for chunk in iter(lambda: f.read(65536), b""): - h.update(chunk) - actual = h.hexdigest() - if _hash_matches(declared, actual): - return None + for h in digests.values(): + h.update(chunk) + crc = zlib.crc32(chunk, crc) + actual = {name: h.hexdigest() for name, h in digests.items()} + actual["crc32"] = format(crc & 0xFFFFFFFF, "08x") - # Recalbox Md5Composite: MD5 over sorted inner contents of a ZIP, - # independent of compression and metadata (Zip::Md5Composite()). - if mode == "md5" and member.endswith(".zip"): + def composite() -> str | None: + # Recalbox's Md5Composite, over the sorted members of a ZIP. + if not member.endswith(".zip"): + return None import io try: - with zipfile.ZipFile(io.BytesIO(zf.read(member))) as inner: - names = sorted(n for n in inner.namelist() if not n.endswith("/")) - ch = hashlib.md5() - for n in names: - ch.update(inner.read(n)) - if _hash_matches(declared, ch.hexdigest()): - return None - except (zipfile.BadZipFile, OSError): - pass + return md5_composite_of(io.BytesIO(zf.read(member))) + except (zipfile.BadZipFile, OSError) as exc: + print(f" {member}: no member digest ({exc})", file=sys.stderr) + return None - return f"{member}: {mode} {actual} != declared {declared}" + if digest_verdict( + file_entry, zf.getinfo(member).file_size, lambda: actual, mode, rule, composite + ): + return None + return f"{member}: {mode} {actual[mode]} != declared {declared}" def _repo_satisfies_declaration( entries: list[dict], db: dict, mode: str @@ -666,6 +672,7 @@ def verify_pack_against_platform( # one, the pack ships its best effort and the gap is a data issue # reported by verify.py, not a generation bug. digest = digest_algorithm(verification_mode) + hash_rule = config.get("hash_rule", "") if reads_file_contents(verification_mode) and digest: for member, decl_entries in decl_by_member.items(): checkable = [ @@ -682,13 +689,13 @@ def verify_pack_against_platform( for fe in checkable: if not fe.get("zipped_file"): continue - err = _check_member_hash(zf, member, fe, digest) + err = _check_member_hash(zf, member, fe, digest, hash_rule) if err is not None and (db is None or _repo_holds_member(fe, db)): errors.append(err) member_errors = [] satisfied = False for fe in checkable: - err = _check_member_hash(zf, member, fe, digest) + err = _check_member_hash(zf, member, fe, digest, hash_rule) if err is None: satisfied = True break diff --git a/scripts/validation.py b/scripts/validation.py index 34fc6ec6..56145e55 100644 --- a/scripts/validation.py +++ b/scripts/validation.py @@ -9,7 +9,7 @@ from __future__ import annotations import os import zipfile -from collections.abc import Iterator, Mapping +from collections.abc import Callable, Iterator, Mapping from common import ( adler32_of, @@ -652,13 +652,43 @@ def frontend_digest_matches( match and then any one of md5, sha1 and crc32 does: a mistyped hash next to a right one does not withhold the file, a wrong size does. """ + if not local_path: + return False + + def composite() -> str | None: + if ".zip" not in os.path.basename(local_path): + return None + try: + return md5_composite(local_path) + except (zipfile.BadZipFile, OSError): + return None + + return digest_verdict( + file_entry, os.path.getsize(local_path), lambda: compute_hashes(local_path), + algorithm, rule, composite, + ) + + +def digest_verdict( + file_entry: dict, + size: int, + hashes: Callable[[], dict[str, str]], + algorithm: str, + rule: str = "", + composite: Callable[[], str | None] = lambda: None, +) -> bool: + """The frontend's acceptance rule over content given by its size, its + digests and, for an archive, its member digest. + + One rule for a file on disk and for a member of a built pack: the pack + check once compared only the md5 and refused, under RomM's rule, the + revision the builder had rightly shipped on its sha1 and size. + """ if rule == SIZE_AND_ANY_HASH: - if not local_path: - return False # RomM reads an absent size as 0: such an entry never verifies. - if os.path.getsize(local_path) != int(file_entry.get("size") or 0): + if size != int(file_entry.get("size") or 0): return False - actual = compute_hashes(local_path) + actual = hashes() return any( actual[field].lower() == str(file_entry[field]).strip().lower() for field in ("md5", "sha1", "crc32") @@ -670,19 +700,15 @@ def frontend_digest_matches( value = file_entry.get(algorithm) values = value if isinstance(value, list) else [value] declared = [str(v).strip().lower() for v in values if v] - if not declared or not local_path: + if not declared: return False - actual = compute_hashes(local_path)[algorithm].lower() + actual = hashes()[algorithm].lower() if algorithm != "md5": return actual in declared if any(actual == d or (len(d) < 32 and actual.startswith(d)) for d in declared): return True - if ".zip" not in os.path.basename(local_path): - return False - try: - return md5_composite(local_path).lower() in declared - except (zipfile.BadZipFile, OSError): - return False + member_digest = composite() + return bool(member_digest) and member_digest.lower() in declared def inner_rom_check(file_entry: dict, local_path: str) -> str: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 5e937603..9974e252 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -5327,5 +5327,33 @@ class TheCliSetsBundledFilesApart(unittest.TestCase): self.assertIn("n64.psh [BUNDLED]", text) +class ThePackCheckAppliesTheFrontendRule(unittest.TestCase): + """The integrity step compared a member's md5 alone. Under RomM's rule + (size, then any of md5, sha1 and crc32) the builder ships a revision + matched on its sha1, and the check then refused a correct pack.""" + + def test_romm_accepts_on_size_and_sha1(self): + import hashlib + import zipfile + + from scripts.packverify import _check_member_hash + + payload = b"revision B" + entry = { + "md5": "0" * 32, + "sha1": hashlib.sha1(payload).hexdigest(), + "size": len(payload), + } + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as tmp: + pack = Path(tmp) / "p.zip" + with zipfile.ZipFile(pack, "w") as zf: + zf.writestr("bios/x.bin", payload) + with zipfile.ZipFile(pack) as zf: + self.assertIsNone( + _check_member_hash(zf, "bios/x.bin", entry, "md5", "size_and_any_hash") + ) + self.assertIsNotNone(_check_member_hash(zf, "bios/x.bin", entry, "md5")) + + if __name__ == "__main__": unittest.main()