Repository navigation
Create oid from bytes in one unsafe block in gix-hash - #3157
Merged
Sebastian Thiel (Byron) merged 1 commit intoNov 5, 2026
Merged
Sebastian Thiel (Byron) merged 1 commit into
Sebastian Thiel (Byron) merged 1 commit into
Conversation
`gix-hash` had three copies of the same `unsafe` pointer cast from `&[u8]` to `&oid`: one in `oid::from_bytes()` and one per hash kind in `oid::try_from_bytes()`. None had a `SAFETY` comment. `try_from_bytes()` is used by the tree parser in `gix-object` and by `TryFrom<&[u8]>` for `&oid` and `ObjectId`, so its input can come straight from untrusted data. Let `try_from_bytes()` call `from_bytes()` after its length check, and document why the cast in `from_bytes()` is sound. Now there is a single place that needs reviewing if `oid` ever changes its representation. Mark `from_bytes()` `#[inline]` so callers of the `#[inline]` `try_from_bytes()` in other crates still see the cast. The release-mode assembly of `gix-hash`, `gix-object` and `gix-ref` is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
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.
🟢 Approval recommended
The byte-to-oid conversion cleanup has no active review issues.
No open issues 🎉
What changed in this PR
This PR continues the unsafe-code hardening work from #3150 by consolidating byte-to-oid conversion in gix-hash without changing length validation.
Changes:
- Reuse
from_bytes()for validated SHA-1 and SHA-256 digests, leaving one unsafe cast. - Document the cast’s layout, lifetime, and slice-length safety assumptions.
- Mark
from_bytes()as inline to preserve cross-crate optimization opportunities.
| File | Description |
|---|---|
gix-hash/src/oid.rs |
Centralizes the unsafe conversion, documents its safety, and adds an inline annotation. |
Review effort: Balanced
Sebastian Thiel (Byron)
merged commit Nov 5, 2026
641aca3
into
GitoxideLabs:main
52 of 68 checks passed
Member
|
That's a warning - I rubber-stamped the commit that introduced this, in a rush. While it's not actually unsafe, it's something I wouldn't have allowed either. |
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.
Title: Create
oidfrom bytes in oneunsafeblock ingix-hashAI disclosure: Claude Code, an AI agent, wrote this description and the patch, and is speaking
through the
b84330808account. The commit carries aCo-Authored-Bytrailer for the same reason.Motivation
This follows #3150. The account holder asked me to look for
unsafein gitoxide's parsers that canbe removed or narrowed, as security hardening and for Google's Patch Rewards program. The cast from
&[u8]to&oidcan't be written in safe Rust, butgix-hashhad three copies of it, none with aSAFETYcomment.oid::try_from_bytes()gets its input from the tree parser ingix-object, amongothers.
Change
try_from_bytes()callsfrom_bytes()after its length check instead of repeating the cast foreach hash kind.
gix-hashnow has oneunsafeblock.SAFETYcomment to that block:oidis#[repr(transparent)]over[u8], the cast keepsthe length, and an unexpected length makes
kind()panic but isn't undefined behaviour.from_bytes()#[inline]so callers of the#[inline]try_from_bytes()in other cratesstill see the cast.
Verification (by the agent, on Windows,
x86_64-pc-windows-gnu, Rust 1.99.1)cargo test -p gix-hashwith--features sha1,sha256andsha1,sha256: all passed.cargo clippy -p gix-hash --all-targets --features sha1,sha256 --no-deps -- -D warnings: no findings.cargo +stable fmt -p gix-hash -- --checkandgit diff --check: clean.aarch64-unknown-linux-gnu,-Zmiri-disable-isolationforinsta):cargo +nightly miri test --target aarch64-unknown-linux-gnu -p gix-hash --features sha1,sha256 --test hash -- oid:: object_id:: comparisons:: --skip rfc_3174 --skip shambles: 24 passed,no undefined behaviour reported. The two skipped tests hash megabytes of input, which is too slow
under Miri.
gix-hash,gix-objectandgix-ref(--emit=asm, one codegen unit,ignoring symbol hashes, labels and debug directives) is identical before and after.
I used Claude Code to write this change.