Skip to content

Remove the last unsafe block from gix-revision and forbid unsafe there - #3150

Merged
Sebastian Thiel (Byron) merged 2 commits into
GitoxideLabs:mainfrom
b84330808:gix-revision-forbid-unsafe
Nov 3, 2026
Merged

Sebastian Thiel (Byron) merged 2 commits into
GitoxideLabs:mainfrom
b84330808:gix-revision-forbid-unsafe

Conversation

@b84330808

Copy link
Copy Markdown
Contributor

AI disclosure: Claude Code, an AI agent, wrote this description, the patch and the test, and is
speaking through the b84330808 account. The commit carries a Co-Authored-By trailer for the same reason.

Motivation

The account holder asked me to look for unsafe in gitoxide's OSS-Fuzz-fuzzed parsers that can be
removed without a performance cost, as security hardening. This is
also meant for Google's Patch Rewards program, which rewards removing unsafe from Rust crates in
OSS-Fuzz projects. gix-revision was the smallest case: one unsafe block, and safe code is faster there.

Change

For rev-specs that look like git describe output, such as v1.0-12-g3bee7fb, long_describe_prefix()
computes the length of the anchor's reference name. It did this with <*const u8>::offset_from() on two
pointers into name. That was the only unsafe block in gix-revision.

  • Compute the length as the distance between the address where name starts and the address where the
    last 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.
  • Drop the next_back() call. It only located the first token for offset_from().
  • Switch the crate from #![deny(unsafe_code)] to #![forbid(unsafe_code)]. Then a local
    #[allow(unsafe_code)] or #[expect(unsafe_code)] can't bring unsafe back.
  • Extend full_format_parses_hash_portion_as_prefix with foo--bar-12-g3bee7fb, an anchor name with an
    empty 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 -- --check and git diff --check: clean.
  • Miri (nightly 2026-10-23, interpreting aarch64-unknown-linux-gnu because faster-hex runs cpuid on
    x86): cargo +nightly miri test --target aarch64-unknown-linux-gnu -p gix-revision --features sha1 --test revision -- spec::parse: 94 passed, no undefined behaviour reported.
  • A copy of the old and new function bodies, outside the crate, returned the same result for
    2M pseudo-random inputs and 1M inputs shaped like <prefix>-<digits>-g<hex><suffix>.
  • In a release-mode microbenchmark of those copies (3 inputs, 15 samples of 10M calls, six rounds),
    the median was 33.7–39.0 ns before and 28.2–32.2 ns after.

…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]>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T06:43:00.945333Z a8ec112 PR opened
ℹ️ About Codex in GitHub

Your 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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]>
@Byron
Sebastian Thiel (Byron) merged commit daa6500 into GitoxideLabs:main Nov 3, 2026
25 of 32 checks 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.

2 participants