Repository navigation
Remove the last unsafe block from gix-revision and forbid unsafe there - #3150
Sebastian Thiel (Byron) merged 2 commits into
Conversation
…ision` When a rev-spec looks like `git describe` output, like `v1.0-12-g3bee7fb`, `long_describe_prefix()` computes the anchor's reference name. It found its length with `<*const u8>::offset_from()` on two pointers into `name`, which was the only `unsafe` block in `gix-revision`. The rev-spec parser takes arbitrary input and is fuzzed by OSS-Fuzz. Compute the length as the distance between the addresses where `name` starts and the last anchor token ends. That is safe Rust: if it were ever wrong, the bounds-checked slice of `name` would panic instead of causing undefined behaviour. It also drops the `next_back()` call that only existed to find the first token, which made the function about 5ns faster in a release-mode microbenchmark (about 36ns before, 31ns after). Switch the crate from `#![deny(unsafe_code)]` to `#![forbid(unsafe_code)]` so `unsafe` can't come back through a local `#[allow]` or `#[expect]`. Add a case with an empty token in the anchor's name to the existing test. It passes before and after this change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8ec112487
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
- made test more readable (somewhat unrelated to the changes under review) Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6.0 Astra <[email protected]>
27c0cfc to
7d869b0
Compare
daa6500
into
GitoxideLabs:main
AI disclosure: Claude Code, an AI agent, wrote this description, the patch and the test, and is
speaking through the
b84330808account. The commit carries aCo-Authored-Bytrailer for the same reason.Motivation
The account holder asked me to look for
unsafein gitoxide's OSS-Fuzz-fuzzed parsers that can beremoved without a performance cost, as security hardening. This is
also meant for Google's Patch Rewards program, which rewards removing
unsafefrom Rust crates inOSS-Fuzz projects.
gix-revisionwas the smallest case: oneunsafeblock, and safe code is faster there.Change
For rev-specs that look like
git describeoutput, such asv1.0-12-g3bee7fb,long_describe_prefix()computes the length of the anchor's reference name. It did this with
<*const u8>::offset_from()on twopointers into
name. That was the onlyunsafeblock ingix-revision.namestarts and the address where thelast anchor token ends. This is safe Rust. If the computation were ever wrong, the bounds-checked
name[..len]would panic. It could not cause undefined behaviour.next_back()call. It only located the first token foroffset_from().#![deny(unsafe_code)]to#![forbid(unsafe_code)]. Then a local#[allow(unsafe_code)]or#[expect(unsafe_code)]can't bringunsafeback.full_format_parses_hash_portion_as_prefixwithfoo--bar-12-g3bee7fb, an anchor name with anempty token. The new case passes before and after the change.
Verification (by the agent, on Windows,
x86_64-pc-windows-gnu, Rust 1.99.1)cargo test -p gix-revision --features sha1: 116 passed.cargo clippy -p gix-revision --all-targets --features sha1 --no-deps -- -D warnings: no findings.cargo +stable fmt -p gix-revision -- --checkandgit diff --check: clean.aarch64-unknown-linux-gnubecausefaster-hexrunscpuidonx86):
cargo +nightly miri test --target aarch64-unknown-linux-gnu -p gix-revision --features sha1 --test revision -- spec::parse: 94 passed, no undefined behaviour reported.2M pseudo-random inputs and 1M inputs shaped like
<prefix>-<digits>-g<hex><suffix>.the median was 33.7–39.0 ns before and 28.2–32.2 ns after.