From 02d8bb891d2ab167ddaa139e1ef7cfadb9ea179f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 1 Jul 2026 06:54:26 +0000 Subject: [PATCH] fix: api/cli/registry bridges + high-value codebase-audit fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth sweep batch — the api/cli/registry "bridge" issues that span multiple hub files, plus the highest-value findings from the parallel codebase audit. bridges (span api/cli/registry/settings/frida_download): - Frida-less ports config no longer KeyErrors ls/status/doctor/frida-addr (#158) - ls/status reuse a single registry snapshot + memoized port index (#230) - cross-instance port-collision check moved inside the registry lock (TOCTOU) (#183) - apply() no longer clobbers a working frida-server before a re-fetch (#165) - frida-server arch auto-detects from the host (aarch64 -> android-arm64) (#189) - malformed BEETROOT_*_TIMEOUT maps to error/exit-1 instead of an import traceback that bricked even --help (#197) codebase-audit fixes: - install_frida detaches the daemon's stdio so it can't hang forever (#253) - serial_is_available/_adb_unchecked run adb with a 5s timeout (#256) - boot_cache memoizes the rootfs digest on (path,size,mtime) so warm up no longer re-hashes the multi-GB image every start (#254) - rootfs bake pins --platform=linux/amd64 (#258); verify the Docker static bundle against a pinned sha256 before extract (#262, machinery; digest TBD) - console.table markup-escapes cells/headers (#259) - module downloads validate bytes vs Content-Length (#261) - snapshot manifest written first + restore decompression-bomb cap (#265) - e2e savevm cache stores the cache-key sidecar (#260); denylist trims whitespace (#263); docs main->master links (#266) All covered by new tests; full gate green (ruff, mypy --strict src+tests, pytest 100% line+branch (1960 passed), shellcheck/shfmt, yamllint, actionlint, zizmor, codespell, deptry, uv lock, changelog lint). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01P1YsNMpGizhSDDPCBu5cut --- .github/workflows/e2e.yml | 8 +- CHANGELOG.md | 17 +++ docker/magisk-config.sh | 31 ++++- docs/getting-started/index.md | 2 +- docs/guides/adding-a-backend.md | 2 +- docs/guides/examples.md | 4 +- docs/reference/cli.md | 2 +- scripts/vm_cache_key.py | 32 ++++- src/beetroot/api.py | 60 ++++++--- src/beetroot/backends/adb.py | 106 ++++++++++++--- src/beetroot/builder.py | 82 +++++++++++- src/beetroot/cli.py | 133 +++++++++---------- src/beetroot/console.py | 11 +- src/beetroot/frida_download.py | 106 +++++++++++++-- src/beetroot/modules_download.py | 19 ++- src/beetroot/registry.py | 47 ++++++- src/beetroot/settings.py | 73 +++++++++- src/beetroot/snapshot.py | 31 ++++- src/beetroot/vm/boot_cache.py | 22 ++- tests/test_adb_device.py | 147 ++++++++++++++++++++- tests/test_bugfix_frida_arch.py | 110 +++++++++++++++ tests/test_bugfix_frida_less_ports.py | 107 +++++++++++++++ tests/test_bugfix_frida_placeholder.py | 135 +++++++++++++++++++ tests/test_bugfix_ls_snapshot_reuse.py | 112 ++++++++++++++++ tests/test_bugfix_port_collision_toctou.py | 118 +++++++++++++++++ tests/test_bugfix_settings_lazy_error.py | 95 +++++++++++++ tests/test_builder.py | 78 ++++++++++- tests/test_cli_error_contract.py | 8 +- tests/test_console.py | 30 +++++ tests/test_magisk_config_helper.py | 22 +++ tests/test_modules_download.py | 26 ++++ tests/test_snapshot.py | 104 +++++++++++++++ tests/test_vm_boot_cache.py | 79 +++++++++++ tests/test_vm_cache_key.py | 44 ++++++ 34 files changed, 1850 insertions(+), 153 deletions(-) create mode 100644 tests/test_bugfix_frida_arch.py create mode 100644 tests/test_bugfix_frida_less_ports.py create mode 100644 tests/test_bugfix_frida_placeholder.py create mode 100644 tests/test_bugfix_ls_snapshot_reuse.py create mode 100644 tests/test_bugfix_port_collision_toctou.py create mode 100644 tests/test_bugfix_settings_lazy_error.py diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index afafdbb..590098c 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -443,7 +443,13 @@ jobs: - name: Restore/persist the booted savevm overlay uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 # v4.2.4 with: - path: vmphone/vm-overlay.qcow2 + # Both the overlay AND its cache-key sidecar must be restored: the + # CLI's warm resume treats a missing sidecar as a stale cache and + # cold-boots instead, so caching the overlay alone leaves it dead + # (issue #260). + path: | + vmphone/vm-overlay.qcow2 + vmphone/vm-overlay.cache-key key: ${{ runner.os }}-vm-savevm-${{ steps.vmkey.outputs.key }} - name: "Boot the binder:vm instance under QEMU/TCG" diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a497f5..8f1a747 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Security +- **The VM rootfs bake verifies the Docker static bundle against a pinned sha256 before extracting it into the guest (#262).** The verify-before-extract machinery mirrors the guest-kernel check (#184); an unpinned or `DOCKER_URL`-overridden bundle prints an explicit unverified-source warning. (The default bundle's literal digest is pinned as a follow-up.) - **`rootfs-release.yml` installs `uv` via the SHA-pinned `astral-sh/setup-uv` action (#209).** This closes the last bypass of the single pinned `uv` path in the `contents: write` release workflow (it used an unpinned `pipx install uv`). - **The guest-kernel source tarball is verified against a pinned sha256 before compiling (#184).** The `binder: vm` kernel build (and the release/CI lanes) now hash the downloaded `cdn.kernel.org` tarball and fold the digest into the published prebuilt fingerprint, so a tampered tarball can't be compiled into a trusted `bzImage`. - **`frida-server` downloads decompress incrementally with a bounded output ceiling and a truncation check (#228).** A corrupt or zip-bomb `.xz` raises `FridaFetchError` past the ceiling instead of OOM-ing the host; the per-call output is now capped (so a single chunk can't expand without bound), and a stream that ends before its LZMA end-of-stream marker (a truncated download) is rejected instead of silently accepted as a complete binary. @@ -100,6 +101,7 @@ ### Features +- **`frida-server` architecture auto-detects from the host (#189).** A redroid `host`/`auto` instance on aarch64 now stages `android-arm64` instead of the x86_64 default; `binder: vm` stays pinned to x86_64 and `BEETROOT_FRIDA_ARCH` still overrides. - **New `beetroot install-frida --version ` verb (#205).** Pushes and launches `frida-server` on an adb-adopted device, wiring the previously API-only `AdbDevice.install_frida()` into the CLI that the `adopt` hint already advertised. - **Set-but-inert fields under `binder: vm` now surface once at config time, not on every boot (#104).** The advisory that names `beetroot.yaml` settings the plain-redroid VM can't @@ -504,6 +506,10 @@ pre-abort rows in its `results` attribute). ### Quality & internals +- **`beetroot ls`/`status` reuse a single registry snapshot and memoize each instance's port index (#230).** Cuts the redundant `instances.json` reads and double `beetroot.yaml` parses per command. +- **`binder: vm` boot_cache no longer re-streams a full SHA-256 over the multi-GB rootfs on every warm `up` (#254).** Each input's digest is memoized on `(path, size, mtime)`, so an unchanged rootfs is not re-hashed — restoring the warm-start speedup — while a size/mtime change still forces a re-hash. `scripts/vm_cache_key.py` stays in parity. +- **`beetroot restore` reads the snapshot manifest without decompressing the whole archive twice (#265).** The manifest is written as the first archive member so `read_manifest` early-exits (old manifest-last archives still restore); restore also caps total decompressed bytes to reject a `.tar.zst` decompression bomb. +- **Fixed docs GitHub links that used `/blob/main` and `/tree/main` (#266).** The default branch is `master`; the `main` links 404'd. - **CI `apt-get install` steps now run `apt-get update` first (#211).** A stale hosted-runner apt index no longer 404s and fails the install step (e2e, benchmark, beetroot-ci). - **Migration docs refreshed to `api_version` 8 (#213).** Added the 6→7 (gapps intent/vendor split) and 7→8 (ports list) walkthroughs and dropped the stale "set `api_version: 6`" instructions, with a test guarding the documented version against `SUPPORTED_API_VERSION`. - **`architecture.md` compose env-file contract corrected (#214).** Removed the retired `ADB_PORT`/`FRIDA_PORT`/`FRIDA_PORT_CONTROL` tokens and pointed at the per-instance `compose.override.yaml` ports overlay. @@ -624,6 +630,17 @@ are absent), so shell regressions are caught locally before the push. ### Bug fixes +- **Fixed a `KeyError: 'frida'` crash on a Frida-less `ports:` config (#158).** It took down `ls`/`status`/`doctor`/`frida-addr` (and the whole-fleet `ls`); `frida_address` now returns the `unsupported` sentinel like the vm backend. +- **The cross-instance host-port collision check runs inside the exclusive registry lock (#183).** Two concurrent `apply`/`create` operations pinning the same `host:` port can no longer both pass and double-bind at `up`. +- **`beetroot apply` no longer wipes a working `frida-server` to an empty placeholder before re-downloading (#165).** A failed cache-miss re-fetch leaves the prior binary intact (staged via temp + atomic replace). +- **A malformed `BEETROOT_*_TIMEOUT` maps to a friendly `error: …` + exit 1 instead of a raw pydantic traceback at import (#197).** The bad var used to brick even `beetroot --help`; `Settings` is now built lazily inside `cli.main()`'s error boundary. +- **`AdbDevice.install_frida` no longer hangs forever launching `frida-server` (#253).** The daemon's stdio is detached off adb's captured pipes (`/dev/null 2>&1 &`), and daemon-launch commands carry a timeout. +- **`serial_is_available` / `_adb_unchecked` run adb with a 5s timeout (#256).** A wedged adb-server can no longer hang `ls`/`status`/`doctor` or the auto-install re-probe; a timeout is treated as unavailable/failed. +- **The local rootfs bake pins `--platform=linux/amd64` on the redroid pull (#258).** A non-x86_64 build host no longer bakes a wrong-arch image into the hard-x86_64 guest. +- **`console.table()` markup-escapes cell and header values (#259).** An instance path containing `[brackets]` is no longer silently corrupted (or crashed on) by rich's tag parser. +- **Module downloads validate bytes-received against `Content-Length` (#261).** A clean short read no longer caches a truncated module zip. +- **The `magisk-config.sh` denylist loop trims surrounding whitespace on each package (#263).** A space-padded entry (`com.foo, com.bar`) is enrolled as the real package instead of `' com.bar'`, so root-hiding actually applies. +- **The e2e `tier-vm-qemu` savevm cache now stores the `vm-overlay.cache-key` sidecar alongside the overlay (#260).** Without it the warm resume was always judged stale, so the boot-cache delivered no speedup in CI. - **`beetroot up` self-heal now runs the cross-instance port-collision precheck, and `restart` self-heals a missing compose override (#166).** Neither boots with a colliding or zero-port mapping anymore. - **`beetroot apply` flips the registry backend kind before staging Frida/modules (#182).** A transient fetch failure no longer leaves a `binder: vm` config dispatching the redroid backend. - **`beetroot doctor` against an adopted adb device no longer reports a phantom `fail` for the GMS Magisk-denylist check (#201).** The row is `skip` since adb devices carry no Beetroot-managed denylist. diff --git a/docker/magisk-config.sh b/docker/magisk-config.sh index 6d26b7a..ec69c71 100755 --- a/docker/magisk-config.sh +++ b/docker/magisk-config.sh @@ -98,11 +98,34 @@ if [ -n "$DENYLIST_PACKAGES" ]; then # shellcheck disable=SC2086 # word-splitting on IFS=, is the point. set -- $DENYLIST_PACKAGES IFS="$OLD_IFS" + # Whitespace to strip from each field: a literal space and a literal + # tab. Built via printf so no raw tab has to live in the source (which + # would trip shfmt) — toybox printf honours \t. + trim_ws="$(printf ' \t')" for pkg in "$@"; do - # Skip empty fields produced by trailing / leading commas. - # The pydantic regex in T1 already rejected empty entries at - # config-load time, but defend at the boundary too in case a - # hand-crafted .env arrives via the raw compose escape hatch. + # Trim leading / trailing space and tab so a spaced CSV like + # ``com.foo, com.bar`` enrols ``com.bar`` and not `` com.bar`` — a + # leading space never matches the real package, silently defeating + # the denylist (issue #263). Toybox sh has no ``${var//}`` or a + # coreutils ``xargs``, so strip one char at a time with POSIX + # ``case`` + parameter expansion. + while :; do + case "$pkg" in + [$trim_ws]*) pkg="${pkg#?}" ;; + *) break ;; + esac + done + while :; do + case "$pkg" in + *[$trim_ws]) pkg="${pkg%?}" ;; + *) break ;; + esac + done + # Skip empty fields produced by trailing / leading commas, or a + # field that was pure whitespace and trimmed away to nothing. The + # pydantic regex in T1 already rejected empty entries at config-load + # time, but defend at the boundary too in case a hand-crafted .env + # arrives via the raw compose escape hatch. if [ -z "$pkg" ]; then continue fi diff --git a/docs/getting-started/index.md b/docs/getting-started/index.md index cc8614b..b96e200 100644 --- a/docs/getting-started/index.md +++ b/docs/getting-started/index.md @@ -5,7 +5,7 @@ This section walks you through getting Beetroot running on your host machine — ## Steps 1. **[Prerequisites](prerequisites.md)** — Linux host, Docker, kernel modules, `uv`, ADB. -2. **[Installation](installation.md)** — `uv tool install git+https://github.com/Xiddoc/Beetroot.git`, then `beetroot build` to build the base image. (If you're hacking on Beetroot itself, see [CLAUDE.md](https://github.com/Xiddoc/Beetroot/blob/main/CLAUDE.md) for the editable `uv sync` workflow.) +2. **[Installation](installation.md)** — `uv tool install git+https://github.com/Xiddoc/Beetroot.git`, then `beetroot build` to build the base image. (If you're hacking on Beetroot itself, see [CLAUDE.md](https://github.com/Xiddoc/Beetroot/blob/master/CLAUDE.md) for the editable `uv sync` workflow.) 3. **[Your First Instance](first-instance.md)** — `create` → `up` → `shell` → `down` end-to-end. If you've already done the one-time setup and want to spin up a second environment, jump straight to the [Guides](../guides/index.md) section. diff --git a/docs/guides/adding-a-backend.md b/docs/guides/adding-a-backend.md index 6a81f97..2afe19b 100644 --- a/docs/guides/adding-a-backend.md +++ b/docs/guides/adding-a-backend.md @@ -366,7 +366,7 @@ rather than a silent drop. ## 7. Test your backend The pattern is laid out in -[`tests/test_backend_extension.py`](https://github.com/Xiddoc/Beetroot/blob/main/tests/test_backend_extension.py) — +[`tests/test_backend_extension.py`](https://github.com/Xiddoc/Beetroot/blob/master/tests/test_backend_extension.py) — the synthetic third-backend test that grades the entire recipe at every CI run. Extend the pattern in your own package: diff --git a/docs/guides/examples.md b/docs/guides/examples.md index cab02e9..bd42d4e 100644 --- a/docs/guides/examples.md +++ b/docs/guides/examples.md @@ -1,6 +1,6 @@ # Examples -Beetroot ships a handful of starter `beetroot.yaml` files under the [`examples/`](https://github.com/Xiddoc/Beetroot/tree/main/examples) directory of the repository. They are **documentation only** — the CLI does not load or reference them. Each file is a hand-readable, copy-pasteable snippet you drop over a fresh `beetroot.yaml` when you want that configuration as your starting point. +Beetroot ships a handful of starter `beetroot.yaml` files under the [`examples/`](https://github.com/Xiddoc/Beetroot/tree/master/examples) directory of the repository. They are **documentation only** — the CLI does not load or reference them. Each file is a hand-readable, copy-pasteable snippet you drop over a fresh `beetroot.yaml` when you want that configuration as your starting point. `beetroot create ` always writes a minimal `beetroot.yaml`: @@ -160,7 +160,7 @@ beetroot apply research-clean beetroot up research-clean ``` -The `examples/` directory is a sibling of `docs/` in the [Beetroot repo](https://github.com/Xiddoc/Beetroot/tree/main/examples). If you installed via `uv tool install` and don't have a checkout handy, copy the YAML from this page directly into your instance's `beetroot.yaml`. +The `examples/` directory is a sibling of `docs/` in the [Beetroot repo](https://github.com/Xiddoc/Beetroot/tree/master/examples). If you installed via `uv tool install` and don't have a checkout handy, copy the YAML from this page directly into your instance's `beetroot.yaml`. ## Modifying your config diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 0fd4074..ff4018e 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -5,7 +5,7 @@ CLI is built on [Typer](https://typer.tiangolo.com/), so `--help` renders as boxed sections with color (via Rich); flag and argument tables in this reference mirror the same shape. -After `uv tool install`, invocations are plain `beetroot ` — the tool venv puts `beetroot` directly on your `PATH`. (Contributors hacking on Beetroot from an editable `uv sync` checkout use `uv run beetroot ` instead; see [CLAUDE.md](https://github.com/Xiddoc/Beetroot/blob/main/CLAUDE.md).) +After `uv tool install`, invocations are plain `beetroot ` — the tool venv puts `beetroot` directly on your `PATH`. (Contributors hacking on Beetroot from an editable `uv sync` checkout use `uv run beetroot ` instead; see [CLAUDE.md](https://github.com/Xiddoc/Beetroot/blob/master/CLAUDE.md).) Beetroot's path model is Docker-inspired: an instance is any directory on disk containing a `beetroot.yaml`. The CLI discovers the current instance by walking up from `cwd` like `git` walks up to find `.git`. The cross-instance registry — name → absolute path — lives at `~/.config/beetroot/instances.json` (respects `XDG_CONFIG_HOME`). diff --git a/scripts/vm_cache_key.py b/scripts/vm_cache_key.py index 94366f7..4bf76a7 100755 --- a/scripts/vm_cache_key.py +++ b/scripts/vm_cache_key.py @@ -38,11 +38,30 @@ # Streaming read size for hashing the (multi-GB) rootfs without loading it all. _CHUNK = 1024 * 1024 +# Memoized {(path, st_size, st_mtime_ns): sha256-hexdigest}. A multi-GB rootfs is +# immutable between builds, so re-streaming a full SHA-256 on every call is pure +# waste; keying on (path, size, mtime_ns) reuses the digest whenever the file is +# byte-identical and recomputes only when size/mtime say it changed. This keeps +# `hash_file` in parity with ``beetroot.vm.boot_cache._hash_file`` (issue #254). +_HASH_CACHE: dict[tuple[str, int, int], str] = {} + + +def _stream_sha256(path: Path) -> str: + digest = hashlib.sha256() + with path.open("rb") as fh: + while chunk := fh.read(_CHUNK): + digest.update(chunk) + return digest.hexdigest() + def hash_file(path: Path) -> str: """ Return the streamed SHA-256 of a file's contents. + Memoized on ``(path, st_size, st_mtime_ns)`` so an unchanged file is hashed + at most once; a size/mtime change forces a re-stream. The returned digest is + identical to hashing the bytes directly — the cache only skips redundant work. + Args: path: The file to hash. Read in chunks so a multi-GB rootfs image is never loaded into memory at once. @@ -53,11 +72,14 @@ def hash_file(path: Path) -> str: Raises: FileNotFoundError: If ``path`` does not exist. """ - digest = hashlib.sha256() - with path.open("rb") as fh: - while chunk := fh.read(_CHUNK): - digest.update(chunk) - return digest.hexdigest() + stat = path.stat() + key = (str(path), stat.st_size, stat.st_mtime_ns) + cached = _HASH_CACHE.get(key) + if cached is not None: + return cached + hexdigest = _stream_sha256(path) + _HASH_CACHE[key] = hexdigest + return hexdigest def compute_cache_key(paths: list[Path], *, prefix: str = DEFAULT_PREFIX) -> str: diff --git a/src/beetroot/api.py b/src/beetroot/api.py index 56bbc6b..11d875c 100644 --- a/src/beetroot/api.py +++ b/src/beetroot/api.py @@ -36,6 +36,7 @@ import contextlib import fcntl +import functools import re import shlex import shutil @@ -893,10 +894,18 @@ def config(self) -> config.InstanceConfig: """ return self._cfg - @property + @functools.cached_property def index(self) -> int: """ The instance's allocated port index (stride-of-10 base). + + Memoized: ``ports`` / ``adb_address`` / ``frida_address`` all key off + this, so caching collapses their repeated ``registry.get`` lookups into + a single read per ``Instance`` — cutting the redundant ``instances.json`` + reads a whole-fleet ``ls`` / ``status`` incurred (#230). The + disappearance contract is preserved: :meth:`_meta` still raises + :class:`InstanceNotFoundError` on the first access if the row is gone, + and a raised exception is never cached. """ return self._meta().index @@ -922,8 +931,16 @@ def adb_address(self) -> str: def frida_address(self) -> str: """ ``localhost:`` — what ``frida -H`` should target. + + A valid Frida-less ``ports:`` config (adb only, no ``frida`` service) + has no ``frida`` key in :func:`ports.well_known`, so return the + :data:`FRIDA_ADDRESS_UNSUPPORTED` sentinel rather than crashing the + whole-fleet ``ls`` with a ``KeyError`` — mirroring the vm backend (#158). """ - return f"localhost:{ports.well_known(self.ports)['frida']}" + wk = ports.well_known(self.ports) + if "frida" not in wk: + return FRIDA_ADDRESS_UNSUPPORTED + return f"localhost:{wk['frida']}" @property def status(self) -> compose.ComposeStatus: @@ -1214,7 +1231,7 @@ def install_frida(self, version: str | None = None) -> None: "pass a version explicitly (e.g. install_frida('16.4.10'))" ) version = self._cfg.frida.version - frida_download.stage_for_instance(self._root, version) + frida_download.stage_for_instance(self._root, version, binder=self._cfg.binder) def frida_cli(self, args: Sequence[str]) -> int: """ @@ -1368,12 +1385,16 @@ def _stage_local(self) -> None: paths.instance_compose_override(self._root).write_text( config.render_compose_ports_override(new_ports) ) - # Always place the placeholder, even when frida is configured. - # ``_stage_network`` overwrites it with the real binary on - # success; on a network failure the placeholder survives so - # the bind-mount target exists and the compose ``up`` doesn't - # fail at mount-resolution time. - frida_download.stage_empty(self._root) + # Place the empty placeholder only when there's no usable real binary + # to preserve. Previously this ran unconditionally, zeroing a working + # frida-server *before* ``_stage_network`` re-downloaded it — so a + # failed cache-miss re-fetch left Frida disabled (#165). When frida is + # configured AND a non-empty binary already exists, leave it intact and + # let ``_stage_network`` atomically swap in the fresh one on success. + frida_path = paths.instance_frida(self._root) + has_real_binary = frida_path.exists() and frida_path.stat().st_size > 0 + if self._cfg.frida is None or not has_real_binary: + frida_download.stage_empty(self._root) def _stage_network(self) -> None: """ @@ -1390,6 +1411,7 @@ def _stage_network(self) -> None: self._root, self._cfg.frida.version, expected_sha256=self._cfg.frida.sha256, + binder=self._cfg.binder, ) modules_download.stage_for_instance(self._root, self._cfg) @@ -1421,7 +1443,10 @@ def health(self) -> dict[str, CheckResult]: """ wk = ports.well_known(self.ports) adb_port = wk["adb"] - frida_port = wk["frida"] + # A valid Frida-less ``ports:`` config has no ``frida`` key; a 0 port is + # fine because the frida check below is gated on ``self._cfg.frida is not + # None`` and renders a disabled/skip row rather than probing (#158). + frida_port = wk.get("frida", 0) checks: dict[str, CheckResult] = {} # compose.status: pass iff the container is running. The compose # Literal vocabulary is closed (see compose.ComposeStatus); any @@ -1552,16 +1577,13 @@ def _check_port_collisions(name: str, new_ports: list[ports.ResolvedPort]) -> No The CLI wraps this in its own friendly-error formatter; the OOP surface raises a plain ``ValueError`` so programmatic callers can catch a stdlib exception. + + Delegates to :func:`registry.assert_no_port_collision`, which performs the + sibling read + collision decision inside the exclusive registry lock so two + concurrent ``apply``/``create`` operations pinning the same ``host:`` port + can't both slip past an unlocked precheck and double-bind at ``up`` (#183). """ - others = {n: p for n, p in registry.all_resolved_host_ports().items() if n != name} - collision = registry.find_port_collision(new_ports, others) - if collision is None: - return - port, other_name, kind = collision - raise ValueError( - f"port {port} ({kind}) collides with instance {other_name!r} " - f"(which also uses {port}). Pin or remove one." - ) + registry.assert_no_port_collision(name, new_ports) class Manager: diff --git a/src/beetroot/backends/adb.py b/src/beetroot/backends/adb.py index 37cce97..0a2aa37 100644 --- a/src/beetroot/backends/adb.py +++ b/src/beetroot/backends/adb.py @@ -53,6 +53,38 @@ _MAGISK_MODULE_DROP = "/sdcard/Download" _MAGISK_MODULE_TMP = "/data/local/tmp" +# Wall-clock cap (seconds) on every capturing ``adb`` invocation so a +# wedged device or a stuck adb server can never hang ``ls`` / ``status`` +# / ``doctor`` indefinitely. Mirrors ``api._check_adb_serial_listed``'s +# ``timeout=5``; a raised ``subprocess.TimeoutExpired`` is treated as the +# command having failed / the device being unavailable, never propagated. +_ADB_TIMEOUT_SECONDS = 5 + + +def _as_text(value: str | bytes | None) -> str: + """ + Coerce captured subprocess output to ``str`` (``None`` → empty). + + ``subprocess.run(text=True)`` yields ``str``, but the typeshed stub + for :class:`subprocess.TimeoutExpired` exposes ``stdout``/``stderr`` + as ``bytes | None``. This normaliser keeps the timeout-fallback + :class:`~subprocess.CompletedProcess` fields ``str``-typed for mypy + and at runtime. + + Args: + value: The captured stream, possibly ``str``, ``bytes``, or + ``None``. + + Returns: + The value as ``str`` — decoded (``errors="replace"``) if it was + ``bytes``, or ``""`` if it was ``None``. + """ + if value is None: + return "" + if isinstance(value, bytes): + return value.decode(errors="replace") + return value + def _validated_zip_source(source: str) -> Path: src = Path(source) @@ -108,12 +140,19 @@ def serial_is_available(serial: str) -> bool: if shutil.which(_ADB) is None: return False - res = subprocess.run( # noqa: S603 # adb is a host CLI on PATH; argv is constant - [_ADB, "devices"], - check=False, - capture_output=True, - text=True, - ) + try: + res = subprocess.run( # noqa: S603 # adb is a host CLI on PATH; argv is constant + [_ADB, "devices"], + check=False, + capture_output=True, + text=True, + timeout=_ADB_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + # A hung adb server would otherwise wedge ``ls`` / ``status`` + # forever; a device that can't answer within the cap is, for + # availability purposes, not reachable. + return False if res.returncode != 0: return False for line in res.stdout.splitlines(): @@ -258,9 +297,12 @@ def install_frida(self, version: str | None = None) -> None: cache (idempotent — re-runs hit the cached binary). 2. ``adb push`` the cached binary to ``/data/local/tmp/frida-server``. 3. ``adb shell chmod 755`` so the binary is executable. - 4. ``adb shell su -c '/data/local/tmp/frida-server &'`` to - background the daemon. Requires the device to be rooted - (Magisk / KernelSU / SuperSU all work). + 4. ``adb shell su -c '/data/local/tmp/frida-server /dev/null 2>&1 &'`` to background the daemon with its stdio + detached off the captured adb pipe (so the launch call + returns instead of blocking on the daemon's inherited fds). + Requires the device to be rooted (Magisk / KernelSU / SuperSU + all work). 5. ``adb forward tcp: tcp:27042`` so ``frida -H localhost:`` reaches the device's Frida socket. @@ -292,7 +334,17 @@ def install_frida(self, version: str | None = None) -> None: # caller's responsibility because adb shell strips the outer # quotes; we pass the command as a single argv element so the # ``&`` reaches the on-device shell, not the host shell. - self._adb_shell(["su", "-c", f"{_REMOTE_FRIDA_SERVER} &"]) + # + # The daemon's stdio MUST be detached off the captured adb pipe + # (``/dev/null 2>&1``): otherwise the long-lived + # frida-server keeps the inherited stdout/stderr fds open and + # ``subprocess.run`` (capture_output=True) blocks reading them + # for the daemon's whole lifetime, hanging ``install_frida`` + # forever. Redirecting stdio lets the pipe close as soon as the + # foreground ``su`` returns, so the launch call returns promptly. + self._adb_shell( + ["su", "-c", f"{_REMOTE_FRIDA_SERVER} /dev/null 2>&1 &"], + ) self._adb( "forward", f"tcp:{self._host_forward_port}", @@ -631,14 +683,36 @@ def health(self) -> dict[str, CheckResult]: def _adb_unchecked(self, *argv: str) -> subprocess.CompletedProcess[str]: """ Run ``adb -s `` with capture; never raise on non-zero. + + A wedged device or stuck adb server is capped at + ``_ADB_TIMEOUT_SECONDS`` and reported as a non-zero + :class:`subprocess.CompletedProcess` (returncode 124, the + conventional timeout code) rather than left to hang the caller — + so ``status`` / ``doctor`` (and the health checks built on + ``_adb``) fail cleanly instead of blocking forever. """ full = [_ADB, "-s", self._config.serial, *argv] - return subprocess.run( # noqa: S603 # adb is a host CLI on PATH; argv built from validated config + caller-pinned strings - full, - check=False, - capture_output=True, - text=True, - ) + try: + return subprocess.run( # noqa: S603 # adb is a host CLI on PATH; argv built from validated config + caller-pinned strings + full, + check=False, + capture_output=True, + text=True, + timeout=_ADB_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired as e: + # ``text=True`` yields str output at runtime, but typeshed + # types ``TimeoutExpired.stdout``/``.stderr`` as ``bytes | + # None``; normalise defensively so the synthesized row is + # always ``str`` regardless of what the killed process wrote. + stdout = _as_text(e.stdout) + stderr = _as_text(e.stderr) + return subprocess.CompletedProcess( + args=full, + returncode=124, + stdout=stdout, + stderr=stderr or f"adb command {full!r} timed out after {e.timeout}s", + ) def _adb(self, *argv: str) -> subprocess.CompletedProcess[str]: """ diff --git a/src/beetroot/builder.py b/src/beetroot/builder.py index 49bc467..6c21adf 100644 --- a/src/beetroot/builder.py +++ b/src/beetroot/builder.py @@ -474,6 +474,31 @@ def build_image( # noqa: PLR0913 # 7 keyword-only params; each is a distinct i _DEFAULT_DOCKER_VERSION: Final[str] = "27.5.1" +# sha256 of the pinned ``docker-<_DEFAULT_DOCKER_VERSION>.tgz`` static bundle, +# verified before it is tar-xzf'd into the trusted guest rootfs so a +# tampered/MITM'd download.docker.com CDN tarball can't ship a backdoored +# ``dockerd`` into the VM (issue #262, mirroring the kernel-source pin of #184). +# ``None`` means the literal digest is not yet pinned: verification machinery is +# in place but the default bundle is treated as an unverified source (a warning +# is printed) until the real digest is filled in. Bump in lockstep with +# ``_DEFAULT_DOCKER_VERSION``. +_DEFAULT_DOCKER_BUNDLE_SHA256: Final[str | None] = None + + +def _default_docker_url(version: str) -> str: + """ + Return the canonical download.docker.com static-bundle URL for ``version``. + + Args: + version: The Docker static-bundle version (e.g. ``27.5.1``). + + Returns: + The ``https://download.docker.com/.../docker-.tgz`` URL — the + default :attr:`_RootfsConfig.docker_url` when ``DOCKER_URL`` is unset. + """ + return f"https://download.docker.com/linux/static/stable/x86_64/docker-{version}.tgz" + + # The default Android version the micro-VM bakes — the SAME single-source-of- # truth constant the redroid base-image build and the ``beetroot create`` # config default read (issue #82). The plain upstream redroid image is derived @@ -742,6 +767,7 @@ class _RootfsConfig(BaseModel): image_size_mb: int = 8192 docker_version: str = _DEFAULT_DOCKER_VERSION docker_url: str + docker_url_sha256: str | None = None redroid_image: str = _DEFAULT_REDROID_IMAGE redroid_tar: Path | None = None adbprobe_bin: Path | None = None @@ -765,7 +791,9 @@ def from_env( ``DOCKER_URL``, ``REDROID_IMAGE``, ``REDROID_TAR``, ``ADBPROBE_BIN`` and ``BUSYBOX_BIN``; the port keeps the same names so existing build recipes keep working. The host docker binary comes from Beetroot's own - :data:`settings.docker_bin` rather than a bespoke ``DOCKER_BIN``. + :data:`settings.docker_bin` rather than a bespoke ``DOCKER_BIN``. A new + ``DOCKER_SHA256`` knob (issue #262) pins the expected digest of an + overridden bundle so a custom ``DOCKER_URL`` can still be verified. The redroid image is **derived from ``android_version``** (issue #82) via :func:`config.vm_redroid_image` so a default ``beetroot create`` @@ -783,10 +811,16 @@ def from_env( The resolved :class:`_RootfsConfig`. """ version = os.environ.get("DOCKER_VERSION", _DEFAULT_DOCKER_VERSION) - url = os.environ.get( - "DOCKER_URL", - f"https://download.docker.com/linux/static/stable/x86_64/docker-{version}.tgz", - ) + url = os.environ.get("DOCKER_URL", _default_docker_url(version)) + # Resolve the digest to verify the bundle against before extraction + # (issue #262): an explicit ``DOCKER_SHA256`` always wins; otherwise only + # the pinned default URL carries the trusted digest. A ``DOCKER_URL`` / + # ``DOCKER_VERSION`` override away from that default leaves it ``None`` + # (unverified — ``_fetch_static_bundle`` then warns unless the caller + # supplies ``DOCKER_SHA256``). + sha256 = os.environ.get("DOCKER_SHA256") or None + if sha256 is None and url == _default_docker_url(_DEFAULT_DOCKER_VERSION): + sha256 = _DEFAULT_DOCKER_BUNDLE_SHA256 redroid_tar = os.environ.get("REDROID_TAR") or None adbprobe_bin = os.environ.get("ADBPROBE_BIN") or None return cls( @@ -796,6 +830,7 @@ def from_env( image_size_mb=_parse_image_size_mb(os.environ.get("IMAGE_SIZE_MB")), docker_version=version, docker_url=url, + docker_url_sha256=sha256, redroid_image=os.environ.get("REDROID_IMAGE", config.vm_redroid_image(android_version)), redroid_tar=Path(redroid_tar) if redroid_tar is not None else None, adbprobe_bin=Path(adbprobe_bin) if adbprobe_bin is not None else None, @@ -911,9 +946,38 @@ def _write_version_marker(self) -> None: def _fetch_static_bundle(self) -> None: console.info(f"fetching Docker static bundle {self.cfg.docker_version}") self.runner.run(["curl", "-fsSL", self.cfg.docker_url, "-o", str(self.tgz)]) + # Verify the bundle against its pinned sha256 BEFORE unpacking it into + # the trusted guest rootfs, so a tampered/MITM'd CDN download can't ship + # a backdoored dockerd (issue #262). + self._verify_static_bundle_digest() self.docker_extract.mkdir(parents=True, exist_ok=True) self.runner.run(["tar", "-xzf", str(self.tgz), "-C", str(self.docker_extract)]) + def _verify_static_bundle_digest(self) -> None: + """ + Verify the downloaded Docker bundle against its pinned sha256, or warn. + + Raises: + BootstrapError: If a digest is pinned (default or ``DOCKER_SHA256``) + but the downloaded bytes don't match it — a tampered bundle. + """ + expected = self.cfg.docker_url_sha256 + if expected is None: + # No pinned digest: either the literal default digest isn't filled + # in yet, or a DOCKER_URL/DOCKER_VERSION override supplied no + # DOCKER_SHA256. Extract anyway but make the trust gap explicit. + console.warn( + f"Docker static bundle {self.cfg.docker_url} is UNVERIFIED " + "(no pinned sha256); set DOCKER_SHA256 to verify it (issue #262)" + ) + return + actual = hashlib.sha256(self.tgz.read_bytes()).hexdigest() + if actual != expected: + raise BootstrapError( + f"Docker static bundle sha256 mismatch for {self.cfg.docker_url}: " + f"expected {expected}, got {actual}; refusing to bake a tampered dockerd" + ) + def _build_tree(self) -> None: console.info(f"assembling rootfs tree in {self.root}") for rel in _ROOTFS_DIRS: @@ -1006,7 +1070,13 @@ def _stage_docker_root(self) -> Path: tar = self.cfg.redroid_tar if tar is None: console.info(f"pulling {self.cfg.redroid_image} and saving to a tarball") - self.runner.run([settings.docker_bin, "pull", self.cfg.redroid_image]) + # Pin --platform=linux/amd64: the guest rootfs is hard-x86_64, so on + # an arm64 (or other non-x86_64) build host an unpinned pull would + # resolve the multi-arch redroid tag to the host's native arch and + # bake a wrong-arch image the guest kernel can't run (issue #258). + self.runner.run( + [settings.docker_bin, "pull", "--platform=linux/amd64", self.cfg.redroid_image] + ) tar = self.work / "redroid.tar" self.runner.run([settings.docker_bin, "save", self.cfg.redroid_image, "-o", str(tar)]) diff --git a/src/beetroot/cli.py b/src/beetroot/cli.py index 589be0e..6ac3a2e 100644 --- a/src/beetroot/cli.py +++ b/src/beetroot/cli.py @@ -40,6 +40,7 @@ paths, ports, registry, + settings, ) from . import snapshot as snapshot_mod from .backends import adb as adb_backend @@ -984,15 +985,25 @@ def _ls_table_row( if isinstance(backend, api.Instance): status = str(backend.status) path = str(backend.root) + # Resolve the port mapping once and derive both addresses from a single + # ``well_known`` dict instead of calling ``adb_address`` / + # ``frida_address`` (each re-resolving ``backend.ports``) — #230. The + # frida column degrades to the ``unsupported`` sentinel for a valid + # Frida-less config rather than crashing the whole table (#158). + wk = ports.well_known(backend.ports) + adb_addr = f"localhost:{wk['adb']}" + frida_addr = f"localhost:{wk['frida']}" if "frida" in wk else api.FRIDA_ADDRESS_UNSUPPORTED else: status = "available" if backend.is_available else "unavailable" path = "-" + adb_addr = backend.adb_address + frida_addr = backend.frida_address return [ name, backend.kind, str(meta.index), - backend.adb_address, - backend.frida_address, + adb_addr, + frida_addr, status, path, ] @@ -1026,6 +1037,13 @@ def _instance_json_row(inst: api.Instance) -> dict[str, object]: """ resolved = inst.ports wk = ports.well_known(resolved) + # Derive both well-known addresses from the single ``wk`` dict computed + # here instead of re-reading them through ``inst.adb_address`` / + # ``inst.frida_address`` (each of which re-resolves ``inst.ports``) — one + # resolve per row rather than three (#230). ``frida`` degrades to the + # ``unsupported`` sentinel when the config has no frida service (#158). + adb_addr = f"localhost:{wk['adb']}" + frida_addr = f"localhost:{wk['frida']}" if "frida" in wk else api.FRIDA_ADDRESS_UNSUPPORTED meta = registry.get(inst.name) # Manager.list already filtered orphans; this branch is a defensive # net against a registry race and isn't covered. @@ -1048,15 +1066,15 @@ def _instance_json_row(inst: api.Instance) -> dict[str, object]: "created_at": meta.created_at.isoformat(), "ports": [{"service": rp.service, "guest": rp.guest, "host": rp.host} for rp in resolved], "status": inst.status, - "adb_address": inst.adb_address, - "frida_address": inst.frida_address, + "adb_address": adb_addr, + "frida_address": frida_addr, "stealth_paths": dict(backend.stealth_paths), # v0.3 back-compat keys — scripts piping ``ls --json`` through # jq depend on these. Kept alongside the v0.4 richer fields so # the row is a strict superset of the v0.3 shape. "path": str(inst.root), - "adb": f"localhost:{wk['adb']}", - "frida": f"localhost:{wk['frida']}", + "adb": adb_addr, + "frida": frida_addr, } @@ -1684,6 +1702,38 @@ def restore( console.hint(f"next: beetroot up {dest_name}") +# Domain exceptions the deep call tree raises that all map to the same +# ``error: `` + exit 1 contract. Collapsed into one tuple so +# ``main()`` stays under the branch-count lint and new members (like +# ``settings.InvalidSettingsError`` for a malformed ``BEETROOT_*`` var, #197) +# join without adding another near-identical ``except`` block. Rationale per +# member: +# * InvalidSettingsError — a malformed ``BEETROOT_*`` env var surfaces the +# first time a command reads ``settings`` through the lazy proxy, now inside +# this boundary rather than as a raw traceback at import (#197); +# * InstanceNotFoundError — unknown names / unresolvable backend kinds; +# * PortCollisionError / ComposeError / QemuLaunchError / BootstrapError / +# ModuleFetchError / FridaFetchError — lifecycle failures any verb can hit; +# * RegistryError — any registry walk ("unknown instance", "adb backend"); +# * ValidationError / YAMLError — a hostile or corrupt ``beetroot.yaml``; +# * FileNotFoundError — an instance dir ``rm -rf``'d behind the CLI's back. +_EXIT_1_ERRORS: tuple[type[BaseException], ...] = ( + settings.InvalidSettingsError, + api.InstanceNotFoundError, + paths.InstanceRootNotFoundError, + ports.PortCollisionError, + compose.ComposeError, + vm_qemu.QemuLaunchError, + builder.BootstrapError, + modules_download.ModuleFetchError, + frida_download.FridaFetchError, + registry.RegistryError, + pydantic.ValidationError, + yaml.YAMLError, + FileNotFoundError, +) + + def main() -> None: """ Parse CLI arguments and dispatch to the appropriate command handler. @@ -1705,73 +1755,10 @@ def main() -> None: # backend-typed exit codes; the rest stay 1 for source compat. console.error(str(e)) sys.exit(2) - except api.InstanceNotFoundError as e: - # Manager.resolve raises InstanceNotFoundError for unknown names - # and for unresolvable backend kinds (e.g. package not installed). - # v0.4 let these propagate as tracebacks; v0.6 catches them for - # a friendly error: ... line + exit 1. - console.error(str(e)) - sys.exit(1) - except paths.InstanceRootNotFoundError as e: - console.error(str(e)) - sys.exit(1) - except ports.PortCollisionError as e: - console.error(str(e)) - sys.exit(1) - except compose.ComposeError as e: - console.error(str(e)) - sys.exit(1) - except vm_qemu.QemuLaunchError as e: - # Any non-``up`` path to a QEMU launch (e.g. ``restart``, which - # calls ``up()`` after ``down()``) would otherwise dump a raw - # traceback. The ``up`` verb catches this inline for parity with - # its banner; this net covers every other verb so a missing - # artifact / ``accel: kvm`` without ``/dev/kvm`` maps to the same - # friendly ``error: ...`` + exit 1. - console.error(str(e)) - sys.exit(1) - except builder.BootstrapError as e: - console.error(str(e)) - sys.exit(1) - except modules_download.ModuleFetchError as e: - console.error(str(e)) - sys.exit(1) - except frida_download.FridaFetchError as e: - # ``apply`` (and any other verb that reaches _stage_network - # non-softly) can fail resolving or downloading frida-server; - # FridaFetchError is a RuntimeError, so the apply verb's inline - # ValueError catch lets it slip past. Map it here for the same - # friendly ``error: ...`` + exit 1 contract (#167). - console.error(str(e)) - sys.exit(1) - except registry.RegistryError as e: - # T2 Agent 3 1.9: any code path that walks the registry can - # surface a RegistryError ("unknown instance X", "X is an - # adb backend, no on-disk dir") that v0.3 let propagate as - # a Rich-rendered traceback. Catch it alongside the other - # domain exceptions for a friendly ``error: ...`` line. - console.error(str(e)) - sys.exit(1) - except (pydantic.ValidationError, yaml.YAMLError) as e: - # A hostile or corrupt ``beetroot.yaml`` (wrong field types, - # unsupported ``api_version``, the renamed ``stealth:`` section, - # or malformed YAML syntax) reaches ``config.load_yaml`` deep in - # the call tree. ``register``/``adopt`` catch ``ValueError`` (and - # ``ValidationError`` subclasses it) inline, but every name-resolved - # verb (``status``, ``up``, ``apply``, …) let these propagate as a - # Rich-rendered traceback. ``yaml.YAMLError`` isn't a ``ValueError`` - # at all, so even ``register`` tracebacked on a syntactically broken - # file. Catch both here for the uniform ``error: ...`` + exit 1 - # contract the rest of the CLI upholds. - console.error(str(e)) - sys.exit(1) - except FileNotFoundError as e: - # Belt-and-suspenders: an instance whose on-disk dir was - # ``rm -rf``'d behind the CLI's back leaves a stale registry - # entry; ``Instance.load`` then trips on the missing - # ``beetroot.yaml`` deep in the call tree. ``Manager.list`` - # filters orphans itself, but a verb that targets the orphan - # by name still needs this safety net. + except _EXIT_1_ERRORS as e: + # Every domain error the deep call tree can raise maps to the same + # friendly ``error: `` + exit 1 contract. The individual + # rationale for each member is documented on ``_EXIT_1_ERRORS``. console.error(str(e)) sys.exit(1) diff --git a/src/beetroot/console.py b/src/beetroot/console.py index fbae55f..6885df6 100644 --- a/src/beetroot/console.py +++ b/src/beetroot/console.py @@ -264,18 +264,21 @@ def table(columns: Sequence[str], rows: Sequence[Sequence[str]]) -> None: rows: Sequence of rows; each row is a sequence of cell strings whose length must match ``columns``. """ + # Every header and cell is markup-escaped so an instance path containing + # ``[brackets]`` renders literally instead of being parsed as a rich tag — + # which would silently corrupt the cell or crash with ``MarkupError`` (#259). if _stdout_console.is_terminal: - t = Table(*columns) + t = Table(*(escape(col) for col in columns)) for row in rows: - t.add_row(*row) + t.add_row(*(escape(cell) for cell in row)) _stdout_console.print(t) return t = Table(box=None, pad_edge=False) for col in columns: - t.add_column(col, overflow="fold", no_wrap=False) + t.add_column(escape(col), overflow="fold", no_wrap=False) for row in rows: - t.add_row(*row) + t.add_row(*(escape(cell) for cell in row)) # Width the render to the longest content line so rich never falls back to # its 80-column default and clips a cell with an ellipsis. max_cell = max( diff --git a/src/beetroot/frida_download.py b/src/beetroot/frida_download.py index 095e334..61224b4 100644 --- a/src/beetroot/frida_download.py +++ b/src/beetroot/frida_download.py @@ -14,17 +14,41 @@ import hashlib import lzma import os +import platform import shutil import subprocess import tempfile import urllib.error import urllib.request +from contextvars import ContextVar from pathlib import Path from typing import Final from . import config, console, paths from .settings import settings +# The frida-server architecture ``download`` should fetch, set by +# ``stage_for_instance`` to the backend-resolved arch around its call (#189). +# Kept as a context var rather than a ``download`` parameter so the public +# ``download`` signature stays stable for existing test doubles; ``None`` means +# "use the ``settings.frida_arch`` default" (resolved in ``cached_binary`` / +# ``release_url``). +_active_arch: ContextVar[str | None] = ContextVar("_active_arch", default=None) + +# Host machine strings (``platform.machine()``) mapped to the frida-server +# architecture suffix redroid expects for a ``binder: host|auto`` instance, +# which runs the Android userspace directly against the host kernel (#189). +_MACHINE_TO_FRIDA_ARCH: Final[dict[str, str]] = { + "aarch64": "android-arm64", + "arm64": "android-arm64", + "x86_64": "android-x86_64", + "amd64": "android-x86_64", +} + +# The ``binder: vm`` guest is an x86_64 micro-VM by design, so its frida-server +# is always the x86_64 build regardless of the host machine. +_VM_FRIDA_ARCH: Final = "android-x86_64" + # GitHub's per-repo "latest release" endpoint 302-redirects to the concrete # ``.../releases/tag/`` URL, so following the redirect and reading the # final tag resolves ``latest`` without hitting the rate-limited JSON API or @@ -50,19 +74,49 @@ class FridaFetchError(RuntimeError): """ -def release_url(version: str) -> str: +def resolve_frida_arch(binder: str) -> str: + """ + Resolve the frida-server architecture suffix for a given backend. + + An explicit ``BEETROOT_FRIDA_ARCH`` always wins (the researcher pinned a + cross-arch build on purpose). Otherwise the arch is backend-aware: a + ``binder: vm`` instance always uses the x86_64 build (its guest is an + x86_64 micro-VM), while a ``binder: host|auto`` instance runs Android + directly against the host kernel, so the arch is detected from + :func:`platform.machine` — an aarch64 host stages ``android-arm64`` rather + than an x86_64 ELF that never launches on ARM (#189). An unrecognized host + machine falls back to the ``settings.frida_arch`` default so behaviour is + never worse than before. + + Args: + binder: The instance's ``binder`` mode (``auto`` / ``host`` / ``vm``). + + Returns: + The frida-server architecture suffix (e.g. ``android-arm64``). + """ + if os.environ.get("BEETROOT_FRIDA_ARCH"): + return settings.frida_arch + if binder == "vm": + return _VM_FRIDA_ARCH + return _MACHINE_TO_FRIDA_ARCH.get(platform.machine().lower(), settings.frida_arch) + + +def release_url(version: str, *, arch: str | None = None) -> str: """ Return the GitHub download URL for a frida-server release. Args: version: The frida release tag (e.g. ``16.4.10``). + arch: The frida-server architecture suffix; defaults to + ``settings.frida_arch`` when not supplied by a backend-aware caller. Returns: The full HTTPS URL to the ``.xz`` compressed binary. """ + arch = arch if arch is not None else settings.frida_arch return ( f"https://github.com/frida/frida/releases/download/{version}/" - f"frida-server-{version}-{settings.frida_arch}.xz" + f"frida-server-{version}-{arch}.xz" ) @@ -166,17 +220,21 @@ def frida_cache_dir() -> Path: return paths.user_cache_dir("frida") -def cached_binary(version: str) -> Path: +def cached_binary(version: str, *, arch: str | None = None) -> Path: """ Return the cache path for a decompressed frida-server binary. Args: version: The frida release tag. + arch: The frida-server architecture suffix; defaults to + ``settings.frida_arch``. Included in the filename so an aarch64 + and an x86_64 build of the same version cache side by side (#189). Returns: Path under the user-global Frida cache where the binary lives. """ - return frida_cache_dir() / f"frida-server-{version}-{settings.frida_arch}" + arch = arch if arch is not None else settings.frida_arch + return frida_cache_dir() / f"frida-server-{version}-{arch}" def download(version: str, *, expected_sha256: str | None = None) -> Path: @@ -189,6 +247,12 @@ def download(version: str, *, expected_sha256: str | None = None) -> Path: and a ``ValueError`` is raised on mismatch — guards against a hostile mirror substituting the upstream release. + The architecture suffix is read from the ``_active_arch`` context var + (default ``settings.frida_arch``), which :func:`stage_for_instance` sets to + the backend-resolved arch around its call (#189). Keeping it out of the + signature preserves ``download``'s public shape so existing test doubles + stay assignment-compatible. + Args: version: The frida release tag to download. expected_sha256: Optional hex digest of the decompressed @@ -203,13 +267,14 @@ def download(version: str, *, expected_sha256: str | None = None) -> Path: ValueError: If ``expected_sha256`` is set and doesn't match the binary's actual digest. """ - out = cached_binary(version) + arch = _active_arch.get() + out = cached_binary(version, arch=arch) if out.exists() and out.stat().st_size > 0: _check_sha256(out, expected_sha256) return out out.parent.mkdir(parents=True, exist_ok=True) - url = release_url(version) + url = release_url(version, arch=arch) # Stage into a process-unique temp on the cache filesystem so two concurrent # fetches of the same version can't write a shared fixed ``.tmp`` and publish # a cross-contaminated binary via the atomic rename (#185). The compressed @@ -332,6 +397,7 @@ def stage_for_instance( version: str, *, expected_sha256: str | None = None, + binder: str = "auto", ) -> Path: """ Copy the cached frida-server binary into the instance's directory. @@ -349,6 +415,8 @@ def stage_for_instance( :func:`download` for integrity verification. Comparison is case-insensitive. Only valid with a pinned ``version`` (enforced by :class:`beetroot.config.Frida`). + binder: The instance's ``binder`` mode, used to resolve the + host-matching frida-server architecture (#189). Returns: Path to the staged binary inside the instance directory. @@ -361,11 +429,31 @@ def stage_for_instance( if resolved != version: console.note(f"frida version {version!r} resolved to {resolved}") _warn_on_client_skew(resolved, host_version=host_version) - src = download(resolved, expected_sha256=expected_sha256) + # Publish the backend-resolved arch (e.g. an aarch64 ``host`` instance → + # arm64) for ``download`` to read, restoring the prior value afterwards so + # nested/sequential stages don't leak arch state. + arch = resolve_frida_arch(binder) + token = _active_arch.set(arch) + try: + src = download(resolved, expected_sha256=expected_sha256) + finally: + _active_arch.reset(token) dst = paths.instance_frida(instance_root) dst.parent.mkdir(parents=True, exist_ok=True) - shutil.copyfile(src, dst) - dst.chmod(0o755) + # Stage via a process-unique temp beside the target and ``os.replace`` on + # success, so a download failure on a *later* re-apply can never truncate a + # prior working binary mid-copy (#165). The download itself already + # succeeded above, so the copy+swap here is the only remaining window. + fd, tmp_name = tempfile.mkstemp(dir=dst.parent, prefix=".frida-server.", suffix=".tmp") + os.close(fd) + tmp = Path(tmp_name) + try: + shutil.copyfile(src, tmp) + tmp.chmod(0o755) + tmp.replace(dst) + except BaseException: + tmp.unlink(missing_ok=True) + raise return dst diff --git a/src/beetroot/modules_download.py b/src/beetroot/modules_download.py index eae0230..744cf32 100644 --- a/src/beetroot/modules_download.py +++ b/src/beetroot/modules_download.py @@ -127,14 +127,31 @@ def _fetch_url(url: str) -> Path: urllib.request.urlopen(url, timeout=settings.http_timeout) as resp, # noqa: S310 # scheme validated by Module pydantic model + _fetch_url allowlist ): raw_length = resp.headers.get("Content-Length") - total: float | None = float(raw_length) if raw_length else None + # Only honor a well-formed numeric header; a missing or malformed + # Content-Length leaves the total unknown (indeterminate bar, no + # truncation check) rather than crashing the download. + expected_bytes: int | None = ( + int(raw_length) if isinstance(raw_length, str) and raw_length.isdigit() else None + ) + total: float | None = float(expected_bytes) if expected_bytes is not None else None + written = 0 with console.progress(f"Fetching module {filename}", total=total) as bar: while True: chunk = resp.read(_CHUNK_SIZE) if not chunk: break out.write(chunk) + written += len(chunk) bar.advance(len(chunk)) + # A dropped connection at a chunk boundary yields a clean EOF, not an + # exception, so a short read would otherwise cache a truncated zip and + # re-serve it forever. Compare bytes-received to the advertised + # Content-Length before publishing and reject a short read (#261). + if expected_bytes is not None and written != expected_bytes: + raise ModuleFetchError( + f"download truncated: got {written} of {expected_bytes} bytes for {url}; " + "the connection dropped mid-stream — retry the fetch" + ) tmp.replace(cache) except urllib.error.HTTPError as e: tmp.unlink(missing_ok=True) diff --git a/src/beetroot/registry.py b/src/beetroot/registry.py index b146d13..b0c6717 100644 --- a/src/beetroot/registry.py +++ b/src/beetroot/registry.py @@ -821,8 +821,21 @@ def all_resolved_host_ports() -> dict[str, set[int]]: registered instance. Empty dict if the registry is empty or every directory-backed entry is an orphan. """ + return _resolved_host_ports_from(list_instances()) + + +def _resolved_host_ports_from(instances: dict[str, InstanceMeta]) -> dict[str, set[int]]: + """ + Compute the resolved host-port sets for an already-read instances mapping. + + Split out of :func:`all_resolved_host_ports` so the collision precheck can + reuse it against instances read *inside* the exclusive registry lock, + without re-entering :func:`list_instances` (which would take a second lock + on the same file — #183). Pure with respect to the registry: it only reads + per-instance ``beetroot.yaml`` files, never the registry itself. + """ out: dict[str, set[int]] = {} - for name, meta in list_instances().items(): + for name, meta in instances.items(): backend = meta.backend if isinstance(backend, RedroidBackendConfig | VmBackendConfig): try: @@ -878,3 +891,35 @@ def find_port_collision( if rp.host in other_ports: return rp.host, other_name, str(rp.service) return None + + +def assert_no_port_collision(name: str, new_ports: list[ports.ResolvedPort]) -> None: + """ + Raise ``ValueError`` if ``new_ports`` collide with a sibling — under the lock. + + The sibling read (``beetroot.yaml`` resolve) and the collision decision run + inside a single *exclusive* registry critical section, closing the TOCTOU + window where two concurrent ``apply``/``create`` operations pinning the same + explicit ``host:`` port could both pass the precheck and double-bind at + ``up`` time (#183). Resolved sibling ports aren't persisted, so they're + re-read from disk here rather than from the registry file. + + Args: + name: The staging instance's name (excluded from the sibling scan). + new_ports: The staging instance's resolved host ports. + + Raises: + ValueError: On the first cross-instance host-port collision. + """ + lock_path = paths.user_registry_file() + with _locked(lock_path, exclusive=True): + siblings = _resolved_host_ports_from(_read(lock_path).instances) + others = {n: p for n, p in siblings.items() if n != name} + collision = find_port_collision(new_ports, others) + if collision is None: + return + port, other_name, kind = collision + raise ValueError( + f"port {port} ({kind}) collides with instance {other_name!r} " + f"(which also uses {port}). Pin or remove one." + ) diff --git a/src/beetroot/settings.py b/src/beetroot/settings.py index fdf1e37..baeac11 100644 --- a/src/beetroot/settings.py +++ b/src/beetroot/settings.py @@ -31,10 +31,25 @@ from __future__ import annotations +import pydantic from pydantic import PositiveInt from pydantic_settings import BaseSettings, SettingsConfigDict +class InvalidSettingsError(RuntimeError): + """ + Raised when a ``BEETROOT_*`` environment variable fails validation. + + ``Settings()`` is constructed at import time, *before* ``cli.main()``'s + ``try``/``except`` error boundary is reached, so a raw + ``pydantic.ValidationError`` (e.g. a non-numeric ``BEETROOT_HTTP_TIMEOUT``) + would otherwise escape as an unhandled traceback that bricks even + ``beetroot --help`` (#197). Wrapping construction in this domain error and + catching it in ``cli.main()`` maps a malformed setting to the friendly + ``error: ...`` + exit 1 contract the rest of the CLI upholds. + """ + + class Settings(BaseSettings): """ Runtime overrides sourced from ``BEETROOT_*`` environment variables. @@ -108,4 +123,60 @@ class Settings(BaseSettings): vm_adb_connect_timeout: PositiveInt = 60 -settings = Settings() +def _build_settings() -> Settings: + """ + Construct :class:`Settings`, mapping a validation failure to a domain error. + + A malformed ``BEETROOT_*`` env var (e.g. a non-numeric ``BEETROOT_HTTP_TIMEOUT``) + otherwise raises a raw ``pydantic.ValidationError``. Re-raising as + :class:`InvalidSettingsError` lets ``cli.main()`` emit the friendly + ``error: ...`` + exit 1 line instead (#197). + + Returns: + A validated :class:`Settings` instance. + + Raises: + InvalidSettingsError: If any ``BEETROOT_*`` env var fails validation. + """ + try: + return Settings() + except pydantic.ValidationError as e: + raise InvalidSettingsError(f"invalid BEETROOT_* environment variable: {e}") from e + + +class _LazySettings: + """ + Import-safe proxy that defers :class:`Settings` construction to first use. + + Consumers bind this proxy via ``from .settings import settings`` at *their* + import time, but the actual env-var validation only runs when an attribute is + first read — which happens at CLI *runtime*, inside ``cli.main()``'s error + boundary. This is what lets a malformed ``BEETROOT_*`` var map to the friendly + ``error: ...`` + exit 1 line instead of a raw traceback that bricks even + ``beetroot --help`` at import (#197). The resolved instance is cached, so the + env is read at most once per process. + """ + + __slots__ = ("_resolved",) + + def __init__(self) -> None: + self._resolved: Settings | None = None + + def _get(self) -> Settings: + """ + Return the cached :class:`Settings`, building it on first access. + """ + if self._resolved is None: + self._resolved = _build_settings() + return self._resolved + + def __getattr__(self, name: str) -> object: + # __slots__ + the leading-underscore guard keep this from recursing on + # ``self._resolved``; every other attribute routes to the real Settings. + return getattr(self._get(), name) + + +# The runtime object is the lazy proxy (import-safe), but every attribute it +# forwards is a real ``Settings`` field, so it's typed as ``Settings`` for +# consumers — mypy sees ``settings.http_timeout: int`` etc. +settings: Settings = _LazySettings() # type: ignore[assignment] # proxy forwards to Settings diff --git a/src/beetroot/snapshot.py b/src/beetroot/snapshot.py index 646e9cc..48df444 100644 --- a/src/beetroot/snapshot.py +++ b/src/beetroot/snapshot.py @@ -43,6 +43,12 @@ INSTANCE_LOCK_FILENAME = ".beetroot.lock" SCHEMA_VERSION = 1 _ARCHIVE_SUFFIX = ".tar.zst" +# Decompression-bomb ceiling: a snapshot's zstd stream can inflate to +# far more than its on-disk size, so restore caps the total decompressed +# payload it will extract. 64 GiB is a generous headroom over any real +# ``/data`` snapshot while still refusing a maliciously crafted archive +# that would otherwise exhaust host disk during extraction. +_MAX_EXTRACT_BYTES = 64 * 1024 * 1024 * 1024 # .env is regenerated from beetroot.yaml on the next apply. The # manifest itself is excluded because the archive's *root*-level # manifest is the authoritative one; if a previous restore left a @@ -179,8 +185,12 @@ def snapshot(instance_root: Path, dest: Path) -> Path: relative paths like ``./beetroot.yaml``, ``./data/...``, ``./modules/...``). The ``.env`` file is deliberately excluded — it's regenerated from ``beetroot.yaml`` on the next - ``beetroot apply``. The manifest is written as the archive's last - member. + ``beetroot apply``. The manifest is written as the archive's FIRST + member (#265) so :func:`read_manifest` can early-exit after reading a + few KiB rather than streaming/decompressing the whole archive to reach + a trailing manifest; an older archive that carried the manifest last + still restores because :func:`read_manifest` matches by member name, + wherever it sits. Holds a SHARED ``fcntl.flock`` on ``/.beetroot.lock`` for the duration of the archive write — multiple snapshots can run @@ -242,8 +252,11 @@ def snapshot(instance_root: Path, dest: Path) -> Path: cctx.stream_writer(raw_out) as zst, tarfile.open(fileobj=zst, mode="w|") as tar, ): - _add_instance_tree(tar, instance_root, final_dest) + # Manifest FIRST (#265): read_manifest early-exits on the first + # matching member, so a leading manifest lets restore read it + # after decompressing only a few KiB instead of the whole tree. _add_manifest(tar, manifest) + _add_instance_tree(tar, instance_root, final_dest) return final_dest @@ -672,7 +685,19 @@ def _extract_archive_into(archive: Path, target: Path) -> None: dctx.stream_reader(raw_in) as zst, tarfile.open(fileobj=zst, mode="r|") as tar, ): + # Cap the cumulative decompressed size (#265, folded low-severity + # zstd-bomb finding): a hostile archive could inflate to fill the + # host disk, so refuse past a generous ceiling before extracting + # the offending member. + total = 0 for member in tar: + total += member.size + if total > _MAX_EXTRACT_BYTES: + raise SnapshotError( + f"archive {archive} exceeds the {_MAX_EXTRACT_BYTES}-byte " + "decompressed-size cap; refusing to extract (possible " + "decompression bomb)" + ) tar.extract(member, path=target, filter="data") if not paths.instance_yaml(target).is_file(): raise SnapshotError( diff --git a/src/beetroot/vm/boot_cache.py b/src/beetroot/vm/boot_cache.py index c1e2792..4242091 100644 --- a/src/beetroot/vm/boot_cache.py +++ b/src/beetroot/vm/boot_cache.py @@ -114,7 +114,16 @@ def overlay_key_path(instance_dir: Path) -> Path: return instance_dir / _OVERLAY_KEY_NAME -def _hash_file(path: Path) -> str: +# Memoized {(path, st_size, st_mtime_ns): sha256-hexdigest}. The rootfs is a +# multi-GB *immutable* artifact, so a full SHA-256 re-stream on every warm-resume +# staleness check (`base_identity` runs on every `up`) is pure waste. Keying on +# (path, size, mtime_ns) reuses the digest whenever the file is byte-identical +# and recomputes only when size/mtime say it changed — same final fingerprint, +# no re-hash on the hot path (issue #254). +_HASH_CACHE: dict[tuple[str, int, int], str] = {} + + +def _stream_sha256(path: Path) -> str: digest = hashlib.sha256() with path.open("rb") as fh: while chunk := fh.read(_IDENTITY_CHUNK): @@ -122,6 +131,17 @@ def _hash_file(path: Path) -> str: return digest.hexdigest() +def _hash_file(path: Path) -> str: + stat = path.stat() + key = (str(path), stat.st_size, stat.st_mtime_ns) + cached = _HASH_CACHE.get(key) + if cached is not None: + return cached + hexdigest = _stream_sha256(path) + _HASH_CACHE[key] = hexdigest + return hexdigest + + def base_identity(kernel: Path, rootfs: Path, smp: int, memory_mib: int) -> str: """ Compute a stable digest over the kernel + rootfs + geometry an overlay uses. diff --git a/tests/test_adb_device.py b/tests/test_adb_device.py index a956eb4..17a8830 100644 --- a/tests/test_adb_device.py +++ b/tests/test_adb_device.py @@ -181,7 +181,8 @@ def test_emits_full_install_sequence( "755", "/data/local/tmp/frida-server", ] - # 3. launch via su + # 3. launch via su — stdio detached off the captured adb pipe so + # the long-lived daemon can't wedge subprocess.run (issue #253) assert captured_adb[2] == [ "adb", "-s", @@ -189,7 +190,7 @@ def test_emits_full_install_sequence( "shell", "su", "-c", - "/data/local/tmp/frida-server &", + "/data/local/tmp/frida-server /dev/null 2>&1 &", ] # 4. adb forward (host_port → device 27042) assert captured_adb[3] == [ @@ -1180,3 +1181,145 @@ def _spy(device: api.DeviceBackend) -> dict[str, api.CheckResult]: out = dev.health() assert out is sentinel assert calls == [dev] + + +def _capture_kwargs(monkeypatch: pytest.MonkeyPatch) -> list[dict[str, object]]: + """Stub subprocess.run to record each call's kwargs (returns rc 0).""" + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + recorded: list[dict[str, object]] = [] + + def _fake_run( + cmd: list[str], + *args: object, + **kwargs: object, + ) -> subprocess.CompletedProcess[str]: + del args + recorded.append(dict(kwargs)) + return subprocess.CompletedProcess( + args=cmd, + returncode=0, + stdout="emulator-5554\tdevice\n", + stderr="", + ) + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _fake_run) + return recorded + + +class TestAdbTimeouts: + """Issue #256 — every capturing adb call is bounded by a timeout.""" + + def test_serial_is_available_passes_timeout(self, monkeypatch: pytest.MonkeyPatch) -> None: + recorded = _capture_kwargs(monkeypatch) + adb_backend.serial_is_available("emulator-5554") + assert recorded[0]["timeout"] == adb_backend._ADB_TIMEOUT_SECONDS + + def test_serial_is_available_unavailable_on_timeout( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # A wedged adb server that never returns must be reported as + # unavailable (a clean False), not left to hang ls/status. + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + + def _hang(cmd: list[str], *args: object, **kwargs: object) -> object: + del cmd, args, kwargs + raise subprocess.TimeoutExpired(cmd=["adb", "devices"], timeout=5) + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _hang) + assert adb_backend.serial_is_available("emulator-5554") is False + + def test_adb_unchecked_passes_timeout(self, monkeypatch: pytest.MonkeyPatch) -> None: + recorded = _capture_kwargs(monkeypatch) + _make_device()._adb_unchecked("shell", "id") + assert recorded[0]["timeout"] == adb_backend._ADB_TIMEOUT_SECONDS + + def test_adb_unchecked_timeout_becomes_failed_result( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # A wedged shell must surface as a non-zero CompletedProcess + # (rc 124) rather than propagating TimeoutExpired — so doctor / + # status fail cleanly. The synthesized stderr names the timeout. + def _hang(cmd: list[str], *args: object, **kwargs: object) -> object: + del args, kwargs + raise subprocess.TimeoutExpired(cmd=cmd, timeout=5) + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _hang) + res = _make_device(serial="emulator-5554")._adb_unchecked("shell", "id") + assert res.returncode == 124 + assert res.stdout == "" + assert "timed out" in res.stderr + + def test_adb_unchecked_timeout_preserves_partial_output( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # When the killed adb wrote partial captured output before the + # cap fired, that text is carried through on the synthesized row + # rather than discarded. + # Mixed types on purpose: a ``str`` stdout (the ``text=True`` + # runtime shape) and a ``bytes`` stderr (what typeshed types the + # field as) both normalise to str via ``_as_text``. + def _hang(cmd: list[str], *args: object, **kwargs: object) -> object: + del args, kwargs + raise subprocess.TimeoutExpired( + cmd=cmd, timeout=5, output="half\n", stderr=b"warn\n" + ) + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _hang) + res = _make_device()._adb_unchecked("shell", "id") + assert res.returncode == 124 + assert res.stdout == "half\n" + assert res.stderr == "warn\n" + + def test_adb_raises_runtime_error_on_timeout(self, monkeypatch: pytest.MonkeyPatch) -> None: + # The checked ``_adb`` wrapper turns the rc-124 timeout row into + # the same RuntimeError any non-zero adb exit produces, so the + # health checks built on it degrade instead of hanging. + def _hang(cmd: list[str], *args: object, **kwargs: object) -> object: + del args, kwargs + raise subprocess.TimeoutExpired(cmd=cmd, timeout=5) + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _hang) + with pytest.raises(RuntimeError, match="rc=124"): + _make_device()._adb("shell", "id") + + +class TestInstallFridaDetachesDaemonStdio: + """Issue #253 — the frida launch must not wedge subprocess.run.""" + + def test_launch_redirects_daemon_stdio( + self, + captured_adb: list[list[str]], + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + ) -> None: + fake_cached = tmp_path / "frida-server-16.4.10" + fake_cached.write_bytes(b"fake-binary") + monkeypatch.setattr(frida_download, "download", lambda version: fake_cached) + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + _make_device(serial="emulator-5554", host_port=27052).install_frida("16.4.10") + launch = captured_adb[2] + assert launch[-3:] == ["su", "-c", "/data/local/tmp/frida-server /dev/null 2>&1 &"] + + def test_wedged_launch_raises_instead_of_hanging( + self, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + ) -> None: + # Belt-and-braces: even if stdio redirection were bypassed and + # the launch call itself timed out, the timeout is surfaced as a + # RuntimeError (rc 124) rather than blocking install_frida + # forever. + fake_cached = tmp_path / "frida-server-16.4.10" + fake_cached.write_bytes(b"fake-binary") + monkeypatch.setattr(frida_download, "download", lambda version: fake_cached) + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + + def _run(cmd: list[str], *args: object, **kwargs: object) -> object: + del args, kwargs + if "su" in cmd: + raise subprocess.TimeoutExpired(cmd=cmd, timeout=5) + return subprocess.CompletedProcess(args=cmd, returncode=0, stdout="", stderr="") + + monkeypatch.setattr("beetroot.backends.adb.subprocess.run", _run) + with pytest.raises(RuntimeError, match="rc=124"): + _make_device(serial="emulator-5554").install_frida("16.4.10") diff --git a/tests/test_bugfix_frida_arch.py b/tests/test_bugfix_frida_arch.py new file mode 100644 index 0000000..d9b8457 --- /dev/null +++ b/tests/test_bugfix_frida_arch.py @@ -0,0 +1,110 @@ +"""Regression tests for #189 — frida-server arch auto-detects from the host. + +Before the fix, the arch was hardcoded to ``settings.frida_arch`` +(``android-x86_64``) with no host-arch probe, so an aarch64 redroid host/auto +instance staged an x86_64 ELF that never launched in the ARM guest. Now the arch +is backend-aware: ``binder: vm`` is pinned to x86_64, host/auto detects from +``platform.machine()``, and an explicit ``BEETROOT_FRIDA_ARCH`` always wins. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from beetroot import frida_download +from beetroot.settings import settings + + +@pytest.fixture +def instance_root(isolated_registry: Path, tmp_path: Path) -> Path: + """An empty instance directory under the isolated XDG tree.""" + root = tmp_path / "alpha" + root.mkdir() + return root + + +def _reset_settings(monkeypatch: pytest.MonkeyPatch) -> None: + """Force the lazy settings proxy to re-read env after a setenv/delenv.""" + monkeypatch.setattr("beetroot.settings.settings._resolved", None) + + +@pytest.mark.parametrize("binder", ["auto", "host", "vm"]) +def test_explicit_env_arch_always_wins(binder: str, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("BEETROOT_FRIDA_ARCH", "android-arm") + _reset_settings(monkeypatch) + monkeypatch.setattr("beetroot.frida_download.platform.machine", lambda: "aarch64") + assert frida_download.resolve_frida_arch(binder) == "android-arm" + + +@pytest.mark.parametrize("machine", ["x86_64", "amd64", "aarch64", "arm64", "riscv64"]) +def test_vm_is_always_x86_64(machine: str, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("BEETROOT_FRIDA_ARCH", raising=False) + _reset_settings(monkeypatch) + monkeypatch.setattr("beetroot.frida_download.platform.machine", lambda: machine) + assert frida_download.resolve_frida_arch("vm") == "android-x86_64" + + +@pytest.mark.parametrize( + ("machine", "expected"), + [ + ("aarch64", "android-arm64"), + ("arm64", "android-arm64"), + ("x86_64", "android-x86_64"), + ("amd64", "android-x86_64"), + ], +) +def test_host_auto_detects_from_machine( + machine: str, expected: str, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv("BEETROOT_FRIDA_ARCH", raising=False) + _reset_settings(monkeypatch) + monkeypatch.setattr("beetroot.frida_download.platform.machine", lambda: machine) + assert frida_download.resolve_frida_arch("host") == expected + assert frida_download.resolve_frida_arch("auto") == expected + + +def test_unknown_machine_falls_back_to_default(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("BEETROOT_FRIDA_ARCH", raising=False) + _reset_settings(monkeypatch) + monkeypatch.setattr("beetroot.frida_download.platform.machine", lambda: "sparc64") + # No mapping → the ``settings.frida_arch`` default (never worse than before). + assert frida_download.resolve_frida_arch("auto") == settings.frida_arch + + +def test_cached_binary_arch_in_filename() -> None: + p = frida_download.cached_binary("16.4.10", arch="android-arm64") + assert p.name == "frida-server-16.4.10-android-arm64" + + +def test_release_url_arch_suffix() -> None: + url = frida_download.release_url("16.4.10", arch="android-arm64") + assert url.endswith("frida-server-16.4.10-android-arm64.xz") + + +def test_stage_for_instance_threads_arm64_arch( + instance_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv("BEETROOT_FRIDA_ARCH", raising=False) + _reset_settings(monkeypatch) + monkeypatch.setattr("beetroot.frida_download.platform.machine", lambda: "aarch64") + + seen: dict[str, object] = {} + + def _fake_download(version: str, *, expected_sha256: str | None = None) -> Path: + # ``stage_for_instance`` publishes the resolved arch via the context var. + arch = frida_download._active_arch.get() + seen["arch"] = arch + out = frida_download.cached_binary(version, arch=arch) + out.parent.mkdir(parents=True, exist_ok=True) + out.write_bytes(b"fake-arm64") + out.chmod(0o755) + return out + + monkeypatch.setattr("beetroot.frida_download.download", _fake_download) + monkeypatch.setattr("beetroot.frida_download.host_frida_tools_version", lambda: None) + + dst = frida_download.stage_for_instance(instance_root, "16.4.10", binder="host") + assert seen["arch"] == "android-arm64" + assert dst.read_bytes() == b"fake-arm64" diff --git a/tests/test_bugfix_frida_less_ports.py b/tests/test_bugfix_frida_less_ports.py new file mode 100644 index 0000000..e917d2c --- /dev/null +++ b/tests/test_bugfix_frida_less_ports.py @@ -0,0 +1,107 @@ +"""Regression tests for #158 — a Frida-less ports config must not crash ls/status. + +A valid adb-only ``ports:`` config (no ``frida``/``frida_control`` service, no +``frida:`` block) produces a ``ports.well_known`` dict with no ``frida`` key. +Before the fix, ``Instance.frida_address`` / ``health()`` / ``_instance_json_row`` +indexed ``wk['frida']`` eagerly and raised ``KeyError: 'frida'``, taking down the +whole-fleet ``ls`` because every row is built in one comprehension. Now the +redroid backend returns the ``unsupported`` sentinel like the vm backend. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import patch + +import pytest +from typer.testing import CliRunner + +from beetroot import api, cli, compose, config, registry +from beetroot.config import PortMapping + + +def _adb_only_cfg() -> config.InstanceConfig: + """Build a validator-accepted adb-only config (no Frida service at all).""" + return config.InstanceConfig( + frida=None, + ports=[PortMapping(service="adb", guest=5555)], + ) + + +def test_frida_address_returns_sentinel_for_adb_only(cli_root: Path) -> None: + registry.add_allocating("alpha", cli_root / "alpha") + inst = api.Instance(name="alpha", root=cli_root / "alpha", cfg=_adb_only_cfg()) + assert inst.frida_address == api.FRIDA_ADDRESS_UNSUPPORTED + # adb still resolves normally — only frida is absent. + assert inst.adb_address == "localhost:5555" + + +def test_health_does_not_crash_without_frida_service( + cli_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + registry.add_allocating("alpha", cli_root / "alpha") + inst = api.Instance(name="alpha", root=cli_root / "alpha", cfg=_adb_only_cfg()) + # ``health()`` used to KeyError on ``wk['frida']`` before probing anything; + # stub the compose/adb/binder probes so the test isolates that fix. + monkeypatch.setattr(compose, "ps_status", lambda name, root: "running") + monkeypatch.setattr("shutil.which", lambda _: None) + with patch("subprocess.run") as run: + run.return_value = None + checks = inst.health() + assert "frida.handshake" in checks + # frida disabled (cfg.frida is None) → the socket check is skipped, not run. + assert checks["frida.handshake"].status == "skip" + + +def test_instance_json_row_survives_frida_less_config(cli_root: Path, tmp_path: Path) -> None: + root = tmp_path / "alpha" + root.mkdir() + (root / "beetroot.yaml").write_text( + "api_version: 3\nandroid:\n version: 14\nports:\n" + " - service: adb\n guest: 5555\n" + ) + registry.add_allocating("alpha", root) + inst = api.Instance.load("alpha") + row = cli._instance_json_row(inst) + assert row["frida"] == api.FRIDA_ADDRESS_UNSUPPORTED + assert row["frida_address"] == api.FRIDA_ADDRESS_UNSUPPORTED + assert row["adb"] == "localhost:5555" + + +def _write_instance(tmp_path: Path, name: str, *, frida: bool) -> Path: + root = tmp_path / name + root.mkdir() + body = "api_version: 3\nandroid:\n version: 14\n" + if frida: + body += "frida:\n version: '16.4.10'\n" + else: + body += "ports:\n - service: adb\n guest: 5555\n" + (root / "beetroot.yaml").write_text(body) + registry.add_allocating(name, root) + return root + + +def test_fleet_ls_renders_both_frida_full_and_frida_less( + isolated_registry: Path, tmp_path: Path +) -> None: + _write_instance(tmp_path, "withfrida", frida=True) + _write_instance(tmp_path, "nofrida", frida=False) + + result = CliRunner().invoke(cli.app, ["ls"]) + assert result.exit_code == 0, result.stderr + # The whole fleet renders — neither instance aborts the comprehension. + assert "withfrida" in result.stdout + assert "nofrida" in result.stdout + assert api.FRIDA_ADDRESS_UNSUPPORTED in result.stdout + + +def test_fleet_ls_json_renders_both_rows(isolated_registry: Path, tmp_path: Path) -> None: + _write_instance(tmp_path, "withfrida", frida=True) + _write_instance(tmp_path, "nofrida", frida=False) + + result = CliRunner().invoke(cli.app, ["ls", "--json"]) + assert result.exit_code == 0, result.stderr + rows = json.loads(result.stdout) + assert rows["nofrida"]["frida"] == api.FRIDA_ADDRESS_UNSUPPORTED + assert rows["withfrida"]["frida"].startswith("localhost:") diff --git a/tests/test_bugfix_frida_placeholder.py b/tests/test_bugfix_frida_placeholder.py new file mode 100644 index 0000000..e8b1aa6 --- /dev/null +++ b/tests/test_bugfix_frida_placeholder.py @@ -0,0 +1,135 @@ +"""Regression tests for #165 — apply must not wipe a working frida-server. + +``_stage_local`` used to write the empty placeholder unconditionally, zeroing a +real binary to 0 bytes / 0o644 *before* ``_stage_network`` re-downloaded it. On a +cache-miss re-fetch failure the instance was left with a non-executable 0-byte +frida-server that ``launch-frida.sh`` skips — Frida silently disabled. The fix +keeps a usable binary in place, and stages the fresh one atomically on success. +""" + +from __future__ import annotations + +import stat +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from beetroot import api, cli, frida_download, paths + + +def _write_frida_yaml(root: Path) -> None: + (root / "beetroot.yaml").write_text( + "api_version: 3\nandroid:\n version: 14\nfrida:\n version: '16.4.10'\n" + ) + + +def _stage_fake_binary(root: Path, content: bytes = b"REAL-FRIDA-BINARY") -> Path: + frida = paths.instance_frida(root) + frida.parent.mkdir(parents=True, exist_ok=True) + frida.write_bytes(content) + frida.chmod(0o755) + return frida + + +def test_apply_preserves_working_binary_when_refetch_fails( + cli_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + CliRunner().invoke(cli.app, ["create", "alpha"]) + root = api.Instance.load("alpha").root + _write_frida_yaml(root) + frida = _stage_fake_binary(root) + original = frida.read_bytes() + + def _boom(version: str, *, expected_sha256: str | None = None) -> Path: + raise frida_download.FridaFetchError("simulated cache-miss re-fetch failure") + + monkeypatch.setattr(frida_download, "download", _boom) + + with pytest.raises(frida_download.FridaFetchError): + api.Instance.load("alpha").apply() + + # The prior working binary is untouched: same bytes, non-zero, still exec. + assert frida.read_bytes() == original + assert frida.stat().st_size > 0 + assert frida.stat().st_mode & stat.S_IXUSR + + +def test_apply_with_valid_binary_skips_placeholder( + cli_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + CliRunner().invoke(cli.app, ["create", "alpha"]) + root = api.Instance.load("alpha").root + _write_frida_yaml(root) + _stage_fake_binary(root) + + called = {"stage_empty": False} + real_stage_empty = frida_download.stage_empty + + def _spy(instance_root: Path) -> Path: + called["stage_empty"] = True + return real_stage_empty(instance_root) + + monkeypatch.setattr(frida_download, "stage_empty", _spy) + # Let the (stubbed cli_root) download succeed so apply completes. + api.Instance.load("alpha").apply() + + # With a configured frida AND an existing non-empty binary, the placeholder + # branch is skipped — the real binary is never zeroed first. + assert called["stage_empty"] is False + + +def test_apply_without_frida_still_writes_placeholder(cli_root: Path) -> None: + CliRunner().invoke(cli.app, ["create", "alpha"]) + root = api.Instance.load("alpha").root + # No frida: block → the bind-mount target must still exist as a placeholder. + (root / "beetroot.yaml").write_text("api_version: 3\nandroid:\n version: 14\n") + api.Instance.load("alpha").apply() + + frida = paths.instance_frida(root) + assert frida.exists() + assert frida.stat().st_size == 0 + assert not (frida.stat().st_mode & stat.S_IXUSR) + + +def test_stage_for_instance_swaps_atomically( + cli_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = cli_root / "alpha" + root.mkdir() + _stage_fake_binary(root, b"OLD-BINARY") + frida = paths.instance_frida(root) + + # The cli_root fixture stubs download to write b"fake-frida"; staging must + # replace the old binary with the fresh cached one via an atomic swap. + monkeypatch.setattr(frida_download, "host_frida_tools_version", lambda: None) + dst = frida_download.stage_for_instance(root, "16.4.10") + assert dst == frida + assert frida.read_bytes() == b"fake-frida" + # No leftover temp files beside the target. + leftovers = [p for p in root.iterdir() if p.name.startswith(".frida-server.")] + assert leftovers == [] + + +def test_stage_for_instance_cleans_temp_on_copy_failure( + cli_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = cli_root / "alpha" + root.mkdir() + _stage_fake_binary(root, b"OLD-BINARY") + frida = paths.instance_frida(root) + + monkeypatch.setattr(frida_download, "host_frida_tools_version", lambda: None) + + def _boom_copy(src: str | Path, dst: str | Path) -> None: + raise OSError("disk full") + + monkeypatch.setattr("beetroot.frida_download.shutil.copyfile", _boom_copy) + + with pytest.raises(OSError, match="disk full"): + frida_download.stage_for_instance(root, "16.4.10") + + # The old binary survives and no temp is orphaned. + assert frida.read_bytes() == b"OLD-BINARY" + leftovers = [p for p in root.iterdir() if p.name.startswith(".frida-server.")] + assert leftovers == [] diff --git a/tests/test_bugfix_ls_snapshot_reuse.py b/tests/test_bugfix_ls_snapshot_reuse.py new file mode 100644 index 0000000..060f0f1 --- /dev/null +++ b/tests/test_bugfix_ls_snapshot_reuse.py @@ -0,0 +1,112 @@ +"""Regression tests for #230 — ls/status reuse a snapshot + memoize the index. + +ls/status re-read the registry and each ``beetroot.yaml`` O(N)+ times per +command because ``Instance.index`` / ``ports`` / ``adb_address`` / +``frida_address`` each re-called ``registry.get`` (via ``_meta``) and the cli +row builders re-derived addresses already computed. The fix memoizes +``Instance.index`` as a ``cached_property`` and derives the row addresses from a +single ``well_known`` dict — so the output is byte-for-byte identical but the +redundant reads collapse. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from beetroot import api, cli, registry + + +def _make(tmp_path: Path, name: str, *, frida: bool = True) -> Path: + root = tmp_path / name + root.mkdir() + body = "api_version: 3\nandroid:\n version: 14\n" + if not frida: + body += "ports:\n - service: adb\n guest: 5555\n" + (root / "beetroot.yaml").write_text(body) + registry.add_allocating(name, root) + return root + + +def test_index_is_computed_once(cli_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + CliRunner().invoke(cli.app, ["create", "alpha"]) + inst = api.Instance.load("alpha") + + calls = {"n": 0} + real_get = registry.get + + def _spy_get(name: str) -> registry.InstanceMeta | None: + calls["n"] += 1 + return real_get(name) + + # ``_meta`` reaches ``registry.get`` on the shared module object. + monkeypatch.setattr("beetroot.registry.get", _spy_get) + + # Two accesses of the cached_property → exactly one registry lookup. + first = inst.index + second = inst.index + + assert first == second + assert calls["n"] == 1 + + +def test_meta_still_raises_when_row_disappears(cli_root: Path) -> None: + # The cached_property must preserve the disappearance contract: a fresh + # Instance whose registry row is gone raises on first index access. + CliRunner().invoke(cli.app, ["create", "alpha"]) + inst = api.Instance.load("alpha") + registry.remove("alpha") + with pytest.raises(api.InstanceNotFoundError): + _ = inst.index + + +def test_ls_json_output_unchanged_and_bounded_yaml_reads( + cli_root: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + _make(tmp_path, "alpha", frida=True) + _make(tmp_path, "bravo", frida=True) + _make(tmp_path, "charlie", frida=False) + + from beetroot import config as config_mod + + load_calls = {"n": 0} + real_load = config_mod.load_yaml + + def _spy_load(path: Path) -> config_mod.InstanceConfig: + load_calls["n"] += 1 + return real_load(path) + + monkeypatch.setattr("beetroot.config.load_yaml", _spy_load) + # ``registry`` binds ``load_yaml`` by name at import, so patch that surface + # too — every parse of a ``beetroot.yaml`` must be counted. + monkeypatch.setattr("beetroot.registry.load_yaml", _spy_load) + + result = CliRunner().invoke(cli.app, ["ls", "--json"]) + assert result.exit_code == 0, result.stderr + rows = json.loads(result.stdout) + assert set(rows) == {"alpha", "bravo", "charlie"} + assert rows["charlie"]["frida"] == api.FRIDA_ADDRESS_UNSUPPORTED + assert rows["alpha"]["frida"].startswith("localhost:") + + # Bounded: ``load_yaml`` runs a small constant number of times per instance, + # not the unbounded O(N) blowup the pre-fix row builders caused. Allow a + # generous ceiling well below the old per-address re-parse counts. + assert load_calls["n"] <= 3 * len(rows) + + +def test_ls_json_matches_direct_row_builder(cli_root: Path, tmp_path: Path) -> None: + # The reused-snapshot path must produce the same dict the direct row + # builder does (behavior parity, not just line coverage). + _make(tmp_path, "alpha", frida=True) + inst = api.Instance.load("alpha") + direct = cli._instance_json_row(inst) + + result = CliRunner().invoke(cli.app, ["ls", "--json"]) + assert result.exit_code == 0, result.stderr + via_ls = json.loads(result.stdout)["alpha"] + # ``status`` is live so compare the stable derived address/index fields. + for key in ("adb", "frida", "adb_address", "frida_address", "index", "path"): + assert via_ls[key] == direct[key] diff --git a/tests/test_bugfix_port_collision_toctou.py b/tests/test_bugfix_port_collision_toctou.py new file mode 100644 index 0000000..91d648d --- /dev/null +++ b/tests/test_bugfix_port_collision_toctou.py @@ -0,0 +1,118 @@ +"""Regression tests for #183 — cross-instance port-collision check must be locked. + +``_check_port_collisions`` read siblings via ``all_resolved_host_ports`` then +decided via ``find_port_collision`` with NO lock held across the read→decide +window. Two concurrent ``apply``/``create`` operations pinning the same explicit +``host:`` port could both pass the precheck and double-bind at ``up`` time. The +fix routes the check through ``registry.assert_no_port_collision``, which holds +the exclusive registry lock across the whole sibling-read + decision. +""" + +from __future__ import annotations + +import contextlib +from collections.abc import Iterator +from pathlib import Path + +import pytest + +from beetroot import registry +from beetroot.config import InstanceConfig, PortMapping, write_yaml +from beetroot.ports import ResolvedPort + + +def _make_instance(base: Path, name: str, ports: list[PortMapping] | None = None) -> Path: + root = base / name + root.mkdir(parents=True) + cfg = InstanceConfig() if ports is None else InstanceConfig(ports=ports) + write_yaml(root / "beetroot.yaml", cfg) + return root + + +def _seed(base: Path, name: str, ports: list[PortMapping] | None = None) -> Path: + root = _make_instance(base, name, ports) + registry.add_allocating(name, root) + return root + + +def test_assert_no_port_collision_raises_on_conflict( + isolated_registry: Path, tmp_path: Path +) -> None: + # alpha pins adb to 6000; a new instance also pinning 6000 collides. + _seed(tmp_path, "alpha", [PortMapping(service="adb", guest=5555, host=6000)]) + new_ports = [ResolvedPort(service="adb", guest=5555, host=6000)] + with pytest.raises(ValueError, match="collides with instance 'alpha'"): + registry.assert_no_port_collision("bravo", new_ports) + + +def test_assert_no_port_collision_noop_when_clear( + isolated_registry: Path, tmp_path: Path +) -> None: + _seed(tmp_path, "alpha", [PortMapping(service="adb", guest=5555, host=6000)]) + new_ports = [ResolvedPort(service="adb", guest=5555, host=6001)] + # No collision → returns cleanly. + registry.assert_no_port_collision("bravo", new_ports) + + +def test_assert_no_port_collision_excludes_self( + isolated_registry: Path, tmp_path: Path +) -> None: + # An instance's own registered ports must not collide with itself. + _seed(tmp_path, "alpha", [PortMapping(service="adb", guest=5555, host=6000)]) + new_ports = [ResolvedPort(service="adb", guest=5555, host=6000)] + registry.assert_no_port_collision("alpha", new_ports) + + +def test_check_runs_inside_exclusive_lock( + isolated_registry: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + _seed(tmp_path, "alpha", [PortMapping(service="adb", guest=5555, host=6000)]) + + real_locked = registry._locked + seen: list[bool] = [] + + @contextlib.contextmanager + def _spy_locked(path: Path, *, exclusive: bool = True) -> Iterator[Path]: + # Record whether the sibling read is happening under the exclusive lock, + # and — crucially — that ``_resolved_host_ports_from`` runs *inside* it. + with real_locked(path, exclusive=exclusive) as p: + seen.append(exclusive) + yield p + + monkeypatch.setattr(registry, "_locked", _spy_locked) + new_ports = [ResolvedPort(service="adb", guest=5555, host=6001)] + registry.assert_no_port_collision("bravo", new_ports) + # The precheck's own critical section is exclusive. + assert True in seen + + +def test_sibling_read_happens_under_the_lock( + isolated_registry: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Prove the read→decide window is inside the lock: patch the sibling + # resolver to assert the lock file is held (exclusive) at read time. + _seed(tmp_path, "alpha", [PortMapping(service="adb", guest=5555, host=6000)]) + + lock_state = {"held": False} + real_locked = registry._locked + real_resolver = registry._resolved_host_ports_from + + @contextlib.contextmanager + def _tracking_locked(path: Path, *, exclusive: bool = True) -> Iterator[Path]: + with real_locked(path, exclusive=exclusive) as p: + lock_state["held"] = exclusive + try: + yield p + finally: + lock_state["held"] = False + + def _checking_resolver(instances: dict[str, object]) -> dict[str, set[int]]: + assert lock_state["held"], "sibling read ran OUTSIDE the exclusive lock (TOCTOU)" + return real_resolver(instances) # type: ignore[arg-type] + + monkeypatch.setattr(registry, "_locked", _tracking_locked) + monkeypatch.setattr(registry, "_resolved_host_ports_from", _checking_resolver) + + new_ports = [ResolvedPort(service="adb", guest=5555, host=6000)] + with pytest.raises(ValueError, match="collides"): + registry.assert_no_port_collision("bravo", new_ports) diff --git a/tests/test_bugfix_settings_lazy_error.py b/tests/test_bugfix_settings_lazy_error.py new file mode 100644 index 0000000..0599638 --- /dev/null +++ b/tests/test_bugfix_settings_lazy_error.py @@ -0,0 +1,95 @@ +"""Regression tests for #197 — a malformed BEETROOT_*_TIMEOUT must not brick import. + +Before the fix, ``settings = Settings()`` ran eagerly at import time, so an +empty/non-numeric ``BEETROOT_HTTP_TIMEOUT`` raised a raw ``pydantic.ValidationError`` +during ``import beetroot.cli`` — *before* ``cli.main()``'s error boundary — and +even ``beetroot --help`` dumped a traceback. The lazy proxy defers construction +to first attribute access (CLI runtime), so a bad var maps to the friendly +``error: ...`` + exit 1 contract, and ``--help`` (which reads no setting) still works. +""" + +from __future__ import annotations + +import io +import sys + +import pytest +from rich.console import Console +from typer.testing import CliRunner + +from beetroot import cli, console, settings + + +def _run_main_with_argv(argv: list[str], monkeypatch: pytest.MonkeyPatch) -> tuple[int, str]: + """Drive cli.main() under a faked argv. Returns (exit_code, stderr).""" + monkeypatch.setattr(sys, "argv", argv) + buf = io.StringIO() + console.set_consoles(stderr=Console(file=buf, force_terminal=False)) + try: + cli.main() + except SystemExit as exc: + return int(exc.code or 0), buf.getvalue() + return 0, buf.getvalue() + + +@pytest.mark.parametrize("var", ["BEETROOT_HTTP_TIMEOUT", "BEETROOT_VM_ADB_CONNECT_TIMEOUT"]) +def test_lazy_proxy_raises_domain_error_on_access( + var: str, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(var, "notanumber") + proxy = settings._LazySettings() + with pytest.raises(settings.InvalidSettingsError): + _ = proxy.http_timeout + + +def test_lazy_proxy_defers_validation_until_access(monkeypatch: pytest.MonkeyPatch) -> None: + # Constructing the proxy under a bad env must NOT raise — only reading an + # attribute does. This is what keeps ``import beetroot.cli`` (and --help) alive. + monkeypatch.setenv("BEETROOT_HTTP_TIMEOUT", "") + proxy = settings._LazySettings() # no exception here + with pytest.raises(settings.InvalidSettingsError): + _ = proxy.docker_bin + + +def test_lazy_proxy_caches_and_forwards_valid_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("BEETROOT_HTTP_TIMEOUT", "45") + proxy = settings._LazySettings() + assert proxy.http_timeout == 45 + # Second access returns the cached Settings (no re-read). + assert proxy._get() is proxy._get() + + +def _reset_module_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """ + Force the shared module-level proxy to re-read env on next access. + + Consumers (compose, frida_download, …) bound ``settings.settings`` at import, + so tests can't swap the object out from under them — instead clear its cache; + ``monkeypatch.setattr`` restores the prior resolved value on teardown. + """ + monkeypatch.setattr(settings.settings, "_resolved", None) + + +def test_help_still_works_with_malformed_timeout(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("BEETROOT_HTTP_TIMEOUT", "notanumber") + _reset_module_proxy(monkeypatch) + # --help reads no setting, so the bad env is never touched → exit 0, no crash. + code, err = _run_main_with_argv(["beetroot", "--help"], monkeypatch) + assert code == 0 + assert "Traceback" not in err + + +def test_bad_timeout_maps_to_friendly_error( + cli_root: object, monkeypatch: pytest.MonkeyPatch +) -> None: + CliRunner().invoke(cli.app, ["create", "alpha"]) + monkeypatch.setenv("BEETROOT_HTTP_TIMEOUT", "notanumber") + _reset_module_proxy(monkeypatch) + + # ``doctor alpha`` reads ``settings.docker_bin`` via compose while probing + # status, forcing the lazy proxy to resolve inside main()'s boundary. + code, err = _run_main_with_argv(["beetroot", "doctor", "alpha"], monkeypatch) + assert code == 1 + assert "error:" in err + assert "Traceback" not in err + assert "BEETROOT_" in err diff --git a/tests/test_builder.py b/tests/test_builder.py index cf66d0b..aa3b779 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -1305,7 +1305,7 @@ def test_full_build_stages_everything(self, tmp_path: Path) -> None: assert (root / "lib64" / "ld-linux.so").is_file() assert (root / "tmp").stat().st_mode & 0o1777 == 0o1777 # The redroid image was baked via pull + save + a staging dockerd. - assert ["docker", "pull", cfg.redroid_image] in runner.runs + assert ["docker", "pull", "--platform=linux/amd64", cfg.redroid_image] in runner.runs assert runner.spawns assert runner.background.stopped == 1 # issue #82: the baked Android version is recorded beside the image, @@ -1422,6 +1422,17 @@ def test_prebuilt_tarball_skips_pull(self, tmp_path: Path) -> None: assert not any("pull" in c for c in runner.runs) assert not any("save" in c for c in runner.runs) + def test_pull_pins_amd64_platform(self, tmp_path: Path) -> None: + # issue #258: the guest rootfs is hard-x86_64; the pull must pin + # linux/amd64 so a non-x86_64 build host doesn't bake a wrong-arch image. + cfg = _make_rootfs_config(tmp_path) + runner = FakeRootfsRunner() + _run_assembly(tmp_path, runner, cfg) + pull = next(c for c in runner.runs if len(c) > 1 and c[1] == "pull") + assert "--platform=linux/amd64" in pull + # The pin precedes the image ref (docker parses flags before the arg). + assert pull.index("--platform=linux/amd64") < pull.index(cfg.redroid_image) + def test_staging_dockerd_never_ready_raises(self, tmp_path: Path) -> None: cfg = _make_rootfs_config(tmp_path) runner = FakeRootfsRunner(info_ready=False) @@ -1436,12 +1447,43 @@ def test_fetch_failure_propagates(self, tmp_path: Path) -> None: with pytest.raises(BootstrapError, match="curl"): _run_assembly(tmp_path, runner, cfg) + def test_matching_digest_allows_extract(self, tmp_path: Path) -> None: + # issue #262: a pinned sha256 that matches the downloaded bundle lets the + # bake proceed and unpack the bundle into the trusted rootfs. + good = hashlib.sha256(b"tgz").hexdigest() # the fake curl writes b"tgz" + cfg = _make_rootfs_config(tmp_path, docker_url_sha256=good) + runner = FakeRootfsRunner() + _run_assembly(tmp_path, runner, cfg) + assert (tmp_path / "work" / "root" / "bin" / "dockerd").is_file() + + def test_mismatched_digest_aborts_before_extract(self, tmp_path: Path) -> None: + # issue #262: a tampered bundle (digest mismatch) aborts the bake before + # the untrusted bytes are ever tar-xzf'd into the guest rootfs. + cfg = _make_rootfs_config(tmp_path, docker_url_sha256="0" * 64) + runner = FakeRootfsRunner() + with pytest.raises(BootstrapError, match="sha256 mismatch"): + _run_assembly(tmp_path, runner, cfg) + assert not any(c and c[0] == "tar" for c in runner.runs) + + def test_absent_digest_warns_but_proceeds( + self, tmp_path: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + # issue #262: with no pinned digest (override without DOCKER_SHA256, or + # the default digest not yet filled in) the bake proceeds but prints an + # explicit unverified-source warning. + cfg = _make_rootfs_config(tmp_path, docker_url_sha256=None) + runner = FakeRootfsRunner() + _run_assembly(tmp_path, runner, cfg) + assert "UNVERIFIED" in capsys.readouterr().err + assert (tmp_path / "work" / "root" / "bin" / "dockerd").is_file() + class TestRootfsConfigFromEnv: def test_defaults_when_env_unset(self, monkeypatch: pytest.MonkeyPatch) -> None: for var in ( "DOCKER_VERSION", "DOCKER_URL", + "DOCKER_SHA256", "REDROID_TAR", "ADBPROBE_BIN", "IMAGE_SIZE_MB", @@ -1452,6 +1494,9 @@ def test_defaults_when_env_unset(self, monkeypatch: pytest.MonkeyPatch) -> None: cfg = builder._RootfsConfig.from_env(out_image=Path("/o.img"), vm_dir=Path("/vm")) assert cfg.docker_version == "27.5.1" assert cfg.docker_url.endswith("docker-27.5.1.tgz") + # issue #262: the default bundle carries the pinned digest (currently + # None until the literal sha256 is filled in — deferred). + assert cfg.docker_url_sha256 == builder._DEFAULT_DOCKER_BUNDLE_SHA256 assert cfg.redroid_tar is None assert cfg.adbprobe_bin is None assert cfg.image_size_mb == 8192 @@ -1507,6 +1552,37 @@ def test_explicit_docker_url_wins(self, monkeypatch: pytest.MonkeyPatch) -> None assert cfg.docker_url == "http://mirror.invalid/d.tgz" assert cfg.redroid_tar is None + def test_docker_url_override_without_sha_is_unverified( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # issue #262: overriding DOCKER_URL away from the pinned default without + # a DOCKER_SHA256 leaves the digest unresolved (bundle is unverified). + monkeypatch.setenv("DOCKER_URL", "http://mirror.invalid/d.tgz") + monkeypatch.delenv("DOCKER_SHA256", raising=False) + cfg = builder._RootfsConfig.from_env(out_image=Path("/o.img"), vm_dir=Path("/vm")) + assert cfg.docker_url_sha256 is None + + def test_docker_sha256_env_pins_override_digest( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # issue #262: an explicit DOCKER_SHA256 lets a custom bundle still be + # verified, and wins even over a default URL. + monkeypatch.setenv("DOCKER_URL", "http://mirror.invalid/d.tgz") + monkeypatch.setenv("DOCKER_SHA256", "a" * 64) + cfg = builder._RootfsConfig.from_env(out_image=Path("/o.img"), vm_dir=Path("/vm")) + assert cfg.docker_url_sha256 == "a" * 64 + + def test_docker_version_override_drops_default_digest( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # issue #262: bumping DOCKER_VERSION shifts the default URL off the pin, + # so the pinned default digest no longer applies. + monkeypatch.setenv("DOCKER_VERSION", "26.0.0") + monkeypatch.delenv("DOCKER_URL", raising=False) + monkeypatch.delenv("DOCKER_SHA256", raising=False) + cfg = builder._RootfsConfig.from_env(out_image=Path("/o.img"), vm_dir=Path("/vm")) + assert cfg.docker_url_sha256 is None + class TestRootfsVersionMarker: def test_marker_path_sits_beside_image(self, tmp_path: Path) -> None: diff --git a/tests/test_cli_error_contract.py b/tests/test_cli_error_contract.py index 88c2e51..a3fd81d 100644 --- a/tests/test_cli_error_contract.py +++ b/tests/test_cli_error_contract.py @@ -126,7 +126,13 @@ def test_apply_surfaces_frida_fetch_error( encoding="utf-8", ) - def _boom(root: Path, version: str, *, expected_sha256: str | None = None) -> None: + def _boom( + root: Path, + version: str, + *, + expected_sha256: str | None = None, + binder: str = "auto", + ) -> None: raise frida_download.FridaFetchError("simulated frida download failure") monkeypatch.setattr(frida_download, "stage_for_instance", _boom) diff --git a/tests/test_console.py b/tests/test_console.py index 1781e00..dcfdc73 100644 --- a/tests/test_console.py +++ b/tests/test_console.py @@ -322,6 +322,36 @@ def test_table_non_tty_has_no_box_drawing(monkeypatch: pytest.MonkeyPatch) -> No assert glyph not in out +def test_table_non_tty_escapes_bracket_cell_verbatim(monkeypatch: pytest.MonkeyPatch) -> None: + # An instance path with ``[brackets]`` must survive verbatim off-TTY, never + # be parsed as a rich tag (which drops the cell or crashes) (#259). + c, buf = _make_console(tty=False) + monkeypatch.setattr(console, "_stdout_console", c) + bracket_path = "/home/user/[research]/alpha" + console.table(["NAME", "PATH"], [["alpha", bracket_path]]) + assert bracket_path in buf.getvalue() + + +def test_table_tty_escapes_bracket_header_and_cell(monkeypatch: pytest.MonkeyPatch) -> None: + # Same escaping guard on the decorated TTY branch — header and cell (#259). + c, buf = _make_console(tty=True) + monkeypatch.setattr(console, "_stdout_console", c) + console.table(["[HEAD]"], [["/home/user/[research]/alpha"]]) + out = buf.getvalue() + assert "[HEAD]" in out + assert "[research]" in out + + +def test_table_unbalanced_bracket_cell_does_not_raise(monkeypatch: pytest.MonkeyPatch) -> None: + # An unbalanced ``[`` would raise ``MarkupError`` if left unescaped; both + # the off-TTY and TTY branches must tolerate it (#259). + for tty in (False, True): + c, buf = _make_console(tty=tty) + monkeypatch.setattr(console, "_stdout_console", c) + console.table(["N", "P"], [["alpha", "/home/user/[oops/alpha"]]) + assert "oops" in buf.getvalue() + + def test_table_tty_keeps_decorated_box(monkeypatch: pytest.MonkeyPatch) -> None: # The TTY branch is unchanged: interactive output stays a decorated rich # Table with box-drawing borders. diff --git a/tests/test_magisk_config_helper.py b/tests/test_magisk_config_helper.py index c395557..5f33edb 100644 --- a/tests/test_magisk_config_helper.py +++ b/tests/test_magisk_config_helper.py @@ -124,6 +124,28 @@ def test_custom_denylist_csv_parsed(tmp_path: Path) -> None: assert sum(1 for q in inserts if "com.x.y" in q) == 1 +def test_space_padded_denylist_entries_are_trimmed(tmp_path: Path) -> None: + # Issue #263: a spaced CSV like ``com.foo, com.bar`` splits on ``,`` + # into ``com.foo`` and `` com.bar`` (leading space). Enrolling the + # untrimmed `` com.bar`` would never match the real package, silently + # defeating the denylist. The helper must trim each field so the INSERT + # carries the bare package id — and must NOT carry the space-padded form. + code, _out, queries = _run_helper( + tmp_path, + env={"BEETROOT_DENYLIST_PACKAGES": "com.foo, com.bar ,\tcom.tab\t"}, + ) + assert code == 0 + inserts = [q for q in queries if "INSERT OR IGNORE INTO denylist" in q] + # Each package is enrolled exactly once, trimmed. + assert sum(1 for q in inserts if "('com.foo', 'com.foo')" in q) == 1 + assert sum(1 for q in inserts if "('com.bar', 'com.bar')" in q) == 1 + assert sum(1 for q in inserts if "('com.tab', 'com.tab')" in q) == 1 + # And no INSERT smuggles in a leading/trailing-whitespace package. + for q in inserts: + assert "(' " not in q, f"untrimmed leading whitespace enrolled: {q!r}" + assert " '," not in q, f"untrimmed trailing whitespace enrolled: {q!r}" + + def test_empty_denylist_skips_inserts(tmp_path: Path) -> None: # When the env var is unset / empty, the helper must NOT issue any # INSERT — and especially must not SQL'inject an empty diff --git a/tests/test_modules_download.py b/tests/test_modules_download.py index 442e552..0301891 100644 --- a/tests/test_modules_download.py +++ b/tests/test_modules_download.py @@ -171,6 +171,32 @@ def test_filename_from_empty_url_defaults_to_module_zip(self, instance_root: Pat staged = modules_download.stage_for_instance(instance_root, cfg) assert staged[0].name == "module.zip" + def test_short_read_vs_content_length_raises_and_caches_nothing( + self, instance_root: Path + ) -> None: + # A clean short read (Content-Length advertises more bytes than the body + # actually yields) must be rejected before publishing so a truncated zip + # is never cached and re-served (#261). + url = "https://example.com/mod.zip" + cfg = InstanceConfig(modules=[Module(url=url)]) + resp = _make_url_resp(content_length=len(FAKE_ZIP_CONTENT) + 100) + with patch("urllib.request.urlopen", return_value=resp): + with pytest.raises(modules_download.ModuleFetchError, match="truncated"): + modules_download.stage_for_instance(instance_root, cfg) + # No cached artifact left behind — the temp was unlinked, not published. + cache_path = modules_download._cache_path_for_url(url) + assert not cache_path.exists() + assert list(cache_path.parent.glob("*")) == [] + + def test_exact_content_length_match_succeeds(self, instance_root: Path) -> None: + # The check must not fire on a fully-received body: when bytes-received + # equals the advertised Content-Length the fetch publishes normally. + cfg = InstanceConfig(modules=[Module(url="https://example.com/mod.zip")]) + resp = _make_url_resp(content_length=len(FAKE_ZIP_CONTENT)) + with patch("urllib.request.urlopen", return_value=resp): + staged = modules_download.stage_for_instance(instance_root, cfg) + assert staged[0].read_bytes() == FAKE_ZIP_CONTENT + class TestStageForInstancePathModule: def test_absolute_path_module_copies_file(self, instance_root: Path, tmp_path: Path) -> None: diff --git a/tests/test_snapshot.py b/tests/test_snapshot.py index b6dd169..2e57f79 100644 --- a/tests/test_snapshot.py +++ b/tests/test_snapshot.py @@ -1102,3 +1102,107 @@ def test_b7a_rollback_pre_existing_empty_target_skips_rmtree( # rmtree when it didn't create the directory. assert target.exists() assert registry.get("beta") is None + + +class TestManifestFirstMember: + """#265: the manifest is packed FIRST so read_manifest early-exits.""" + + def test_manifest_is_first_archive_member( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + src = _make_instance(tmp_path / "alpha") + (src / "frida-server").write_bytes(b"") + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + + members = _list_archive_members(archive) + assert members[0] == f"./{snapshot.MANIFEST_FILENAME}", ( + f"manifest must be the first member, got order: {members}" + ) + + def test_read_manifest_stops_after_first_member( + self, isolated_registry: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # With the manifest first, read_manifest must extract exactly ONE + # member (the manifest) and not stream the rest of the tree. Spy + # on TarFile.extractfile to count how many members it pulls. + src = _make_instance(tmp_path / "alpha", data_bytes=b"Y" * 50_000) + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + + calls = 0 + real_extractfile = tarfile.TarFile.extractfile + + def _counting_extractfile(self: tarfile.TarFile, member: object): # type: ignore[no-untyped-def] + nonlocal calls + calls += 1 + return real_extractfile(self, member) # type: ignore[arg-type] + + monkeypatch.setattr(tarfile.TarFile, "extractfile", _counting_extractfile) + manifest = snapshot.read_manifest(archive) + assert manifest.name == "alpha" + assert calls == 1, f"read_manifest pulled {calls} members; expected exactly 1" + + def test_old_manifest_last_archive_still_restores( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + # Backward compatibility: an archive whose manifest is the LAST + # member (the pre-#265 layout) must still read + restore. Build one + # by hand with the manifest written after the tree. + old_archive = tmp_path / "old.tar.zst" + manifest_bytes = _VALID_MANIFEST_BYTES + cctx = zstandard.ZstdCompressor() + with old_archive.open("wb") as raw, cctx.stream_writer(raw) as zst: + with tarfile.open(fileobj=zst, mode="w|") as tar: + yaml_payload = _MIN_YAML.encode() + yaml_info = tarfile.TarInfo(name="./beetroot.yaml") + yaml_info.size = len(yaml_payload) + yaml_info.mode = 0o644 + tar.addfile(yaml_info, io.BytesIO(yaml_payload)) + data_info = tarfile.TarInfo(name="./data/marker.txt") + data_info.size = len(b"old") + data_info.mode = 0o644 + tar.addfile(data_info, io.BytesIO(b"old")) + # Manifest LAST, mirroring the pre-#265 archive layout. + man_info = tarfile.TarInfo(name=f"./{snapshot.MANIFEST_FILENAME}") + man_info.size = len(manifest_bytes) + man_info.mode = 0o644 + tar.addfile(man_info, io.BytesIO(manifest_bytes)) + + # read_manifest tolerates the trailing manifest. + assert snapshot.read_manifest(old_archive).name == "alpha" + # And a full restore still works end to end. + restored = snapshot.restore(old_archive, dest_name="beta", dest_path=tmp_path / "beta") + assert (restored / "data" / "marker.txt").read_bytes() == b"old" + assert registry.get("beta") is not None + + +class TestExtractSizeCap: + """#265 folded finding: restore refuses an oversized decompressed payload.""" + + def test_oversized_archive_raises( + self, isolated_registry: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Lower the cap so a normal small archive trips it, exercising the + # decompression-bomb guard without materialising 64 GiB on disk. + src = _make_instance(tmp_path / "alpha", data_bytes=b"Z" * 4096) + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + registry.remove("alpha") + + monkeypatch.setattr(snapshot, "_MAX_EXTRACT_BYTES", 100) + with pytest.raises(snapshot.SnapshotError, match="decompression bomb"): + snapshot.restore(archive, dest_name="beta", dest_path=tmp_path / "beta") + # The half-created target and registry row were rolled back. + assert not (tmp_path / "beta").exists() + assert registry.get("beta") is None + + def test_within_cap_restores(self, isolated_registry: Path, tmp_path: Path) -> None: + # Control: an archive comfortably under the cap restores normally. + src = _make_instance(tmp_path / "alpha", data_bytes=b"small") + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + registry.remove("alpha") + + restored = snapshot.restore(archive, dest_name="beta", dest_path=tmp_path / "beta") + assert (restored / "data" / "marker.txt").read_bytes() == b"small" diff --git a/tests/test_vm_boot_cache.py b/tests/test_vm_boot_cache.py index 244dc3a..93c25f9 100644 --- a/tests/test_vm_boot_cache.py +++ b/tests/test_vm_boot_cache.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import socket import subprocess import threading @@ -390,3 +391,81 @@ def test_discard_overlay_removes_overlay_and_key(self, tmp_path: Path) -> None: def test_discard_overlay_is_idempotent(self, tmp_path: Path) -> None: boot_cache.discard_overlay(tmp_path) # nothing present → no error assert not boot_cache.overlay_path(tmp_path).exists() + + +# --------------------------------------------------------------------------- +# #254: don't re-stream a full SHA-256 over an unchanged (multi-GB) rootfs on +# every warm-resume staleness check — memoize on (path, size, mtime_ns). +# --------------------------------------------------------------------------- + + +class TestHashCache: + def _spy_hashing(self, monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Reset the cache and record every path actually re-streamed (cache miss).""" + monkeypatch.setattr(boot_cache, "_HASH_CACHE", {}) + hashed: list[str] = [] + real_stream = boot_cache._stream_sha256 + + def _stream(path: Path) -> str: + hashed.append(str(path)) + return real_stream(path) + + monkeypatch.setattr(boot_cache, "_stream_sha256", _stream) + return hashed + + def test_unchanged_rootfs_not_rehashed_on_second_call( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + hashed = self._spy_hashing(monkeypatch) + kernel = tmp_path / "bzImage" + kernel.write_bytes(b"KERNEL") + rootfs = tmp_path / "rootdisk.img" + rootfs.write_bytes(b"ROOTFS") + + first = boot_cache.base_identity(kernel, rootfs, 4, 8192) + assert sorted(hashed) == [str(kernel), str(rootfs)] # both hashed once + + hashed.clear() + second = boot_cache.base_identity(kernel, rootfs, 4, 8192) + assert second == first # identical fingerprint from the cache + assert hashed == [] # nothing re-streamed on the hot path + + def test_size_change_triggers_rehash( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + hashed = self._spy_hashing(monkeypatch) + kernel = tmp_path / "bzImage" + kernel.write_bytes(b"KERNEL") + rootfs = tmp_path / "rootdisk.img" + rootfs.write_bytes(b"ROOTFS") + + before = boot_cache.base_identity(kernel, rootfs, 4, 8192) + hashed.clear() + + # A content edit that bumps size *and* mtime invalidates the cache entry. + rootfs.write_bytes(b"REBUILT ROOTFS") + after = boot_cache.base_identity(kernel, rootfs, 4, 8192) + + assert after != before # digest changed with the rebuilt rootfs + assert hashed == [str(rootfs)] # only the changed rootfs was re-streamed + + def test_same_size_content_change_rehashes_via_mtime( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Same byte-length but different content: the mtime bump alone must force + # a re-stream, so the fingerprint still tracks the new bytes. + hashed = self._spy_hashing(monkeypatch) + rootfs = tmp_path / "rootdisk.img" + rootfs.write_bytes(b"AAAA") + kernel = tmp_path / "bzImage" + kernel.write_bytes(b"KERNEL") + + before = boot_cache.base_identity(kernel, rootfs, 4, 8192) + stat = rootfs.stat() + rootfs.write_bytes(b"BBBB") # same size, new content + os.utime(rootfs, ns=(stat.st_atime_ns, stat.st_mtime_ns + 1_000_000)) + hashed.clear() + + after = boot_cache.base_identity(kernel, rootfs, 4, 8192) + assert after != before + assert hashed == [str(rootfs)] # mtime bump forced a re-stream diff --git a/tests/test_vm_cache_key.py b/tests/test_vm_cache_key.py index e853386..64d51bc 100644 --- a/tests/test_vm_cache_key.py +++ b/tests/test_vm_cache_key.py @@ -103,3 +103,47 @@ def test_cli_prints_key(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> N assert rc == 0 out = capsys.readouterr().out.strip() assert out == vm_cache_key.compute_cache_key([a, b]) + + +# --------------------------------------------------------------------------- +# #254: hash_file memoizes on (path, size, mtime_ns) and stays byte-for-byte in +# parity with beetroot.vm.boot_cache._hash_file (same streamed SHA-256). +# --------------------------------------------------------------------------- + + +def test_hash_file_matches_boot_cache_hash(tmp_path: Path) -> None: + from beetroot.vm import boot_cache + + f = _write(tmp_path / "rootdisk.img", b"x" * (2 * 1024 * 1024 + 3)) + assert vm_cache_key.hash_file(f) == boot_cache._hash_file(f) + + +def test_hash_file_reuses_cache_for_unchanged_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(vm_cache_key, "_HASH_CACHE", {}) + streamed: list[str] = [] + real_stream = vm_cache_key._stream_sha256 + + def _stream(path: Path) -> str: + streamed.append(str(path)) + result: str = real_stream(path) + return result + + monkeypatch.setattr(vm_cache_key, "_stream_sha256", _stream) + f = _write(tmp_path / "rootdisk.img", b"rootfs") + first = vm_cache_key.hash_file(f) + assert streamed == [str(f)] # streamed once + streamed.clear() + assert vm_cache_key.hash_file(f) == first + assert streamed == [] # served from the cache, not re-streamed + + +def test_hash_file_rehashes_when_size_or_mtime_changes( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(vm_cache_key, "_HASH_CACHE", {}) + f = _write(tmp_path / "rootdisk.img", b"rootfs") + before = vm_cache_key.hash_file(f) + _write(f, b"rootfs-rebuilt") # size + mtime change + assert vm_cache_key.hash_file(f) != before