Address some feedback - #1
Merged
Merged
Conversation
…t elevating punctuation into a safety issue (Feedback F1) Quoted feedback: > "The safety comment formatting guidelines contradict clippy's lint" > "Clippy requires the :" Documents // SAFETY: as the canonical spelling recognized by Clippy's undocumented_unsafe_blocks lint, while clarifying that capitalization or punctuation differences do not affect the semantic validity of a safety proof.
…-reference uniqueness (Feedback F2) Quoted feedback: > "L550 on temporal scope claims UnsafeCell can relax the rules for *mutable* refs, but that doesn't seem to be correct based on that type's docs" UnsafeCell only relaxes the immutability guarantee of shared references (&T) for its contents; it never relaxes mutable-reference (&mut T) exclusivity.
…bal no-mutation guarantees (Feedback F3) Quoted feedback: > "Around L400 the example doesn't discharge the aliased mutation obligation — the evidence says \"this function doesn't mutate\" but the obligation is \"nobody mutates\" so [reviewer] considers it unfulfilled and I think I agree" Updates the slice::from_raw_parts proof example to show that the caller's safety precondition must guarantee no alias or external code mutates the slice memory for the full returned lifetime 'a.
…rts for ZSTs and zero capacity (Feedback F4) Quoted feedback: > "The example at L593 and the list at L1230 don't consider the 0-sized exception for allocations that means we don't necessarily need a real allocation to have happened" Updates Vec::from_raw_parts contract requirements and reject patterns to distinguish when size_of::<T>() != 0 && cap != 0 (requiring allocator layout match) from ZST or zero-capacity cases (where no real allocation is required).
…view topic (Feedback F5) Quoted feedback: > "[reviewer] also complains that padding isn't mentioned as a thing to review" Adds a padding and object-representation checklist row and transmute review guidance for operations that observe, hash, or serialize raw struct bytes.
…it in the checklist (Feedback F6) Quoted feedback: > "The checklist around L1008-1011 doesn't mention forget / leak. It also seems to say \"forgotten initialized value\" in the non-technical sense, not `forget()` which might be a bit confusing. [reviewer] suggests a caveat around user code being able to panic, forget, etc." Clarifies the Drop and Panic checklist rows so safety does not depend on destructors running (accounting for mem::forget and leak safety) and caller-controlled operations are treated as potential panic and reentrancy points.
…ing new owner (Feedback F7) Quoted feedback: > "[reviewer] says that it's worth mentioning that it's important to disable old owners' destructors before extracting or transferring ownership of raw parts, so that you don't get a panic in between and then double-free" Adds ownership transfer sequencing guidance requiring consuming APIs or disabling the old owner's destructor with ManuallyDrop before constructing a replacement owner to prevent double-free windows during unwinding.
…mporary Pin handles (Feedback F8) Quoted feedback: > "It's also nitpicking on the lifetime entry in the checklist at L1007 by pointing out that Pin obligations persist even after the Pin<&mut T> is dropped" Clarifies that pinning obligations attach to the underlying pointee until its destruction, outliving any temporary Pin<&mut T> wrapper.
…ics, poisoning, and generic wrappers (Feedback F9) Quoted feedback: > "It also has a lot of suggestions for things to add: memory ordering, happens-before, mixed-size atomics, mutex poisoning, variance, PhantomData, drop checking > I don't necessarily think we have to add all of these but it's certainly food for thought" Adds targeted, conditional checklist prompts for memory ordering / happens-before, mixed-size overlapping atomics, advisory mutex poisoning, and generic wrapper semantics (variance, PhantomData, dropck).
…ike Pin::new_unchecked (Feedback F10) Quoted feedback: > "Wow this bit is kind of a spectacular find > L199 says to not rely on safe impls in unsafe proofs. I agree with this — seems reasonable! But [reviewer] says hold on, Pin::new_unchecked is an exception which demands things of the Deref / DerefMut / Drop impls of its owner's generic type > It suggests some more nuanced wording" Explains that when an unsafe API (such as Pin::new_unchecked) explicitly makes safe trait behavior part of its # Safety caller contract, callers discharge that obligation by proving the contract holds for the concrete implementations used.
…MaybeUninit (Feedback F11) Quoted feedback: > "Rule E on L1439 says \"Do not expose partially initialized buffers to caller code\" and [reviewer] complains that this prohibits Vec::spare_capacity_mut() and APIs involving MaybeUninit" Refines Rule E to distinguish unsound exposure of uninitialized storage as initialized [T] / &mut T from sound typed exposure as MaybeUninit<T>.
…ross outer wrappers (Feedback F12) Quoted feedback: > "[reviewer] suggests documenting niches a bit more, including that \"T and UnsafeCell have the same layout\" is not a valid argument that wrapping both in Outer<_> will result in two types with the same layout as well" Documents that inner layout equivalence does not automatically compose through outer wrapper types due to niche suppression (e.g. Option<NonNull<u8>> vs Option<UnsafeCell<NonNull<u8>>>).
… unsafe transmute forgery (Feedback F13) Quoted feedback: > "Rule C around L1379 needs an exception around transmute forgery being assumed out of scope — this is pedantic but not wrong" Clarifies that ordinary safe-client sealing proofs require proving downstream safe code cannot obtain sealing tokens; resisting token forgery via unsafe transmute is out of scope for safe-client soundness.
Manishearth
force-pushed
the
audit-feedback-2
branch
from
August 12, 2026 00:45
4df1c6c to
e3cd1bd
Compare
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
2 participants
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.
@obi1kenobi had some feedback, this addresses it.
cc @joshlf