Skip to content

Read untrusted storage through one bounded, regular-file-only path - #28

Merged
hmirin merged 1 commit into
mainfrom
fix/bounded-storage-reads
Oct 29, 2026
Merged

hmirin merged 1 commit into
mainfrom
fix/bounded-storage-reads

Conversation

@hmirin

@hmirin hmirin commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Step 4 of the refactoring plan: Opus 1.5, 1.6 and 3.5 (partly); Astra Category 1 mediums on carrier bounds, writer and reader limits, and special files.

Size and file-type checks were applied per call site, and several reads escaped them:

  • FORMAT and pack objects used a plain File::open, so a FIFO hung every helper call;
  • HEAD was checked by path before it was opened, so swapping it in between could still hang;
  • carrier chunks were read whole with git show before their size was checked;
  • historical restore had no byte budget;
  • writers could publish objects larger than readers accept.

Change

  1. One opener. src/bounded.rs:
    • open_regular opens with O_NOFOLLOW | O_NONBLOCK, checks with fstat that the opened handle is a regular file, then clears O_NONBLOCK.
    • read_regular_bounded refuses a file whose length is over the limit and reads at most limit + 1 bytes.
    • Every directory-backend read goes through it: HEAD, FORMAT, open_object, existing-object verification, and carrier checkout chunks and root files.
  2. Invalid HEAD. A typed StoredHead { Bytes, NotRegular, Oversized } replaces the raw head bytes. Ordinary reads fail with "storage HEAD is not a valid head: …". recover classifies such a head as Invalid (InvalidState), so it can be overridden from the floor. Before, an oversized HEAD failed with a budget error and could not be recovered. A FIFO or symlinked HEAD, previously "unavailable", is now Invalid too.
  3. Carrier reads.
    • A CatFile wrapper over cat-file --batch checks the object type and size in the header before reading the body. It serves chunk reads, carrier_blob (replacing git show, so a tree is refused) and read_copy_at, which streams 64 KiB pieces.
    • ls-tree output is capped at 8 MiB, and the process is killed if it exceeds that.
    • restore_historical_object takes max_bytes and charges every byte read, including bytes from rejected copies.
  4. Writers respect readers' limits. Max HEAD bytes, max manifest bytes, chunk size, max chunk count and the derived max object size (128 GiB) are shared constants. A ValidatingObjectStage in front of both backends' writers refuses to write past kind.max_bytes(). An oversized manifest or pack therefore fails before the compare-and-swap. Splitting larger objects is not supported.

Dependency. rustix 1.1 (feature fs), unix only, for the open flags and fcntl. It was already in the lockfile through tempfile. libc 0.2 is a unix-only dev-dependency for getrusage. On Windows the opener does a plain open plus a regular-file check, so symlinks are followed there; this is documented.

Tests

Each test below fails when its change is reverted.

  • Five FIFO and symlink cases, each run with a 5 s timeout: FIFO FORMAT, FIFO pack on open and on put, FIFO HEAD, HEAD symlinked to a FIFO, and HEAD symlinked to a regular file (rejected because symlinks are not followed). All five fail without NOFOLLOW|NONBLOCK.
  • an_oversized_storage_head_is_invalid_and_recoverable_from_the_floor. On 379fd1a it failed with "recovery outer-head byte budget exhausted".
  • tests/carrier_chunk_bounds.rs, in its own binary so the measurement is not shared with other tests. Peak RSS must grow by less than half a chunk while recovery skips a 32 MiB+1 manifest chunk. The old code grew by 35.8 MB.
  • carrier_historical_restore_charges_rejected_copies_against_its_byte_budget
  • writers_refuse_a_manifest_readers_would_reject_before_publication. Nothing is stored and HEAD is unchanged.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --all-targets (183 passed). ~/.local/state/git-e2ee and ~/.config/git-e2ee did not exist before or after the suite.

Not in this PR

  • The restore budget is twice the kind's maximum: 32 MiB for manifests and 256 GiB for packs. The pack descriptor's exact ciphertext size could give a tighter bound.
  • error.is::<io::Error>() handling of a failed ls-tree (Opus 3.5) belongs to step 5, the Git process layer.

Size and file-type checks were applied per call site, so several reads
escaped them: FORMAT and packs used a plain File::open (a FIFO hung every
helper call), HEAD was checked by path before opening (a swap could still
hang), carrier chunks were read whole with git show before their size was
checked, historical restore had no byte budget, and writers could publish
objects larger than readers accept.

- src/bounded.rs opens storage files with O_NOFOLLOW | O_NONBLOCK, checks
  the opened handle is a regular file, and reads small files with a bound.
  Every directory-backend read goes through it.
- An oversized or non-regular HEAD is a typed StoredHead observation:
  ordinary reads report an invalid head and recovery treats it as Invalid,
  instead of failing with a budget error.
- Carrier chunks are read through cat-file --batch with the type and size
  checked from the header before the body; ls-tree output is bounded;
  historical restore charges every byte read, including rejected copies.
- Object size limits are shared constants, and both backends refuse to
  stage a manifest or pack beyond them, before the compare-and-swap.

Adds rustix (unix only) for the open flags.
@hmirin
hmirin merged commit 2720776 into main Oct 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant