fix: a verified large file keeps its bytes

fetch_large_file verified a cached copy and returned its name, which another run replaces when the release serves a re-upload: the caller then packed bytes it never verified. The returned path is now a hard link under .cache/large/.by-sha1/<sha1>/, kept only if it holds the inode that was hashed. Revisions no name holds are dropped a day later. restore_large_files hashes each copy before moving it into place.

scripts/largefiles.py

@@ -12,6 +12,7 @@ import os
 import sys
 import re
 import tempfile
+import time
 import urllib.error
 import urllib.parse
 import urllib.request
@@ -26,6 +27,13 @@ from hashing import compute_hashes
 LARGE_FILES_RELEASE = "large-files"
 LARGE_FILES_REPO = "Abdess/retrobios"
 LARGE_FILES_CACHE = ".cache/large"
+# Verified revisions, hard-linked as <sha1>/<asset> beside the cache. A name
+# in the cache is replaced when the release serves new bytes under it, so a
+# path handed out by name could hold other bytes by the time the caller read
+# them. Nothing ever writes other bytes under a sha1. The asset name stays
+# last: callers read the extension (an archive is packed as one).
+VERIFIED_STORE = ".by-sha1"
+ORPHAN_GRACE = 24 * 3600
 GITIGNORE = Path(__file__).resolve().parent.parent / ".gitignore"
 
 _UNSAFE_ASSET_CHARS = re.compile(r"[^A-Za-z0-9._-]+")
@@ -169,14 +177,14 @@ def _fetch_asset(
 
     if os.path.isdir(cached):
         return None
-    if os.path.exists(cached) and not (expected_sha1 or expected_md5):
-        return cached
     if os.path.exists(cached):
         snapshot = _read_once(cached)
         if snapshot is None:
             pass
         elif _matches(snapshot[0], expected_sha1, expected_md5):
-            return cached
+            pinned = _pin_verified(cached, snapshot, dest_dir, name)
+            if pinned:
+                return pinned
         elif not _replaced_on_release(name, snapshot[0]["sha256"], snapshot[1], offline):
             # A copy of this asset that answers another hash: the caller
             # wants a different revision under the same name, and the
@@ -220,17 +228,71 @@ def _fetch_asset(
                 print(f"  large file {candidate}: {exc}", file=sys.stderr)
         if not downloaded:
             return None
-        if (expected_sha1 or expected_md5) and not _matches(
-            compute_hashes(tmp_path), expected_sha1, expected_md5
-        ):
+        snapshot = _read_once(tmp_path)
+        if snapshot is None or not _matches(snapshot[0], expected_sha1, expected_md5):
             return None
         set_plain_mode(tmp_path)
+        pinned = _pin_verified(tmp_path, snapshot, dest_dir, name)
         os.replace(tmp_path, cached)
-        return cached
+        _prune_verified(dest_dir)
+        return pinned
     finally:
         _drop(tmp_path)
 
 
+def _pin_verified(
+    path: str, snapshot: tuple[dict[str, str], int, int], dest_dir: str, name: str,
+) -> str | None:
+    """The verified revision at *path*, as <sha1>/<name> in VERIFIED_STORE.
+
+    *snapshot* is what _read_once read: the link is kept only if it holds
+    that inode, so a copy another run promoted after the hash is never
+    passed off as the one verified. None when the name moved in between.
+    """
+    revision = os.path.join(dest_dir, VERIFIED_STORE, snapshot[0]["sha1"])
+    os.makedirs(revision, exist_ok=True)
+    final = os.path.join(revision, name)
+    if os.path.isfile(final):
+        return final
+    scratch = os.path.join(revision, f".{os.getpid()}.{os.urandom(4).hex()}.tmp")
+    try:
+        os.link(path, scratch)
+    except FileNotFoundError:
+        return None
+    try:
+        if os.stat(scratch).st_ino != snapshot[2]:
+            return None
+        os.replace(scratch, final)
+        return final
+    finally:
+        with contextlib.suppress(FileNotFoundError):
+            os.unlink(scratch)
+
+
+def _prune_verified(dest_dir: str) -> None:
+    """Drop the revisions no cache name has held for ORPHAN_GRACE seconds.
+
+    Losing its cache name changes the inode's ctime, so a revision another
+    run pinned just before the release replaced it stays readable while that
+    run packs it.
+    """
+    store = os.path.join(dest_dir, VERIFIED_STORE)
+    now = time.time()
+    for revision in os.scandir(store):
+        if not revision.is_dir():
+            continue
+        for entry in os.scandir(revision.path):
+            if entry.name.endswith(".tmp"):
+                continue
+            with contextlib.suppress(FileNotFoundError):
+                stat = entry.stat()
+                if stat.st_nlink == 1 and now - stat.st_ctime > ORPHAN_GRACE:
+                    os.unlink(entry.path)
+        # Another run may be linking a copy into it: left for the next prune.
+        with contextlib.suppress(FileNotFoundError, OSError):
+            os.rmdir(revision.path)
+
+
 def _asset_urls(name: str) -> list[tuple[str, str]]:
     """Release URLs an asset may be published under, with their names.
 
@@ -252,9 +314,9 @@ def _asset_urls(name: str) -> list[tuple[str, str]]:
     ]
 
 
-def _read_once(path: str) -> tuple[dict[str, str], int] | None:
-    """Digests and size of the file at *path*, all read from one file, or
-    None when it is gone.
+def _read_once(path: str) -> tuple[dict[str, str], int, int] | None:
+    """Digests, size and inode of the file at *path*, all read from one
+    file, or None when it is gone.
 
     Every judgement on a cache copy reads this one snapshot. Hashed, then
     hashed again by path for the release check, a copy another run promoted
@@ -271,7 +333,7 @@ def _read_once(path: str) -> tuple[dict[str, str], int] | None:
         except FileNotFoundError:
             return None
         if (before.st_ino, before.st_mtime_ns) == (after.st_ino, after.st_mtime_ns):
-            return hashes, after.st_size
+            return hashes, after.st_size, after.st_ino
     return None
 
 

scripts/restore_large_files.py

@@ -17,11 +17,14 @@ Usage:
 from __future__ import annotations
 
 import argparse
+import contextlib
 import hashlib
 import os
 import sys
+import tempfile
 
 sys.path.insert(0, os.path.dirname(__file__))
+from artifacts import set_plain_mode
 from common import copy_file_atomic, load_database
 
 
@@ -56,6 +59,36 @@ def index_cache(cache_dir: str) -> dict[str, str]:
     return index
 
 
+def _copy_verified(source: str, path: str, sha1: str) -> bool:
+    """Copy *source* to *path* only if the copy hashes to *sha1*.
+
+    The cache was hashed before the copy, and another run can replace a
+    cached name in between (a re-uploaded asset): the copy is what counts.
+    Whole or absent: a scan running beside a plain copy hashed the truncated
+    file and published that in the database.
+    """
+    handle, scratch = tempfile.mkstemp(
+        dir=os.path.dirname(os.path.abspath(path)),
+        prefix=f".{os.path.basename(path)}.", suffix=".tmp",
+    )
+    os.close(handle)
+    try:
+        copy_file_atomic(source, scratch)
+        digest = hashlib.sha1()
+        with open(scratch, "rb") as f:
+            for chunk in iter(lambda: f.read(1 << 20), b""):
+                digest.update(chunk)
+        if digest.hexdigest() != sha1:
+            print(f"Changed in the cache while restoring: {source}", file=sys.stderr)
+            return False
+        set_plain_mode(scratch)
+        os.replace(scratch, path)
+        return True
+    finally:
+        with contextlib.suppress(FileNotFoundError):
+            os.unlink(scratch)
+
+
 def restore(
     cache_dir: str, db_path: str, gitignore: str
 ) -> tuple[int, list[str]]:
@@ -79,9 +112,9 @@ def restore(
             unsatisfied.append(path)
             continue
         os.makedirs(os.path.dirname(path), exist_ok=True)
-        # Whole or absent: a scan running beside a plain copy hashed the
-        # truncated file and published that in the database.
-        copy_file_atomic(source, path)
+        if not _copy_verified(source, path, sha1):
+            unsatisfied.append(path)
+            continue
         print(f"Restored: {path}")
         restored += 1
     print(f"Total: {restored} files restored")

tests/test_audit_regressions.py

@@ -2054,6 +2054,44 @@ class CheckoutCompletenessRegressions(unittest.TestCase):
             finally:
                 os.chdir(cwd)
 
+    def test_restore_refuses_a_copy_replaced_after_the_index(self):
+        """The cache is hashed first and copied later: another run that
+        replaced the cached name in between put its bytes at a path the
+        database pins to the old ones."""
+        from scripts import restore_large_files
+
+        with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory:
+            root = Path(directory)
+            cache = root / "cache"
+            cache.mkdir()
+            payload = b"firmware bytes"
+            (cache / "asset.bin").write_bytes(payload)
+            sha1 = hashlib.sha1(payload).hexdigest()
+            (root / ".gitignore").write_text("bios/Sony/big.pup\n", encoding="utf-8")
+            (root / "database.json").write_text(
+                json.dumps({"files": {sha1: {"path": "bios/Sony/big.pup"}}}),
+                encoding="utf-8",
+            )
+            real_index = restore_large_files.index_cache
+
+            def index_then_replace(cache_dir: str) -> dict[str, str]:
+                index = real_index(cache_dir)
+                (cache / "asset.bin").write_bytes(b"re-uploaded bytes")
+                return index
+
+            cwd = os.getcwd()
+            os.chdir(root)
+            try:
+                with mock.patch.object(restore_large_files, "index_cache", index_then_replace):
+                    restored, unsatisfied = restore_large_files.restore(
+                        str(cache), "database.json", ".gitignore"
+                    )
+                self.assertEqual((restored, unsatisfied), (0, ["bios/Sony/big.pup"]))
+                self.assertFalse((root / "bios/Sony/big.pup").exists())
+                self.assertEqual(os.listdir(root / "bios/Sony"), [])
+            finally:
+                os.chdir(cwd)
+
     def test_restore_leaves_tracked_paths_alone(self):
         from scripts.restore_large_files import restore
 

tests/test_large_file_cache.py

@@ -158,9 +158,9 @@ class LargeFileCacheTest(unittest.TestCase):
             raise AssertionError("must not download when the cache is valid")
 
         largefiles.urllib.request.urlopen = fail
-        self.assertEqual(
-            common.fetch_large_file("asset.bin", dest_dir=self.dir), str(cached)
-        )
+        self.assertTrue(os.path.samefile(
+            common.fetch_large_file("asset.bin", dest_dir=self.dir), cached
+        ))
 
     def test_offline_cache_miss_never_opens_the_network(self):
         def fail(req, timeout=None):
@@ -182,15 +182,15 @@ class LargeFileCacheTest(unittest.TestCase):
             raise AssertionError("offline cache hit must not open the network")
 
         largefiles.urllib.request.urlopen = fail
-        self.assertEqual(
+        self.assertTrue(os.path.samefile(
             common.fetch_large_file(
                 "asset.bin",
                 dest_dir=self.dir,
                 expected_sha1=hashlib.sha1(PAYLOAD_A).hexdigest(),
                 offline=True,
             ),
-            str(cached),
-        )
+            cached,
+        ))
 
     def _serve(self, payload: bytes, gets: list):
         class _Head(io.BytesIO):
@@ -222,7 +222,7 @@ class LargeFileCacheTest(unittest.TestCase):
             "asset.bin", dest_dir=self.dir,
             expected_sha1=hashlib.sha1(rebuilt).hexdigest(),
         )
-        self.assertEqual(result, str(cached))
+        self.assertTrue(os.path.samefile(result, cached))
         self.assertEqual(cached.read_bytes(), rebuilt)
         self.assertEqual(len(gets), 1)
 
@@ -582,7 +582,7 @@ class ReleaseAssetNames(unittest.TestCase):
                 offline=True,
                 registered=self.REGISTERED,
             )
-            self.assertEqual(got, os.path.join(cache, names[self.RTCW]))
+            self.assertTrue(os.path.samefile(got, os.path.join(cache, names[self.RTCW])))
             # The cache entry of the other asset is a valid file, not stale.
             self.assertTrue(Path(cache, names[self.ET]).exists())
 
@@ -762,7 +762,7 @@ class ASameSizeReuploadIsSeen(unittest.TestCase):
                 "dsi_nand.bin", str(self.dir), hashlib.sha1(new).hexdigest(), "",
                 offline=False,
             )
-        self.assertEqual(found, str(self.cached))
+        self.assertTrue(os.path.samefile(found, self.cached))
 
     def test_the_release_check_compares_digests_at_equal_size(self):
         expected = {"bios/Nintendo/DSi/dsi_nand.bin": 8}
@@ -792,3 +792,49 @@ class ASameSizeReuploadIsSeen(unittest.TestCase):
 
 if __name__ == "__main__":
     unittest.main()
+
+
+class AVerifiedPathKeepsItsBytes(unittest.TestCase):
+    """fetch_large_file verified a cached copy and returned its name. Another
+    run that found the release re-uploaded then replaced that name with the
+    new revision, and the first run packed bytes it never verified, under a
+    manifest sha1 that describes the old ones."""
+
+    def setUp(self):
+        _test_registry(self)
+        self.dir = Path(tempfile.mkdtemp(dir=REPO_ROOT / "tmp"))
+        self.addCleanup(shutil.rmtree, self.dir, True)
+        (self.dir / "asset.bin").write_bytes(PAYLOAD_A)
+        self.served = {"asset.bin": hashlib.sha256(PAYLOAD_B).hexdigest()}
+
+    def _fetch(self, payload: bytes, offline: bool) -> str | None:
+        def urlopen(req, timeout=None):
+            return _SlowResponse(PAYLOAD_B)
+
+        with mock.patch.object(largefiles, "_served_digests", lambda: self.served), \
+                mock.patch.object(largefiles.urllib.request, "urlopen", urlopen):
+            return largefiles.fetch_large_file(
+                "asset.bin", dest_dir=str(self.dir),
+                expected_sha1=hashlib.sha1(payload).hexdigest(), offline=offline,
+            )
+
+    def test_a_revision_promoted_after_the_check_is_not_read(self):
+        first = self._fetch(PAYLOAD_A, offline=True)
+        second = self._fetch(PAYLOAD_B, offline=False)
+        self.assertEqual((self.dir / "asset.bin").read_bytes(), PAYLOAD_B)
+        self.assertEqual(Path(second).read_bytes(), PAYLOAD_B)
+        self.assertEqual(Path(first).read_bytes(), PAYLOAD_A)
+        self.assertEqual(Path(first).name, "asset.bin")
+
+    def test_an_orphaned_revision_is_dropped_after_the_grace(self):
+        first = self._fetch(PAYLOAD_A, offline=True)
+        self._fetch(PAYLOAD_B, offline=False)
+        self.assertTrue(Path(first).exists())
+        with mock.patch.object(largefiles, "ORPHAN_GRACE", -1):
+            largefiles._prune_verified(str(self.dir))
+        self.assertFalse(Path(first).exists())
+        self.assertEqual((self.dir / "asset.bin").read_bytes(), PAYLOAD_B)
+        self.assertEqual(
+            [p.name for p in (self.dir / largefiles.VERIFIED_STORE).iterdir()],
+            [hashlib.sha1(PAYLOAD_B).hexdigest()],
+        )

wiki/release-process.md

@@ -206,7 +206,12 @@ file's own name. A path whose asset name differs from its file name is
 uploaded from a copy carrying the asset name.
 
 **Local cache.** `generate_pack.py` calls `fetch_large_file()` which downloads
-from the release and caches in `.cache/large/` for subsequent runs.
+from the release and caches in `.cache/large/` for subsequent runs. The path it
+returns is a hard link under `.cache/large/.by-sha1/<sha1>/`, never the cached
+name: another run that finds the asset re-uploaded replaces the name, and the
+first run still reads the bytes it verified. A revision no name holds is
+dropped a day later, at the next download. `restore_large_files.py` hashes
+each copy before it moves it into place.
 
 **Check.** The installer refuses a download whose `Content-Length` differs
 from the manifest size, and the manifest size is that of the local file. A