diff --git a/scripts/common.py b/scripts/common.py index 79b0e00d..5a49ccca 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -1505,6 +1505,17 @@ def fetch_large_file( MAX_ZIP_MEMBERS = 100_000 +def sanitize_pack_path(raw: str) -> str: + """Strip traversal components from a relative destination. + + The builder and the coverage report key their region grouping on this + value, so they have to derive it the same way: a destination normalized on + one side only would be looked up under a key the other side never emits. + """ + raw = raw.replace("\\", "/") + return "/".join(p for p in raw.split("/") if p and p not in ("..", ".")) + + MAX_ZIP_MEMBER_SIZE = 8 * 1024 * 1024 * 1024 # The largest generated pack is already ~5 GB uncompressed and the collection # only grows; this bounds a malicious archive without capping a real one. diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 85f1eb87..d19e9d16 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -50,6 +50,7 @@ from common import ( require_yaml, resolution_is_hash_exact, resolve_local_file, + sanitize_pack_path, yaml_load, ) import region as region_mod @@ -289,7 +290,7 @@ def _pack_member_groups( for sys_id, system in pack_systems.items(): members = groups.setdefault(sys_id, []) for fe in system.get("files", []): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if dest: members.append((dest, fe.get("name", ""))) if source == "platform": @@ -302,7 +303,7 @@ def _pack_member_groups( config, emulators_dir, db, set(), base_dest, emu_profiles, target_cores=target_cores, include_all=(source == "truth"), ): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest: continue for sys_id in emu_systems.get(fe.get("source_emulator", ""), ["_extras"]): @@ -361,13 +362,6 @@ def _target_tag(target_name: str) -> str: ) -def _sanitize_path(raw: str) -> str: - """Strip path traversal components from a relative path.""" - raw = raw.replace("\\", "/") - parts = [p for p in raw.split("/") if p and p not in ("..", ".")] - return "/".join(parts) - - def _path_parents(dest: str) -> list[str]: """Return all parent directory segments of a path.""" parts = dest.split("/") @@ -709,6 +703,12 @@ def _collect_emulator_extras( "hle_fallback": u.get("hle_fallback", False), "source_emulator": u.get("emulator", ""), "source_profile": u.get("profile", ""), + # Identity of the report entry this extra came from. The name + # alone does not identify it: Dolphin declares three IPL.bin that + # differ only by path, and collapsing them onto one key withdraws + # the wrong regional variant from the report. + "source_name": u.get("name", ""), + "source_path": u.get("path") or "", "source_system": u.get("system"), "source_systems": u.get("systems", []), "region": u.get("region"), @@ -951,6 +951,68 @@ def _extra_system_ids(extra: dict) -> list[str]: return [str(value) for value in extra.get("source_systems", []) if value] +def platform_region_groups( + config: dict, + systems: dict, + emulators_dir: str, + db: dict | None, + base_dest: str, + emu_profiles: dict | None, + *, + target_cores: set[str] | None = None, + include_extras: bool = True, + include_all: bool = False, +) -> tuple[dict[str, list[tuple[str, str]]], dict[tuple[str, str, str], str]]: + """Group a platform's pack candidates the way region filtering reads them. + + Returns the groups and, for every core extra, the destination it was + grouped under, keyed by (emulator, name). verify.py needs that mapping to + withdraw from its report exactly what the builder withdraws from the pack: + grouping the declared files here and the core extras there would let the + two answer differently on the same request. + """ + groups: dict[str, list[tuple[str, str]]] = {} + for sys_id, system in systems.items(): + members = groups.setdefault(sys_id, []) + for file_entry in system.get("files", []): + dest = sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) + if dest: + members.append((dest, file_entry.get("name", ""))) + + extra_dests: dict[tuple[str, str, str], str] = {} + if not include_extras or db is None: + return groups, extra_dests + + for extra in _collect_emulator_extras( + config, + emulators_dir, + db, + set(), + base_dest, + emu_profiles, + target_cores=target_cores, + include_all=include_all, + ): + dest = sanitize_pack_path(extra.get("destination", extra.get("name", ""))) + if not dest: + continue + name = extra.get("name", "") + extra_dests[ + ( + extra.get("source_emulator", ""), + extra.get("source_name", ""), + extra.get("source_path", ""), + ) + ] = dest + variant = extra.get("variant_group") + for sys_id in _extra_system_ids(extra) or ["_extras"]: + group_id = f"{sys_id}:variant:{variant}" if variant else sys_id + groups.setdefault(group_id, []).append((dest, name)) + return groups, extra_dests + + def _emulator_region_group(emu_name: str, profile: dict, file_entry: dict) -> str: """Stable group ID for regional alternatives within an emulator profile.""" variant = file_entry.get("variant_group") @@ -1403,7 +1465,7 @@ def generate_pack( for file_entry in system.get("files", []): if required_only and file_entry.get("required") is False: continue - dest = _sanitize_path( + dest = sanitize_pack_path( file_entry.get("destination", file_entry.get("name", "")) ) if not dest: @@ -1447,36 +1509,17 @@ def generate_pack( region_fallbacks: list[str] = [] if regions: region_index = region_mod.build_region_index(emu_profiles or {}) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in pack_systems.items(): - members = region_groups.setdefault(sys_id, []) - for file_entry in system.get("files", []): - dest = _sanitize_path( - file_entry.get("destination", file_entry.get("name", "")) - ) - if dest: - members.append((dest, file_entry.get("name", ""))) - if source != "platform": - for fe in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - target_cores=target_cores, - include_all=(source == "truth"), - ): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if not dest: - continue - systems = _extra_system_ids(fe) or ["_extras"] - for sys_id in systems: - variant = fe.get("variant_group") - group_id = f"{sys_id}:variant:{variant}" if variant else sys_id - region_groups.setdefault(group_id, []).append( - (dest, fe.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + pack_systems, + emulators_dir, + db, + base_dest, + emu_profiles, + target_cores=target_cores, + include_extras=(source != "platform"), + include_all=(source == "truth"), + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -1510,7 +1553,7 @@ def generate_pack( for file_entry in system.get("files", []): if required_only and file_entry.get("required") is False: continue - dest = _sanitize_path(file_entry.get("destination", file_entry["name"])) + dest = sanitize_pack_path(file_entry.get("destination", file_entry["name"])) if region_drops and dest in region_drops: continue if not dest: @@ -1827,7 +1870,7 @@ def generate_pack( for fe in core_files: if required_only and fe.get("required") is False: continue - dest = _sanitize_path(fe.get("destination", fe["name"])) + dest = sanitize_pack_path(fe.get("destination", fe["name"])) if region_drops and dest in region_drops: continue if not dest: @@ -2020,7 +2063,7 @@ def _extract_zip_to_archive( for info in src.infolist(): if info.is_dir(): continue - clean_name = _sanitize_path(info.filename) + clean_name = sanitize_pack_path(info.filename) if not clean_name: continue data = src.read(info.filename) @@ -2105,7 +2148,7 @@ def _resolve_destination( else: rel = file_entry.get("name", "") - rel = _sanitize_path(rel) + rel = sanitize_pack_path(rel) # Prepend pack_structure prefix if pack_structure: @@ -2264,7 +2307,7 @@ def generate_emulator_pack( # Pack archives as units archive_prefix = profile.get("archive_prefix", "") for archive_name in sorted(archives): - archive_dest = _sanitize_path(archive_name) + archive_dest = sanitize_pack_path(archive_name) if archive_prefix: archive_dest = f"{archive_prefix}/{archive_dest}" if pack_structure: @@ -3661,36 +3704,17 @@ def generate_manifest( region_drops: set[str] = set() if regions: region_index = region_mod.build_region_index(emu_profiles) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in pack_systems.items(): - members = region_groups.setdefault(sys_id, []) - for file_entry in system.get("files", []): - d = _sanitize_path( - file_entry.get("destination", file_entry.get("name", "")) - ) - if d: - members.append((d, file_entry.get("name", ""))) - if source != "platform": - for fe in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - target_cores=target_cores, - include_all=(source == "truth"), - ): - d = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if not d: - continue - systems = _extra_system_ids(fe) or ["_extras"] - for sid in systems: - variant = fe.get("variant_group") - group_id = f"{sid}:variant:{variant}" if variant else sid - region_groups.setdefault(group_id, []).append( - (d, fe.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + pack_systems, + emulators_dir, + db, + base_dest, + emu_profiles, + target_cores=target_cores, + include_extras=(source != "platform"), + include_all=(source == "truth"), + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -3699,7 +3723,7 @@ def generate_manifest( if source != "truth": for sys_id, system in sorted(pack_systems.items()): for file_entry in system.get("files", []): - dest = _sanitize_path(file_entry.get("destination", file_entry["name"])) + dest = sanitize_pack_path(file_entry.get("destination", file_entry["name"])) if not dest: continue if region_drops and dest in region_drops: @@ -3806,7 +3830,7 @@ def generate_manifest( core_files = [] extras_pfx = _detect_extras_prefix(config, base_dest) for fe in core_files: - dest = _sanitize_path(fe.get("destination", fe["name"])) + dest = sanitize_pack_path(fe.get("destination", fe["name"])) if not dest: continue if region_drops and dest in region_drops: @@ -4385,36 +4409,14 @@ def verify_pack_against_platform( region_drops: set[str] = set() if regions: region_index = region_mod.build_region_index(emu_profiles) - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in config.get("systems", {}).items(): - members = region_groups.setdefault(sys_id, []) - for fe in system.get("files", []): - d = _sanitize_path(fe.get("destination", fe.get("name", ""))) - if d: - members.append((d, fe.get("name", ""))) - if db is not None: - for extra in _collect_emulator_extras( - config, - emulators_dir, - db, - set(), - base_dest, - emu_profiles, - ): - d = _sanitize_path( - extra.get("destination", extra.get("name", "")) - ) - if not d: - continue - systems_for_extra = _extra_system_ids(extra) or ["_extras"] - for sys_id in systems_for_extra: - variant = extra.get("variant_group") - group_id = ( - f"{sys_id}:variant:{variant}" if variant else sys_id - ) - region_groups.setdefault(group_id, []).append( - (d, extra.get("name", "")) - ) + region_groups, _extra_dests = platform_region_groups( + config, + config.get("systems", {}), + emulators_dir, + db, + base_dest, + emu_profiles, + ) region_drops = region_mod.resolve_region_drops( region_groups, region_index, regions ) @@ -4468,7 +4470,7 @@ def verify_pack_against_platform( baseline_groups: dict[str, list[dict]] = {} for _sys_id, system in config.get("systems", {}).items(): for fe in system.get("files", []): - dest = _sanitize_path(fe.get("destination", fe.get("name", ""))) + dest = sanitize_pack_path(fe.get("destination", fe.get("name", ""))) if not dest: continue if region_drops and dest in region_drops: @@ -4555,10 +4557,10 @@ def verify_pack_against_platform( extras_pfx = _detect_extras_prefix(config, base_dest) for fe in core_files: raw_dest = fe.get("destination", fe.get("name", "")) - dest = _sanitize_path(raw_dest) + dest = sanitize_pack_path(raw_dest) if not dest: continue - if region_drops and _sanitize_path(dest) in region_drops: + if region_drops and sanitize_pack_path(dest) in region_drops: continue if extras_pfx and not (is_flat and extras_pfx == base_dest): if not dest.startswith(f"{extras_pfx}/"): diff --git a/scripts/verify.py b/scripts/verify.py index 53559158..b3e8317d 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -49,6 +49,7 @@ from common import ( require_yaml, resolve_local_file, resolve_platform_cores, + sanitize_pack_path, ) yaml = require_yaml() @@ -803,16 +804,25 @@ def verify_platform( file_severity: dict[str, str] = {} region_drops: set[str] = set() + region_extra_dests: dict[tuple[str, str, str], str] = {} if regions: import region as region_mod - region_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, system in verify_systems.items(): - members = region_groups.setdefault(sys_id, []) - for fe in system.get("files", []): - dest = fe.get("destination", fe.get("name", "")) - if dest: - members.append((dest, fe.get("name", ""))) + # The builder owns pack composition, so it owns the grouping the region + # pass reads. Grouping the platform files here and the core extras + # there let the two answer differently on one request: the report kept + # every core extra a region run withdraws from the pack. + from generate_pack import platform_region_groups + + region_groups, region_extra_dests = platform_region_groups( + config, + verify_systems, + emulators_dir, + db, + config.get("base_destination", ""), + profiles, + target_cores=target_cores, + ) region_drops = region_mod.resolve_region_drops( region_groups, region_mod.build_region_index(profiles), regions ) @@ -820,7 +830,9 @@ def verify_platform( for sys_id, system in verify_systems.items(): for file_entry in system.get("files", []): if region_drops and ( - file_entry.get("destination", file_entry.get("name", "")) + sanitize_pack_path( + file_entry.get("destination", file_entry.get("name", "")) + ) in region_drops ): continue @@ -916,6 +928,15 @@ def verify_platform( target_cores=target_cores, data_names=supplemental_names, ) + if region_drops: + undeclared = [ + u + for u in undeclared + if region_extra_dests.get( + (u.get("emulator", ""), u.get("name", ""), u.get("path") or "") + ) + not in region_drops + ] exclusions = find_exclusion_notes( config, emulators_dir, emu_profiles, target_cores=target_cores ) diff --git a/tests/test_region.py b/tests/test_region.py index f1e25646..94bd4ec2 100644 --- a/tests/test_region.py +++ b/tests/test_region.py @@ -5,6 +5,7 @@ from __future__ import annotations import os import sys import unittest +from pathlib import Path sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) @@ -450,5 +451,119 @@ class TestVerifyModesHonourRegion(unittest.TestCase): self.assertNotEqual(plain, filtered) +class TestReportAndBuilderNarrowTogether(unittest.TestCase): + """A region run must withdraw the same files from report and pack. + + The two sides grouped their candidates separately: the builder grouped the + platform files and the core extras, the report only the platform files. + So the report kept every core extra a region run withdraws from the pack + -on recalbox --region us, 59 files it described as covered. + """ + + def _fixture(self): + import hashlib + import tempfile + + tmp = tempfile.TemporaryDirectory() + root = Path(tmp.name) + (root / "emulators").mkdir() + db = {"files": {}, "indexes": { + "by_name": {}, "by_md5": {}, "by_sha256": {}, + "by_crc32": {}, "by_path_suffix": {}, + }} + # Two regional alternatives of one system, both held locally. + for fname, tag in (("us.bin", b"US BYTES"), ("jp.bin", b"JP BYTES")): + blob = root / fname + blob.write_bytes(tag) + sha1 = hashlib.sha1(tag).hexdigest() + db["files"][sha1] = { + "path": str(blob), "name": fname, "size": len(tag), + "sha1": sha1, "md5": hashlib.md5(tag).hexdigest(), + "sha256": hashlib.sha256(tag).hexdigest(), "crc32": "00000000", + } + db["indexes"]["by_name"][fname] = [sha1] + db["indexes"]["by_md5"][db["files"][sha1]["md5"]] = sha1 + (root / "emulators" / "demo.yml").write_text( + "emulator: demo\n" + "type: libretro\n" + "display_name: Demo\n" + "systems: [demo-system]\n" + "cores: [demo]\n" + "files:\n" + " - name: us.bin\n" + " system: demo-system\n" + " region: [north-america]\n" + " required: true\n" + " - name: jp.bin\n" + " system: demo-system\n" + " region: [japan]\n" + " required: true\n" + ) + config = { + "platform": "Demo", + "verification_mode": "existence", + "cores": ["demo"], + "base_destination": "", + "systems": {}, + } + return tmp, root, db, config + + def test_the_report_withdraws_what_the_builder_withdraws(self): + sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) + import common + import generate_pack as builder + import verify as reporter + + tmp, root, db, config = self._fixture() + try: + common._emulator_profiles_cache.clear() + profiles = common.load_emulator_profiles(str(root / "emulators")) + groups, extra_dests = builder.platform_region_groups( + config, config["systems"], str(root / "emulators"), + db, "", profiles, + ) + drops = region.resolve_region_drops( + groups, region.build_region_index(profiles), ["north-america"] + ) + self.assertEqual( + {d for d in drops}, {"jp.bin"}, + "the builder must drop the Japanese alternative, not the US one", + ) + + plain = reporter.verify_platform( + config, db, str(root / "emulators"), profiles, + supplemental_names=set(), + ) + narrowed = reporter.verify_platform( + config, db, str(root / "emulators"), profiles, + supplemental_names=set(), regions=["north-america"], + ) + before = {u["name"] for u in plain["undeclared_files"]} + after = {u["name"] for u in narrowed["undeclared_files"]} + self.assertEqual(before, {"us.bin", "jp.bin"}) + self.assertEqual( + before - after, + {d for d in drops}, + "the report must withdraw exactly the builder's drop set", + ) + finally: + common._emulator_profiles_cache.clear() + tmp.cleanup() + + def test_one_grouping_pass_serves_both_sides(self): + """A second hand-rolled grouping is how the two drifted apart.""" + scripts = Path(__file__).resolve().parent.parent / "scripts" + hand_rolled = 0 + for name in ("generate_pack.py", "verify.py"): + for line in (scripts / name).read_text().splitlines(): + if "region_groups.setdefault(" in line: + hand_rolled += 1 + self.assertLessEqual( + hand_rolled, 2, + "platform region grouping belongs to platform_region_groups; " + "the only other pass is the per-emulator pack shape", + ) + + if __name__ == "__main__": unittest.main()