diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ba0175..f128e81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,23 @@ `cjson` (v1.7.18), `nanopb` (v0.4.9.1), `lvgl` (v9.2.2), `tinyusb` (v0.18.0), and `unity` (v2.6.1). ### Fixed +- **Package archive downloads can no longer hang or poison the cache.** + `PackageFetcher` downloaded with `urlretrieve()`, which has no timeout: one + unresponsive mirror left `ebuild build` hanging until the user killed it. + Downloads are now streamed through `urllib.request.urlopen(..., timeout=...)` + (30 s default, configurable via `PackageFetcher(..., timeout=...)`), refuse + anything larger than 512 MB while streaming (so a server that lies about or + omits `Content-Length` still cannot fill the disk), and are written to a + `.part` file renamed into place only when complete — a connection that dies + mid-body no longer leaves a truncated archive in the download cache to + satisfy every later fetch (`ebuild/packages/fetcher.py`). +- **Archive fetching is now gated in offline mode.** `EBUILD_OFFLINE=1` and + `ebuild update-index --offline` already governed index synchronization; + package archive fetching was exempt and hit the network anyway. A fetch + whose archive is not already in the download cache now fails with a message + naming the missing archive; a cached archive still extracts offline, so + air-gapped rebuilds work from a warmed cache + (`ebuild/packages/fetcher.py`). - **`ebuild test` now finds Windows test binaries.** The Ninja edge for a native `type: test` target already carried the platform suffix (`_exe_suffix()` names it `.exe` on Windows), but `ebuild test` diff --git a/TASKS.md b/TASKS.md index 09f6bcb..ba842ec 100644 --- a/TASKS.md +++ b/TASKS.md @@ -15,6 +15,7 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | T-003 | `ebuild package` looks for the unsuffixed binary on Windows (`_build/app` rather than `_build/app.exe`) | backend | Maintenance | review | none | | T-004 | `_report_footprint` (the flash/RAM report `ebuild build` prints) looks for the unsuffixed binary on Windows, and fails silently rather than logging why | backend | Maintenance | review | none | | T-005 | Move `executable_output_path()` out of the Ninja-specific backend into a backend-neutral module (`ebuild/build/layout.py`), re-exported from `ninja_backend` for compatibility | backend | Maintenance | todo | none | +| T-006 | `PackageRecipe.to_dict()` was deleted in #112; `ebuild update-index` fails with `AttributeError` on every recipe it caches (`index_sync.py:354` calls it); 9 tests in `tests/unit/test_index_sync.py` fail on master, and mypy reports the call site | backend | Maintenance | todo | none | ### Evidence (self-reported by implementer; pending independent review per `.ai/reviewer.md` — "if you implemented it, you do not approve it") diff --git a/ebuild/packages/fetcher.py b/ebuild/packages/fetcher.py index f04e8f9..d80d0bb 100644 --- a/ebuild/packages/fetcher.py +++ b/ebuild/packages/fetcher.py @@ -5,19 +5,41 @@ Handles HTTP/HTTPS downloads, SHA-256 checksum verification, and tar/zip extraction into the package cache directory. + +Downloads are bounded and atomic: a socket timeout keeps a stalled mirror +from hanging the build forever, an overall size cap keeps a runaway +response from filling the disk, and the archive is streamed to a ``.part`` +file that is renamed into place only once complete, so a crashed download +never leaves a truncated archive behind to satisfy the cache. """ from __future__ import annotations import hashlib +import os import tarfile +import urllib.error +import urllib.request import zipfile from pathlib import Path from typing import Optional -from urllib.request import urlretrieve +from ebuild.packages.index_sync import is_offline from ebuild.packages.recipe import PackageRecipe +# Seconds a download may stall before it is abandoned. urlretrieve() carried +# no timeout at all, so one unresponsive mirror hung `ebuild build` until the +# user killed it. +DEFAULT_DOWNLOAD_TIMEOUT_SECONDS = 30 + +# Upper bound on a single downloaded archive (512 MB). Enforced while +# streaming, so a server that lies about (or omits) Content-Length still +# cannot fill the disk. +MAX_ARCHIVE_SIZE_BYTES = 512 * 1024 * 1024 + +# Chunk size for streaming a download to disk. +DOWNLOAD_CHUNK_SIZE_BYTES = 256 * 1024 + class FetchError(Exception): """Raised when a package cannot be fetched or verified.""" @@ -30,8 +52,13 @@ class PackageFetcher: Checksums are verified to ensure integrity. """ - def __init__(self, download_dir: str | Path) -> None: + def __init__( + self, + download_dir: str | Path, + timeout: int = DEFAULT_DOWNLOAD_TIMEOUT_SECONDS, + ) -> None: self.download_dir = Path(download_dir) + self.timeout = timeout self.download_dir.mkdir(parents=True, exist_ok=True) def fetch(self, recipe: PackageRecipe, extract_to: str | Path) -> Path: @@ -45,8 +72,21 @@ def fetch(self, recipe: PackageRecipe, extract_to: str | Path) -> Path: Path to the extracted source directory. Raises: - FetchError: If download or verification fails. + FetchError: If download or verification fails, or if offline + mode is active and the archive is not already cached. """ + # Offline mode must gate archive fetching the same way it gates index + # synchronization: a fetch that is not already in the download cache + # requires the network, and in offline mode there is no network. + if is_offline() and not self.is_downloaded(recipe): + archive_path = self._archive_path(recipe) + where = f" ({archive_path})" if archive_path else "" + raise FetchError( + f"Offline mode (EBUILD_OFFLINE=1): package {recipe.name} " + f"v{recipe.version} is not in the download cache{where}. " + f"Re-run without offline mode to download it." + ) + # PackageRecipe.validate() rejects a recipe without a checksum, but # fetch() is reachable with a hand-built recipe too, so refuse here as # well rather than falling through to an unverified extract. @@ -90,14 +130,42 @@ def _download(self, recipe: PackageRecipe) -> Path: return archive_path archive_path.parent.mkdir(parents=True, exist_ok=True) + + # Stream to a .part sibling and rename only once complete. A download + # that dies halfway must not leave a truncated file at archive_path: + # _download() short-circuits on existence, so a partial archive would + # be served by every later fetch as if it were the real thing. + partial_path = archive_path.with_name(archive_path.name + ".part") try: - urlretrieve(recipe.url, str(archive_path)) - except Exception as e: - archive_path.unlink(missing_ok=True) - raise FetchError( - f"Failed to download {recipe.name} v{recipe.version} " - f"from {recipe.url}: {e}" - ) + try: + request = urllib.request.Request( + recipe.url, + headers={"User-Agent": "ebuild-package-manager/3.0"}, + ) + with urllib.request.urlopen( + request, timeout=self.timeout + ) as response: + received = 0 + with open(partial_path, "wb") as f: + while True: + chunk = response.read(DOWNLOAD_CHUNK_SIZE_BYTES) + if not chunk: + break + received += len(chunk) + if received > MAX_ARCHIVE_SIZE_BYTES: + raise FetchError( + f"Archive from {recipe.url} exceeds the " + f"maximum size of {MAX_ARCHIVE_SIZE_BYTES} bytes" + ) + f.write(chunk) + os.replace(partial_path, archive_path) + except (urllib.error.URLError, OSError) as e: + raise FetchError( + f"Failed to download {recipe.name} v{recipe.version} " + f"from {recipe.url}: {e}" + ) from e + finally: + partial_path.unlink(missing_ok=True) return archive_path diff --git a/tests/ebuild/test_package_fetcher.py b/tests/ebuild/test_package_fetcher.py index f01c5d6..1c03f01 100644 --- a/tests/ebuild/test_package_fetcher.py +++ b/tests/ebuild/test_package_fetcher.py @@ -3,7 +3,7 @@ """Tests for ebuild.packages.fetcher.PackageFetcher. -No test here touches the network: ``urlretrieve`` is replaced with a stub +No test here touches the network: ``urlopen`` is replaced with a stub that synthesises a small tarball whose contents identify the URL it was asked for. That makes cross-package contamination observable — a package extracted from the wrong archive carries the wrong marker. @@ -99,12 +99,21 @@ def fake_download(monkeypatch): """ calls = [] - def _urlretrieve(url, filename): + class _FakeResponse(io.BytesIO): + """Duck-typed urlopen() response: BytesIO plus a context manager.""" + + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + def _urlopen(request, timeout=None): + url = request.full_url if hasattr(request, "full_url") else request calls.append(url) - with open(filename, "wb") as f: - f.write(targz_bytes(url)) + return _FakeResponse(targz_bytes(url)) - monkeypatch.setattr("ebuild.packages.fetcher.urlretrieve", _urlretrieve) + monkeypatch.setattr("ebuild.packages.fetcher.urllib.request.urlopen", _urlopen) return calls @@ -252,12 +261,19 @@ def test_fetch_extracts_into_the_requested_directory(tmp_path, fake_download): def test_unsupported_archive_format_is_rejected(tmp_path, monkeypatch): + class _Response(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + monkeypatch.setattr( - "ebuild.packages.fetcher.urlretrieve", - lambda url, filename: open(filename, "wb").write(b"not an archive"), + "ebuild.packages.fetcher.urllib.request.urlopen", + lambda request, timeout=None: _Response(b"not an archive"), ) fetcher = PackageFetcher(tmp_path / "dl") - # Checksum of the bytes the patched urlretrieve writes, so the fetch gets + # Checksum of the bytes the patched urlopen serves, so the fetch gets # past verification and reaches the format check this test is about. recipe = make_recipe( "littlefs", @@ -287,12 +303,114 @@ def test_non_http_url_schemes_are_rejected(tmp_path, url): def test_failed_download_leaves_no_partial_archive(tmp_path, monkeypatch): - def _boom(url, filename): - with open(filename, "wb") as f: - f.write(b"partial") - raise OSError("connection reset") + class _Response(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + def _boom(request, timeout=None): + return _Response(b"partial") + + monkeypatch.setattr( + "ebuild.packages.fetcher.urllib.request.urlopen", _boom + ) + monkeypatch.setattr( + "ebuild.packages.fetcher.MAX_ARCHIVE_SIZE_BYTES", 4 + ) + fetcher = PackageFetcher(tmp_path / "dl") + recipe = make_recipe("littlefs") + + with pytest.raises(FetchError, match="maximum size"): + fetcher.fetch(recipe, tmp_path / "src") + + assert fetcher.is_downloaded(recipe) is False + + +# ── Download hardening (timeout, size cap, atomicity) ──────── + + +def test_download_passes_the_configured_timeout(tmp_path, monkeypatch): + """urlretrieve() carried no timeout, so a stalled mirror hung the build. + + The timeout must actually reach urlopen — it is the whole point. + """ + seen = {} + + def _urlopen(request, timeout=None): + seen["timeout"] = timeout + return io.BytesIO(b"") + + monkeypatch.setattr( + "ebuild.packages.fetcher.urllib.request.urlopen", _urlopen + ) + fetcher = PackageFetcher(tmp_path / "dl", timeout=7) + # A mismatching checksum lets the download run to completion before the + # verification failure ends the fetch. + recipe = make_recipe("littlefs", checksum="sha256:" + "0" * 64) + with pytest.raises(FetchError): + fetcher.fetch(recipe, tmp_path / "src") + + assert seen["timeout"] == 7 + + +def test_default_fetcher_gets_the_default_timeout(tmp_path, fake_download): + """A fetcher constructed with no explicit timeout still times out.""" + from ebuild.packages.fetcher import DEFAULT_DOWNLOAD_TIMEOUT_SECONDS + + fetcher = PackageFetcher(tmp_path / "dl") + assert fetcher.timeout == DEFAULT_DOWNLOAD_TIMEOUT_SECONDS + + +def test_oversized_download_is_rejected(tmp_path, monkeypatch): + """A response larger than the cap must be refused, not written to disk.""" + class _Response(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + monkeypatch.setattr( + "ebuild.packages.fetcher.urllib.request.urlopen", + lambda request, timeout=None: _Response(b"x" * 16), + ) + monkeypatch.setattr( + "ebuild.packages.fetcher.MAX_ARCHIVE_SIZE_BYTES", 8 + ) + fetcher = PackageFetcher(tmp_path / "dl") + recipe = make_recipe("littlefs", checksum="sha256:" + "0" * 64) + + with pytest.raises(FetchError, match="maximum size"): + fetcher.fetch(recipe, tmp_path / "src") + + assert fetcher.is_downloaded(recipe) is False + # Only the (empty) package directory may remain — no archive, no .part. + assert all(p.is_dir() for p in (tmp_path / "dl").rglob("*")) + - monkeypatch.setattr("ebuild.packages.fetcher.urlretrieve", _boom) +def test_truncated_download_leaves_no_cache_entry(tmp_path, monkeypatch): + """A connection that dies mid-body must not satisfy the cache. + + _download() short-circuits on existence, so a truncated archive left at + the cache path would be extracted (or checksum-failed) forever after. + The streamed .part file must never be renamed into place on failure. + """ + class _TruncatedResponse(io.BytesIO): + def read(self, size=-1): + raise OSError("connection reset by peer") + + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + monkeypatch.setattr( + "ebuild.packages.fetcher.urllib.request.urlopen", + lambda request, timeout=None: _TruncatedResponse(b""), + ) fetcher = PackageFetcher(tmp_path / "dl") recipe = make_recipe("littlefs") @@ -300,3 +418,40 @@ def _boom(url, filename): fetcher.fetch(recipe, tmp_path / "src") assert fetcher.is_downloaded(recipe) is False + # No .part litter either. + assert not list((tmp_path / "dl").rglob("*.part")) + + +# ── Offline mode ───────────────────────────────────────────── + + +def test_offline_mode_refuses_an_uncached_package(tmp_path, monkeypatch): + """Offline mode must gate archive fetching like it gates index sync.""" + monkeypatch.setattr( + "ebuild.packages.fetcher.is_offline", lambda offline_flag=False: True + ) + fetcher = PackageFetcher(tmp_path / "dl") + recipe = make_recipe("littlefs") + + with pytest.raises(FetchError, match="Offline mode"): + fetcher.fetch(recipe, tmp_path / "src") + + # Nothing was downloaded or extracted. + assert fetcher.is_downloaded(recipe) is False + assert not (tmp_path / "src").exists() + + +def test_offline_mode_uses_the_cached_archive(tmp_path, monkeypatch, fake_download): + """A package already in the download cache stays buildable offline.""" + fetcher = PackageFetcher(tmp_path / "dl") + recipe = make_recipe("littlefs") + fetcher.fetch(recipe, tmp_path / "src-warm") + assert fake_download == [LITTLEFS_URL] + + monkeypatch.setattr( + "ebuild.packages.fetcher.is_offline", lambda offline_flag=False: True + ) + fetcher.fetch(recipe, tmp_path / "src-offline") + + assert fake_download == [LITTLEFS_URL] + assert marker_in(tmp_path / "src-offline") == LITTLEFS_URL