From d165005a78ab02d3e1987011ea97916c98d85cce Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sat, 17 Oct 2026 19:25:41 +0200 Subject: [PATCH] fix: lock the installer's partial downloads --- install.ps1 | 2 +- install.py | 76 ++++++++++++++++++++++++++------ install.sh | 2 +- tests/test_install_experience.py | 24 ++++++++++ 4 files changed, 88 insertions(+), 16 deletions(-) diff --git a/install.ps1 b/install.ps1 index cc260a6d..d4ce326d 100644 --- a/install.ps1 +++ b/install.ps1 @@ -8,7 +8,7 @@ param( $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -$defaultInstallSha256 = "8c0e9c762d2fce4460e4ee097055dc0c51f068363ce20583652f7fe6b4a7ee40" +$defaultInstallSha256 = "351450de7dc16195231c2065aef1d1ee72a2231d9a5d7af158dc199099b198ad" $maximumInstallerBytes = 2MB $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } $temporary = $null diff --git a/install.py b/install.py index 71a0453f..0286ece4 100644 --- a/install.py +++ b/install.py @@ -1888,27 +1888,38 @@ def _download_one( tmp_path: Path | None = None counted = 0 resumed_count = 0 + partial = None try: offset = 0 if resumable: tmp_path = dest.parent / f".{dest.name}.part" - offset = _partial_size(tmp_path) - if offset >= expected_size: - tmp_path.unlink() - offset = 0 + _partial_size(tmp_path) + # Two runs into one BIOS folder used to append to the same + # partial copy: the one that finds it locked downloads into + # a private file instead. + partial = _claim_partial(tmp_path) + if partial is None: + tmp_path = None + else: + offset = os.fstat(partial.fileno()).st_size + if offset >= expected_size: + partial.truncate(0) + offset = 0 request = urllib.request.Request(url) if offset: request.add_header("Range", f"bytes={offset}-") with urllib.request.urlopen(request, timeout=60) as resp: if offset and getattr(resp, "status", 200) != 206: offset = 0 # the server sent the whole file again + partial.truncate(0) raw_length = resp.headers.get("Content-Length") if resp.headers else None if raw_length and int(raw_length) != expected_size - offset: raise ValueError( f"Content-Length {raw_length} != expected {expected_size - offset}" ) - if resumable: - out = _open_partial(tmp_path, append=bool(offset)) + if partial is not None: + partial.seek(offset) + out = _Unclosed(partial) else: out = tempfile.NamedTemporaryFile( mode="wb", dir=dest.parent, prefix=f".{dest.name}.", @@ -1952,6 +1963,10 @@ def _download_one( # file is: the temporary file is private, and a BIOS folder # written with sudo has to stay readable by the emulator's user. os.chmod(tmp_path, (0o777 if f.get("executable") else 0o666) & ~_UMASK) + if partial is not None and os.name == "nt": + # Windows renames no open file; elsewhere the lock is held + # until the copy has its final name. + partial.close() os.replace(tmp_path, dest) return f["dest"], True, "" @@ -1959,7 +1974,7 @@ def _download_one( # Stopped mid-transfer: a large file keeps its partial copy for # the next run; a small one is dropped rather than left beside # the destination. - if tmp_path is not None and not resumable and tmp_path.exists(): + if tmp_path is not None and partial is None and tmp_path.exists(): tmp_path.unlink() return f["dest"], False, "stopped" @@ -1976,7 +1991,7 @@ def _download_one( reason += " (GitHub rate limit or access refused)" if verbose: print(f" Attempt {attempt} failed: {exc}", file=sys.stderr) - if tmp_path is not None and not resumable and tmp_path.exists(): + if tmp_path is not None and partial is None and tmp_path.exists(): tmp_path.unlink() if exc.code == 416 and tmp_path is not None and tmp_path.exists(): tmp_path.unlink() # the partial copy no longer fits the file @@ -1991,10 +2006,14 @@ def _download_one( reason = str(getattr(exc, "reason", "") or exc) if verbose: print(f" Attempt {attempt} failed: {exc}", file=sys.stderr) - if not refused and tmp_path is not None and not resumable and tmp_path.exists(): + if not refused and tmp_path is not None and partial is None and tmp_path.exists(): tmp_path.unlink() delay = _retry_delay(attempt, None) + finally: + if partial is not None: + partial.close() + if progress is not None and counted: progress.add(-counted) if resumed_count: @@ -2019,11 +2038,40 @@ def _partial_size(path: Path) -> int: return 0 -def _open_partial(path: Path, append: bool): - """Open the partial copy without following a link planted at its name.""" - flags = os.O_WRONLY | os.O_CREAT | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) - flags |= os.O_APPEND if append else os.O_TRUNC - return os.fdopen(os.open(path, flags, 0o644), "ab" if append else "wb") +def _claim_partial(path: Path): + """The partial copy, opened and locked for this run, or None when + another run holds it. A link planted at its name is not followed.""" + flags = os.O_RDWR | os.O_CREAT | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) + handle = os.fdopen(os.open(path, flags, 0o644), "r+b") + try: + if os.name == "nt": + import msvcrt + + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) + else: + import fcntl + + fcntl.flock(handle.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + handle.close() + return None + return handle + + +class _Unclosed: + """The locked partial copy, written through without closing it.""" + + def __init__(self, handle) -> None: + self._handle = handle + + def __enter__(self): + return self._handle + + def __exit__(self, *_exc) -> None: + self._handle.flush() + + def write(self, data: bytes) -> int: + return self._handle.write(data) class _Corrupt(Exception): diff --git a/install.sh b/install.sh index 5fc64e0b..5cbc7c5f 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="8c0e9c762d2fce4460e4ee097055dc0c51f068363ce20583652f7fe6b4a7ee40" +DEFAULT_INSTALL_SHA256="351450de7dc16195231c2065aef1d1ee72a2231d9a5d7af158dc199099b198ad" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install_experience.py b/tests/test_install_experience.py index 3b4946c1..289bdf6a 100644 --- a/tests/test_install_experience.py +++ b/tests/test_install_experience.py @@ -121,6 +121,30 @@ class LargeFilesResume(unittest.TestCase): self.assertEqual(_RangeHandler.ranges, [""]) +class TwoRunsOneFolder(unittest.TestCase): + """Two installs into one BIOS folder appended to the same partial copy: + each hashed the other's bytes and one renamed a file still growing.""" + + @unittest.skipIf(os.name == "nt", "flock semantics") + def test_a_partial_held_by_another_run_is_left_alone(self): + with _Server(), tempfile.TemporaryDirectory() as tmp, \ + mock.patch.object(install, "RESUME_FROM_SIZE", 1024): + part = Path(tmp) / ".big.bin.part" + part.write_bytes(PAYLOAD[:40000]) + other = install._claim_partial(part) + self.assertIsNotNone(other) + try: + dest, ok, reason = install._download_one(dict(ENTRY), Path(tmp)) + self.assertEqual((ok, reason), (True, "")) + self.assertEqual((Path(tmp) / "big.bin").read_bytes(), PAYLOAD) + self.assertEqual(part.read_bytes(), PAYLOAD[:40000]) + self.assertEqual(_RangeHandler.ranges, [""]) + self.assertIsNone(install._claim_partial(part)) + finally: + other.close() + self.assertIsNotNone(install._claim_partial(part)) + + class InstalledFileModes(unittest.TestCase): """A file came out 0600, the mode of the temporary it was written to: run with sudo for a root-owned BIOS folder, the emulator's own user could not