Repository navigation
Read untrusted storage through one bounded, regular-file-only path - #28
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
FORMATand pack objects used a plainFile::open, so a FIFO hung every helper call;git showbefore their size was checked;Change
src/bounded.rs:open_regularopens withO_NOFOLLOW | O_NONBLOCK, checks withfstatthat the opened handle is a regular file, then clearsO_NONBLOCK.read_regular_boundedrefuses a file whose length is over the limit and reads at mostlimit + 1bytes.open_object, existing-object verification, and carrier checkout chunks and root files.StoredHead { Bytes, NotRegular, Oversized }replaces the raw head bytes. Ordinary reads fail with "storage HEAD is not a valid head: …".recoverclassifies 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.CatFilewrapper overcat-file --batchchecks the object type and size in the header before reading the body. It serves chunk reads,carrier_blob(replacinggit show, so a tree is refused) andread_copy_at, which streams 64 KiB pieces.ls-treeoutput is capped at 8 MiB, and the process is killed if it exceeds that.restore_historical_objecttakesmax_bytesand charges every byte read, including bytes from rejected copies.ValidatingObjectStagein front of both backends' writers refuses to write pastkind.max_bytes(). An oversized manifest or pack therefore fails before the compare-and-swap. Splitting larger objects is not supported.Dependency.
rustix 1.1(featurefs), unix only, for the open flags andfcntl. It was already in the lockfile throughtempfile.libc 0.2is a unix-only dev-dependency forgetrusage. 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.
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 withoutNOFOLLOW|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_budgetwriters_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-e2eeand~/.config/git-e2eedid not exist before or after the suite.Not in this PR
error.is::<io::Error>()handling of a failedls-tree(Opus 3.5) belongs to step 5, the Git process layer.