Unsound reference construction over uninitialized union and padding bytes in `State::drop`

[Manishearth]

Note

This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

Given that zeroization is a bit of "peeking behind the curtain" anyway, I'm not sure if the "fix" here will work, but it's worth filing either way.

The Issue

When the optional zeroize feature is enabled, the Drop implementation for backend::autodetect::State constructs a mutable reference &mut [u8; SIZE] covering the entire memory layout of the struct:

universal-hashes/poly1305/src/backend/autodetect.rs

Lines 97 to 105 in 47a7296

	#[cfg(feature = "zeroize")]
	impl Drop for State {
	fn drop(&mut self) {
	use zeroize::Zeroize;
	const SIZE: usize = core::mem::size_of::<State>();
	let state = unsafe { &mut *(self as *mut State as *mut [u8; SIZE]) };
	state.zeroize();
	}
	}

State is a #[repr(Rust)] struct containing inner: Inner and token: InitToken. Inner is an untagged #[repr(Rust)] union of ManuallyDrop<backend::avx2::State> (~352 bytes) and ManuallyDrop<backend::soft::State> (56 bytes).

When the software fallback variant soft::State is active (e.g., on systems lacking AVX2 support or when AVX2 is bypassed), the upper ~296 bytes of the Inner union are uninitialized memory (MaybeUninit). Furthermore, because State and Inner lack explicit #[repr(C)] layout guarantees, the compiler is permitted to insert uninitialized padding bytes between fields or at the tail of State.

Producing a reference &mut [u8; N] requires every single byte in the referenced memory range to be an initialized, valid u8 value. Constructing &mut [u8; SIZE] over uninitialized union or padding bytes is immediately UB.

Minimal Reproduction (Miri)

Minimal Reproduction (Miri)

MIRIFLAGS="-Zmiri-disable-isolation" CPUFEATURES_AVX2_DISABLE=1

use poly1305::{Poly1305, universal_hash::KeyInit};

fn main() {
    let key = [0u8; 32];
    let _mac = Poly1305::new((&key).into());
}

This doesn't actually trigger miri failures since miri doesn't eagerly check slice validity, unfortunately.

Suggested Fix

Suggested Fix

Perform zeroization using raw pointer operations without forming an intermediate
reference to the memory layout, such as core::ptr::write_bytes(self as *mut State as *mut u8, 0, SIZE). Alternatively, inspect the witness token
(self.token.get()) to determine which union variant is active and explicitly
zeroize the active field ((*self.inner.avx2) or (*self.inner.soft)).

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

Unsafe Rust Review: poly1305 (v0_8)

Overall Safety Assessment

The poly1305 crate (version v0_8) implements the Poly1305 one-time universal hash function and message authentication code (MAC). The crate uses a dual-backend architecture:

1. A portable software backend (src/backend/soft.rs), which is written in 100% safe Rust.
2. A high-performance vectorized backend (src/backend/avx2.rs and src/backend/avx2/helpers.rs) targeting x86/x86_64 CPUs supporting the avx2 instruction set.
3. An autodetection state wrapper (src/backend/autodetect.rs) that uses cpufeatures at runtime to dispatch between the avx2 and soft backends.

Density of Unsafe Code: The density of unsafe code is high in the AVX2 backend and the autodetection wrapper. The crate relies on untagged unions (autodetect::Inner), raw SIMD intrinsics (__m256i, __m128i), raw pointer dereferences, and target-feature sensitive execution.

Architectural Soundness & Proof Obligations:
Applying rigorous unsafe Rust code review proof-obligation principles reveals critical gaps in contract enforcement and formal safety documentation across the crate:

• Complete Absence of Safety Documentation: Across the entire codebase, there is not a single // SAFETY: comment justifying an unsafe operation, nor is there a single /// # Safety docstring defining caller preconditions on unsafe fn declarations.
• Soundness Vulnerability (Critical): The autodetect::State struct contains a real soundness vulnerability in its Drop implementation when the optional zeroize feature is enabled. Creating a mutable reference &mut [u8; SIZE] covering the untagged union Inner and struct padding bytes violates core Rust validity invariants for u8 (uninitialized memory cannot be read as valid u8).
• Encapsulation of Unchecked Preconditions in Safe APIs (Fishy): Multiple helper functions marked safe (backend::avx2::State::new, Aligned130::add/mul, Unreduced130::reduce, IntegerTag::write, etc.) directly execute AVX2 or SSE2 intrinsics inside unsafe blocks without dynamic CPUID verification or target-feature attributes on the methods themselves. Under Rule 7 ("Do not impose hidden safety obligations on safe callers"), module privacy cannot be used to justify hiding unchecked memory-safety preconditions on safe helper functions.

---

Critical Findings

1. Unsound Reference Creation over Uninitialized Union and Padding Bytes in State::drop 🔴 ⚠️

• Priority: 🔴 High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unsound Reference Construction

• Location: src/backend/autodetect.rs:102 (compiled when cfg(feature = "zeroize") is active).

• Code:

  #[cfg(feature = "zeroize")]
  impl Drop for State {
      fn drop(&mut self) {
          use zeroize::Zeroize;
          const SIZE: usize = core::mem::size_of::<State>();
          let state = unsafe { &mut *(self as *mut State as *mut [u8; SIZE]) };
          state.zeroize();
      }
  }

• Description of Soundness Vulnerability:
  State is a default #[repr(Rust)] struct containing inner: Inner and token: InitToken. Inner is a #[repr(Rust)] union of ManuallyDrop<backend::avx2::State> and ManuallyDrop<backend::soft::State>.
  The software variant soft::State has a size of exactly 56 bytes (ten u32 limbs plus four u32 padding words). The vectorized variant avx2::State is significantly larger (~352 bytes). When soft::State is active (e.g., on x86 CPUs lacking AVX2 support or when AVX2 is bypassed), the upper ~296 bytes of Inner are uninitialized memory (MaybeUninit). Furthermore, because State and Inner lack explicit #[repr(C)] or #[repr(transparent)] layout guarantees, the Rust compiler is permitted to insert uninitialized padding bytes between fields or at the tail of State.
  Casting *mut State to *mut [u8; SIZE] and dereferencing it as &mut [u8; SIZE] creates a safe slice reference covering the entire memory layout of State.
  Axiom: According to the Rust Reference (Validity requirements), producing a reference &mut [u8; N] requires every single byte in the referenced memory range to be an initialized, valid u8 value. Uninitialized memory (MaybeUninit<u8>) is not a valid u8.
  Constructing &mut [u8; SIZE] over uninitialized union or padding bytes constitutes immediate Undefined Behavior (constructing an invalid value). Safe caller code can trigger this UB simply by constructing Poly1305 and letting it drop when the crate is built with --features zeroize.

• Recommended Fix:
  Perform zeroization using raw pointer operations without forming an intermediate reference, e.g., core::ptr::write_bytes(self as *mut State as *mut u8, 0, SIZE), or inspect self.token.get() and explicitly zeroize the active union variant (*self.inner.avx2) or (*self.inner.soft).

---

Fishy Findings

1. Hidden Target-Feature Preconditions on Safe Helpers and Trait Impls 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unenforced Precondition

• Locations:

  • src/backend/avx2.rs:53 (State::new)
  • src/backend/avx2/helpers.rs:81, 420, 900, 1599 (Display impls for Aligned130, Unreduced130, Aligned4x130, Unreduced4x130)
  • src/backend/avx2/helpers.rs:202, 215, 252, 446, 557, 1108, 1123, 1344, 1663, 1718, 1806, 1975 (Arithmetic and conversion methods add, mul, reduce, sum, from)

• Description:
  The constructor backend::avx2::State::new is marked safe (pub(crate) fn new(key: &Key)), yet it directly invokes unsafe { prepare_keys(key) } which requires the avx2 target feature. Similarly, numerous SIMD wrapper structs implement safe trait methods (Add::add, Mul::mul, Display::fmt) or safe inherent methods (reduce, sum) that directly invoke AVX, AVX2, or SSE2 SIMD intrinsics inside unsafe blocks. None of these methods carry #[target_feature(enable = "avx2")] attributes or perform dynamic CPUID validation.
  Violation: Rule 7 of the governing standard mandates: "Do not impose hidden safety obligations on safe callers... This rule applies to private helper functions as well. Do not rely on module privacy to hide memory-safety preconditions on a safe function."
  While autodetect.rs currently ensures avx2::State is only constructed when runtime CPUID detection succeeds, relying on implicit "witness token" semantics without documenting the type invariant or marking the constructors unsafe violates architectural boundaries and makes internal maintenance fragile.

2. Missing Out-of-Bounds Validation on Safe Helper IntegerTag::write 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Missing Bounds Check

• Location: src/backend/avx2/helpers.rs:1986

• Description:
  IntegerTag::write(self, tag: &mut [u8]) is a safe helper method that executes _mm_storeu_si128(tag.as_mut_ptr() as *mut _, self.0). It unconditionally writes 16 bytes (128 bits) to tag.as_mut_ptr() without verifying tag.len() >= 16. If called with a slice shorter than 16 bytes, it causes a buffer overflow. Although currently invoked exclusively with a 16-byte GenericArray in avx2.rs, its safe signature violates Rule 7 by exposing an unchecked memory-safety precondition.

---

Missing Safety Comments

Every single unsafe block and unsafe fn declaration in the crate lacks // SAFETY: comments and /// # Safety docstrings. Below is the exhaustive enumeration of missing safety documentation along with rigorous proof obligations.

File: src/backend/autodetect.rs 🔴

1. Line 48: unsafe { (*self.inner.avx2).compute_block(block, partial) }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.avx2` and call to `compute_block`.
     // Required contract: The `avx2` union variant must be active and initialized, and the host CPU must support the `avx2` target feature.
     // Evidence:
     // - `self.token.get()` returns true only if runtime CPUID detection verified `avx2` support during `State::new`.
     // - By `State::new`, when `self.token.get()` is true, `self.inner` was initialized with the `avx2` variant.
     // - No intervening mutation changes `self.token` or `self.inner`.
     // Therefore, accessing `self.inner.avx2` is valid and calling `compute_block` is sound.

2. Line 50: unsafe { (*self.inner.soft).compute_block(block, partial) }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.soft`.
     // Required contract: The `soft` union variant must be active and initialized.
     // Evidence: `self.token.get()` is false, which by `State::new` guarantees `self.inner` was initialized with the `soft` variant.

3. Line 61: unsafe { f.call(&mut *self.inner.avx2) }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.avx2`.
     // Required contract: The `avx2` union variant must be active and initialized.
     // Evidence: `self.token.get()` is true, guaranteeing `self.inner.avx2` is active and initialized.

4. Line 63: unsafe { f.call(&mut *self.inner.soft) }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.soft`.
     // Required contract: The `soft` union variant must be active and initialized.
     // Evidence: `self.token.get()` is false, guaranteeing `self.inner.soft` is active and initialized.

5. Line 71: unsafe { (*self.inner.avx2).finalize() }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.avx2` and call to `finalize`.
     // Required contract: `self.inner.avx2` must be active, and `avx2` target feature must be supported.
     // Evidence: `self.token.get()` is true, proving `avx2` CPU support and union variant liveness.

6. Line 73: unsafe { (*self.inner.soft).finalize_mut() }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.soft`.
     // Required contract: `self.inner.soft` must be active and initialized.
     // Evidence: `self.token.get()` is false, proving `soft` variant liveness.

7. Line 82: avx2: ManuallyDrop::new(unsafe { (*self.inner.avx2).clone() }),

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.avx2` and call to `<avx2::State as Clone>::clone`.
     // Required contract: `self.inner.avx2` must be active.
     // Evidence: `self.token.get()` is true, proving `self.inner.avx2` is active and initialized.

8. Line 86: soft: ManuallyDrop::new(unsafe { (*self.inner.soft).clone() }),

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Union field access `self.inner.soft`.
     // Required contract: `self.inner.soft` must be active.
     // Evidence: `self.token.get()` is false, proving `self.inner.soft` is active and initialized.

9. Line 102: let state = unsafe { &mut *(self as *mut State as *mut [u8; SIZE]) };

   • Missing: // SAFETY: comment. (Note: As documented under Critical Findings, this operation is unsound due to uninitialized padding/union bytes. A valid safety proof cannot be written for this line as-is).

File: src/backend/avx2.rs 🔴

1. Line 55: let (k, r1) = unsafe { prepare_keys(key) };

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `prepare_keys(key)`.
     // Required contract: Host CPU must support `avx2` target feature, and `key` must point to 32 readable bytes.
     // Evidence:
     // - `key` is a `&Key` (reference to `[u8; 32]`), guaranteeing 32 initialized readable bytes.
     // - `State::new` is instantiated exclusively by `autodetect.rs` after runtime verification of `avx2` CPUID availability.

2. Line 73: pub(crate) unsafe fn compute_par_blocks(&mut self, blocks: &ParBlocks)

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

3. Line 82: pub(crate) unsafe fn compute_block(&mut self, block: &Block, partial: bool)

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

4. Line 103: unsafe fn process_blocks(&mut self, blocks: Aligned4x130)

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

5. Line 121: pub(crate) unsafe fn finalize(&mut self) -> Tag

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

6. Line 185: unsafe { self.compute_block(block, false) };

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `self.compute_block(block, false)`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `avx2::State` is constructed only when `avx2` is detected at runtime, acting as a witness token that `avx2` is available for all inherent trait methods.

7. Line 191: unsafe { self.compute_par_blocks(blocks) };

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `self.compute_par_blocks(blocks)`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `avx2::State` existence guarantees runtime `avx2` CPU support.

File: src/backend/avx2/helpers.rs 🔴

1. Line 53: pub(super) unsafe fn prepare_keys(key: &Key) -> (AdditionKey, PrecomputedMultiplier)

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature, and `key` is valid for reads of 32 bytes.
     

2. Line 81: unsafe { _mm256_storeu_si256(v0.as_mut_ptr() as *mut _, self.0); }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: `_mm256_storeu_si256(v0.as_mut_ptr() as *mut _, self.0)`.
     // Required contract: Host CPU must support `avx`, destination pointer must be valid for writes of 32 bytes.
     // Evidence:
     // - `v0` is a local `[u8; 32]` array, providing 32 bytes valid for writes.
     // - `Aligned130` is only instantiated within `avx2::State`, acting as a witness that `avx2` (and thus `avx`) is supported.

3. Line 104: pub(super) unsafe fn from_block(block: &Block) -> Self

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

4. Line 121: pub(super) unsafe fn from_partial_block(block: &Block) -> Self

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature, and that the high bit is correctly set.
     

5. Line 132: unsafe fn new(x: __m256i) -> Self

   • Missing: /// # Safety docstring.

   • Proposed Doc:

     /// # Safety
     ///
     /// The caller must ensure that the host CPU supports the `avx2` target feature.
     

6. Line 202: unsafe { Aligned130(_mm256_add_epi32(self.0, other.0)) }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: `_mm256_add_epi32(self.0, other.0)`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `self: Aligned130` acts as a witness token that `avx2` is available.

7. Line 215: unsafe { ... } (inside From<Aligned130> for PrecomputedMultiplier)

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: AVX2 SIMD permutation and addition intrinsics.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `r: Aligned130` acts as a witness token guaranteeing runtime `avx2` support.

8. Line 252: unsafe { ... } (inside Mul<PrecomputedMultiplier> for Aligned130)

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: AVX2 SIMD multiplication and addition intrinsics.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `self: Aligned130` guarantees runtime `avx2` support.

9. Line 420: unsafe { _mm256_storeu_si256(...); _mm256_storeu_si256(...); } (inside Display for Unreduced130)

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Two unaligned 32-byte AVX vector stores.
     // Required contract: Host CPU must support `avx`, destination buffers must be valid for 32-byte writes.
     // Evidence: `v0` and `v1` are local `[u8; 32]` arrays; `Unreduced130` witness implies `avx` support.

10. Line 446: unsafe { ... } (inside Unreduced130::reduce)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: Calls to `adc`, `red`, and AVX2 intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `Unreduced130` is produced exclusively by operations on `Aligned130`, guaranteeing `avx2` CPU support.

11. Line 466: unsafe fn adc(v1: __m256i, v0: __m256i) -> (__m256i, __m256i)

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

12. Line 507: unsafe fn red(v1: __m256i, v0: __m256i) -> (__m256i, __m256i)

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

13. Line 541: pub(super) unsafe fn from_blocks(src: &[Block; 2]) -> Self

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

14. Line 557: unsafe { ... } (inside Aligned2x130::mul_and_sum)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD multiplication and addition intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `self: Aligned2x130` contains `Aligned130` witness tokens guaranteeing `avx2` CPU support.

15. Line 857: pub(super) unsafe fn new(...) -> (Self, PrecomputedMultiplier) (on SpacedMultiplier4x130)

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

16. Line 900: unsafe { ... } (inside Display for Aligned4x130)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: Three unaligned 32-byte AVX vector stores.
      // Required contract: Host CPU must support `avx`, arrays must be 32 bytes.
      // Evidence: `v0`, `v1`, `v2` are local 32-byte arrays; `Aligned4x130` witness implies `avx` support.

17. Line 967: pub(super) unsafe fn from_blocks(src: &[Block; 4]) -> Self

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

18. Line 978: pub(super) unsafe fn from_par_blocks(src: &ParBlocks) -> Self

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

19. Line 993: unsafe fn from_loaded_blocks(blocks_01: __m256i, blocks_23: __m256i) -> Self

    • Missing: /// # Safety docstring.

    • Proposed Doc:

      /// # Safety
      ///
      /// The caller must ensure that the host CPU supports the `avx2` target feature.
      

20. Line 1108: unsafe { ... } (inside Add for Aligned4x130)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: `_mm256_add_epi32` AVX2 vector additions.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `self: Aligned4x130` guarantees `avx2` CPU support.

21. Line 1123: unsafe { ... } (inside Mul for &Aligned4x130)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD multiplication intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `self: &Aligned4x130` guarantees `avx2` CPU support.

22. Line 1344: unsafe { ... } (inside Mul for Aligned4x130)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD multiplication intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `self: Aligned4x130` guarantees `avx2` CPU support.

23. Line 1599: unsafe { ... } (inside Display for Unreduced4x130)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: Five unaligned 32-byte AVX vector stores.
      // Required contract: Host CPU must support `avx`, destination buffers must be 32 bytes.
      // Evidence: `v0`..`v4` are local 32-byte arrays; `Unreduced4x130` implies `avx` support.

24. Line 1663: unsafe { ... } (inside Unreduced4x130::reduce)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD shift, mask, multiply, and blend intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `Unreduced4x130` witness implies `avx2` support.

25. Line 1718: unsafe { ... } (inside Unreduced4x130::sum)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD unpack, add, and permute intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `Unreduced4x130` witness implies `avx2` support.

26. Line 1806: unsafe { ... } (inside AdditionKey::add)

    • Missing: // SAFETY: comment. (Inside this block, nested helper functions unsafe fn propagate_carry(x: __m256i) -> __m256i on line 1814 and unsafe fn propagate_carry_32(x: __m256i) -> __m256i on line 1930 also lack /// # Safety docstrings).

    • Proposed Proof:

      // SAFETY:
      // Operation: AVX2 SIMD bitwise and arithmetic intrinsics.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `self: AdditionKey` witness guarantees runtime `avx2` support.

27. Line 1975: unsafe { ... } (inside From<AdditionKey> for IntegerTag)

    • Missing: // SAFETY: comment.

    • Proposed Proof:

      // SAFETY:
      // Operation: `_mm256_permutevar8x32_epi32`.
      // Required contract: Host CPU must support `avx2`.
      // Evidence: `k: AdditionKey` witness guarantees runtime `avx2` support.

28. Line 1987: unsafe { _mm_storeu_si128(tag.as_mut_ptr() as *mut _, self.0); } (inside IntegerTag::write)

    • Missing: // SAFETY: comment.

    • Proposed Proof (assuming required bounds check assert!(tag.len() >= 16) is added):

      // SAFETY:
      // Operation: `_mm_storeu_si128(tag.as_mut_ptr() as *mut _, self.0)`.
      // Required contract: Destination pointer must be valid for writes of 16 bytes (128 bits).
      // Evidence: `assert!(tag.len() >= 16)` guarantees that `tag.as_mut_ptr()` points to at least 16 writeable bytes.

File: src/fuzz.rs 🔴

1. Line 13: unsafe { avx2.compute_block(block, false); }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `avx2.compute_block(block, false)`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: `fuzz_avx2` is only compiled when `target_feature = "avx2"` is enabled at compile time.

2. Line 21: unsafe { avx2.compute_block(&block, true); }

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `avx2.compute_block(&block, true)`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: Compiled under `#[cfg(target_feature = "avx2")]`.

3. Line 32: (_i + 1, unsafe { avx2.clone().finalize() }),

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `finalize()`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: Compiled under `#[cfg(target_feature = "avx2")]`.

4. Line 37: assert_eq!(unsafe { avx2.finalize() }, soft.finalize());

   • Missing: // SAFETY: comment.

   • Proposed Proof:

     // SAFETY:
     // Operation: Call to `finalize()`.
     // Required contract: Host CPU must support `avx2`.
     // Evidence: Compiled under `#[cfg(target_feature = "avx2")]`.

[newpavlov]

Constructing &mut [u8; SIZE] over uninitialized union or padding bytes is immediately UB.

IIRC there is a movement towards not considering it UB in future, but it does not matter since we call "external" code on the constructed reference.

Personally, I would prefer to write the following:

// requires zeroize v1.9
unsafe { core::ptr::write_bytes(self, 0, 1) }
zeroize::black_box(self);

It's cleaner and compiles into a significantly more efficient code, but @tarcieri does not like relying in black_box, so we probably should use zeroize_flat_type.

[Manishearth]

Yeah, I actually learned just today that they decided this is no longer UB. IMO still worth fixing in the common case, but this crate could be special.

And if you decide to not fix this, worth linking to rust-lang/unsafe-code-guidelines#414 somewhere in the code as a citation

[tarcieri]

@newpavlov I don't mind using it in specific Zeroize impls where it makes sense, I just didn't want you to change the default implementations of all of the zeroize traits to use it as the one-and-only defense

[robofinch]

Yeah, I actually learned just today that they decided this is no longer UB. IMO still worth fixing in the common case, but this crate could be special.

I saw that UCG#414 was still open, so I asked Ralf about it on Zulip. “It is opsem consensus but not yet a stable guarantee.” - Ralf

So, I think it’s technically still UB, but it’ll almost certainly be defined behavior soon.