[Rust] Enable unsafe_op_in_unsafe_fn and document the runtime's unsafe operations - #9231
Open
sanil18 wants to merge 1 commit into
Open
[Rust] Enable unsafe_op_in_unsafe_fn and document the runtime's unsafe operations#9231sanil18 wants to merge 1 commit into
sanil18 wants to merge 1 commit into
Conversation
…fe ops The Rust runtime is a zero-copy parser: `Follow::follow` and the primitives beneath it read attacker-controlled buffers through raw pointer operations whose correctness rests on invariants established elsewhere, usually by `Verifier`. Where those reads appear as `unsafe` blocks inside safe functions, the crate already documents them -- 30 blocks with 29 safety comments. The gap is the other side: the 38 `unsafe fn` bodies. On edition 2018 the body of an `unsafe fn` is itself an implicit `unsafe` block, so the unsafe operations inside those functions never had to be acknowledged at all, and none of them carry a safety comment. The compiler cannot distinguish "carefully reasoned about" from "happens to sit inside an `unsafe fn`". Enabling the lint on the crate as it stands makes the compiler count them: 54 errors, none documented. That is the same gap google#8638 closed for generated code ahead of edition 2024. But google#8638 only touched `*_generated.rs` and `idl_gen_rust.cpp`; it never touched the runtime crate the generated code calls into, and added no safety comments. This commit closes it for the runtime. * Enable `#![deny(unsafe_op_in_unsafe_fn)]`, so every unsafe operation must be explicitly acknowledged and new unsafe code cannot silently regress. * Wrap the 54 previously-implicit unsafe operations in explicit `unsafe` blocks, each with a `// SAFETY:` comment naming the invariant that makes it sound and who establishes it. Every `unsafe` block in the crate is now documented. * Fix four safety contracts that were wrong rather than merely missing. Three documented `size_of::<T>()` where the implementation reads `size_of::<T::Scalar>()`: - `read_scalar`, which also documented `>` where the requirement is `>=`. - `read_scalar_at`. - `emplace_scalar`. `EndianScalar` is public and only its associated type is sealed, so an out-of-crate implementation may pair a small `Self` with a wider `Scalar`. A caller honouring the documented bound then reads out of bounds: with `Self` one byte and `Scalar` eight, Miri reports an 8-byte read from a 2-byte allocation. - `emplace_scalar_array` documented `s.len() >= size_of::<[T; N]>()`, naming a parameter that does not exist and ignoring `loc` entirely. The write starts at `loc`, so a caller honouring the documented bound could still write out of bounds. Now `buf.len() >= loc + size_of::<[T::Scalar; N]>()`. No in-tree caller is affected by any of these: every in-crate `EndianScalar` implementor has `size_of::<T>() == size_of::<T::Scalar>()`, and `flatc` emits offsets that fit the struct. They are latent contract defects rather than live bugs, but they are what external `unsafe` callers are told to rely on. * Fix a copy-pasted `debug_assert!` message in `read_scalar` that reported itself as `emplace_scalar`. No functional change. `unsafe` blocks and comments carry no runtime semantics, and this was verified rather than assumed: compiling `monster_example` with `--emit asm` before and after yields an identical instruction stream across 9,153 instruction lines, with zero differing instruction lines. The two `.s` files are not byte-identical -- three embedded panic-location records differ, because their line numbers shift when lines are added.
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
1 participant
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.
The gap
On edition 2018 the body of an
unsafe fnis itself an implicitunsafeblock, sounsafe operations inside one never have to be acknowledged. The runtime has 38 such
bodies. Enabling the lint makes the compiler count what is sitting in them:
54 unsafe operations, none carrying a safety comment — the compiler cannot tell
"carefully reasoned about" from "happens to sit inside an
unsafe fn".#8638 closed exactly this for generated code ahead of edition 2024, but it only
touched
*_generated.rsandidl_gen_rust.cpp— never the runtime crate thatgenerated code calls into. This closes it there.
The change
#![deny(unsafe_op_in_unsafe_fn)]on the runtime crate, so new unsafe codecannot silently regress.
unsafeblocks, each with a// SAFETY:comment naming the invariant and who establishes it.
debug_assert!inread_scalarthat reported itself asemplace_scalar.The four wrong contracts
read_scalars.len() > size_of::<T>()s.len() >= size_of::<T::Scalar>()read_scalar_ats.len() >= loc + size_of::<T>()loc + size_of::<T::Scalar>()emplace_scalars.len() >= size_of::<T>()size_of::<T::Scalar>()emplace_scalar_arrays.len() >= size_of::<[T; N]>()buf.len() >= loc + size_of::<[T::Scalar; N]>()read_scalaralso said>where the requirement is>=.emplace_scalar_arraynamed a parameter that does not exist and ignored
locentirely, so a callerhonouring the documented bound could still write out of bounds.
EndianScalaris public and only its associated type is sealed, so an out-of-crateimplementation may pair a small
Selfwith a widerScalar. A caller honouring thedocumented bound then reads out of bounds: with
Selfone byte andScalarfour,Miri reports a 4-byte read from a 1-byte allocation.
No in-tree type is affected — every in-crate implementor has
size_of::<T>() == size_of::<T::Scalar>(). These are latent contract defects, butthey are what external
unsafecallers are told to rely on.No functional change
Verified rather than assumed:
monster_examplebuilt with--emit asmbefore andafter gives 9,153 instruction lines each, zero differing. The
.sfiles are notbyte-identical only because three
.ascizpanic-location records shift when linesare added.
Test suite 316 passed / 0 failed. Builds clean for default,
--no-default-featuresand
--features serialize.unsafe_op_in_unsafe_fnstabilised in 1.52; the cratedeclares
rust-version = "1.51", but that is already unreachable on master(
bitflags 2.8requires 1.56), so the effective MSRV does not move.Your call
denyvswarn—denyis what stops backsliding, but it is a one-word change.// SAFETY:vs// Safety:— I used the former becauseclippy::undocumented_unsafe_blocksrecognises it. The crate currently has 28 ofthe latter and 1 of the former; happy to match the majority instead.