Manishearth commented 2026年06月22日 11:38:19 +02:00

Note

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

In src/rwlock.rs, the implementation of lock_api::RawRwLockUpgrade::try_upgrade for RwLock<(), R> creates a temporary RwLockUpgradableGuard referencing self and calls try_upgrade() on it

335981e742/src/rwlock.rs

If the atomic upgrade fails (for instance, because concurrent shared readers are actively holding the lock), try_upgrade_internal returns Err(tmp_guard). Because the .map(|g| core::mem::forget(g)) combinator is skipped on Err, the temporary tmp_guard is dropped at the end of the statement.

When tmp_guard drops, <RwLockUpgradableGuard as Drop>::drop executes self.inner.lock.fetch_sub(UPGRADED, Ordering::AcqRel), which clears the UPGRADED bit from the atomic lock state. However, RawRwLockUpgrade::try_upgrade returns false to lock_api::RwLockUpgradableReadGuard::try_upgrade, which retains the caller's original upgradable read guard intact.

Consequently, the underlying RwLock state no longer records that an upgradable read guard is active, allowing concurrent threads to immediately acquire new exclusive write locks (RwLockWriteGuard) or new upgradable read locks while the first thread still actively holds shared read access via its upgradable guard. This simultaneous shared (&T) and mutable (&mut T) aliasing violates core Rust borrowing invariants and leads directly to undefined behavior and data races.

Minimal Reproduction (Miri)

uselock_api::RwLockUpgradableReadGuard;usestd::sync::Arc;type RwLock<T>=lock_api::RwLock<spin::rwlock::RwLock<()>,T>;fn main(){letlock=Arc::new(RwLock::new(42));std::thread::scope(|s|{// Acquire a shared read guard first. This prevents the upgrade from succeeding.
letshared=lock.try_read().expect("shared read should succeed");// Acquire the upgradable read guard second.
letupgradable=lock.upgradable_read();// Attempt to upgrade the upgradable read guard.
// Because `shared` is active, try_upgrade fails and returns Err(upgradable).
letupgradable=matchRwLockUpgradableReadGuard::try_upgrade(upgradable){Ok(_)=>panic!("upgrade unexpectedly succeeded"),Err(g)=>g,};// Drop the shared read guard so reader count becomes 0.
drop(shared);// Due to premature release of the UPGRADED bit during try_upgrade failure,
// another thread can now acquire an exclusive write lock while `upgradable` is still held.
s.spawn(||{letmutwriter=lock.try_write().expect("try_write should succeed!");*writer=std::hint::black_box(999);});// Reading `*upgradable` while the concurrent thread mutates protected data triggers a data race UB.
let_val=std::hint::black_box(*upgradable);// Prevent the original guard's destructor from running.
// Since the UPGRADED bit was already cleared by the bug, letting the destructor run
// would subtract UPGRADED again, causing an underflow and corrupting the lock state.
core::mem::forget(upgradable);});}

error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) retag write of type `i32` on thread `unnamed-1` at alloc257+0x18
 --> /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/lock_api-0.4.14/src/rwlock.rs:1823:18
 |
1823 | unsafe { &mut *self.rwlock.data.get() }
 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (2) just happened here
 |
help: and (1) occurred earlier here
 --> src/bin/repro1.rs:34:41
 |
 34 | let _val = std::hint::black_box(*upgradable);
 | 

Suggested Fix

When tmp_guard.try_upgrade() returns Err(guard), guard must be explicitly forgotten via core::mem::forget(guard) so its Drop implementation does not execute and release the UPGRADED flag:

#[inline(always)]unsafefn try_upgrade(&self)-> bool {lettmp_guard=RwLockUpgradableGuard{inner: self,data: &(),phantom: PhantomData,};matchtmp_guard.try_upgrade(){Ok(g)=>{core::mem::forget(g);true}Err(g)=>{core::mem::forget(g);false}}}

---

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: spin (v0_10)

Overall Safety Assessment

spin (v0_10) provides spin-based synchronization primitives (Mutex, RwLock, Once, Lazy, and Barrier) intended as no_std-compatible analogs to standard library synchronization types. The architecture relies heavily on interior mutability via UnsafeCell, atomic state transitions (AtomicBool, AtomicUsize, AtomicU8), and manual RAII guard lifecycle management.

Overall, the core spinlock and mutex primitives (SpinMutex, TicketMutex, FairMutex, Once) demonstrate solid mechanical design with careful attention to memory ordering (such as pairing Acquire loads with Release stores to guarantee synchronization of protected data). However, the audit revealed a significant density of missing safety comments across internal unsafe blocks, particularly within RAII guard pointer dereferences and feature-flagged lock_api trait implementations. Most critically, a subtle soundness vulnerability exists in the lock_api::RawRwLockUpgrade implementation for RwLock, where dropping temporary error guards prematurely clears atomic lock flags while the caller still retains shared upgradable access.

Critical Findings

Soundness Vulnerability: Lock State Premature Release in lock_api::RawRwLockUpgrade::try_upgrade 🔴 🤦

• Severity: 🔴 High

• Threat Vector: 🤦 Accidental Misuse

• Bug Type: Premature Lock Release

• Location: src/rwlock.rs:909 (unsafe impl<R: RelaxStrategy> lock_api_crate::RawRwLockUpgrade for RwLock<(), R>)

• Mechanism:
  The lock_api::RawRwLockUpgrade::try_upgrade(&self) -> bool contract requires attempting a non-blocking atomic upgrade from an upgradable shared state to an exclusive state. If the upgrade fails (e.g., due to active shared readers), the function must return false while leaving the lock in its original upgradable shared state.
  In spin's implementation:

  #[inline(always)]unsafefn try_upgrade(&self)-> bool {lettmp_guard=RwLockUpgradableGuard{inner: self,data: &(),phantom: PhantomData,};tmp_guard.try_upgrade().map(|g|core::mem::forget(g)).is_ok()}

  Here, try_upgrade synthesizes a temporary tmp_guard representing the caller's held upgradable guard. It invokes RwLockUpgradableGuard::try_upgrade(tmp_guard), which executes a compare-and-swap (CAS) via compare_exchange(&self.inner.lock, UPGRADED, WRITER, ...).
  If concurrent shared readers hold the lock (meaning the atomic state contains UPGRADED | N * READER), this CAS fails. On failure, try_upgrade_internal returns Err(tmp_guard).
  The .map(|g| core::mem::forget(g)) combinator is skipped on Result::Err, .is_ok() evaluates to false, and the temporary Result::Err(tmp_guard) is dropped at the end of the statement.
  When tmp_guard is dropped, <RwLockUpgradableGuard as Drop>::drop executes:

  self.inner.lock.fetch_sub(UPGRADED,Ordering::AcqRel);

  This subtracts UPGRADED from the atomic state, clearing the upgradable lock flag. However, RawRwLockUpgrade::try_upgrade returns false to lock_api::RwLockUpgradableReadGuard::try_upgrade, which retains the caller's original RAII guard.
  Consequently, another thread can immediately acquire a new RwLockUpgradableGuard or exclusive RwLockWriteGuard while the first thread still holds active shared read access via its upgradable guard. This simultaneous shared and mutable aliasing violates Rust's core memory safety guarantees, resulting in data races and undefined behavior.

• Proof Obligation / Fix:
  When tmp_guard.try_upgrade() returns Err(guard), guard must be explicitly forgotten via core::mem::forget(guard) so its Drop implementation does not run and release the UPGRADED flag:

  #[inline(always)]unsafefn try_upgrade(&self)-> bool {lettmp_guard=RwLockUpgradableGuard{inner: self,data: &(),phantom: PhantomData,};matchtmp_guard.try_upgrade(){Ok(g)=>{core::mem::forget(g);true}Err(g)=>{core::mem::forget(g);false}}}

Fishy Findings

1. Dead Feature-Flagged Trait Implementation 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Dead Feature Flag
• Location: src/rwlock.rs:934
• Description: The codebase contains #[cfg(feature = "lock_api1")] unsafe impl lock_api::RawRwLockUpgradeDowngrade for RwLock<()>. The feature flag lock_api1 is not defined in Cargo.toml and cannot be activated. This appears to be either a typo for lock_api or an intentional disabling workaround. If RawRwLockUpgradeDowngrade support is intended when lock_api is enabled, this dead code prevents expected functionality.

2. Omitted Strategy Generic Parameter R in Sync Implementation for Lazy 🟡 🤦

• Severity: 🟡 Low

• Threat Vector: 🤦 Accidental Misuse

• Bug Type: Missing Trait Implementation

• Location: src/lazy.rs:63

• Description: Lazy<T, F, R> defines three type parameters, defaulting R = Spin. The manual synchronization implementation is written as:

  unsafeimpl<T,F: Send>SyncforLazy<T,F>whereOnce<T>: Sync {}

  Because R is omitted from the impl parameters, Sync is implemented solely for Lazy<T, F, Spin>. If a user instantiates Lazy<T, F, CustomRelaxStrategy>, Lazy will not be Sync, despite Once<T, CustomRelaxStrategy> being Sync.

3. Fragile Reference Synthesis in lock_api Guard Unlocking 🟡 🤸

• Severity: 🟡 Low

• Threat Vector: 🤸 Deliberate Contortion

• Bug Type: Fragile Code Pattern

• Location: src/rwlock.rs:842

• Description: In lock_api::RawRwLock::unlock_exclusive, the code unlocks by synthesizing a dummy RAII guard wrapping unit () and immediately dropping it:

  drop(RwLockWriteGuard{inner: self,data: &mut(),phantom: PhantomData});

  While sound for unit T = () due to implicit reference-to-raw-pointer coercion (&mut () -> *mut ()), synthesizing temporary dummy RAII guards to invoke destructor state transitions is fragile compared to invoking direct atomic state helper methods (e.g., self.force_write_unlock()), which Mutex and SpinMutex implement directly.

Missing Safety Comments

The following locations contain unsafe blocks, traits, or functions that lack required // SAFETY: comments proving adherence to safety invariants:

• src/lazy.rs:63: unsafe impl<T, F: Send> Sync for Lazy<T, F> 🟡

  • Proposed comment: // SAFETY: Lazy<T, F> synchronizes initialization of Cell<Option<F>> via Once<T>. Concurrent callers to Deref or force synchronize on Once::call_once, ensuring Option<F>::take and F invocation occur exactly once on a single thread before immutable &T access is shared across threads.

• src/mutex.rs:117-118: unsafe impl Sync / Send for Mutex<T, R> 🔴

  • Proposed comment: // SAFETY: Mutex<T, R> provides mutually exclusive access to T via InnerMutex<T, R>. Across threads, references &Mutex transfer exclusive access (&mut T or T) upon lock acquisition, making T: Send necessary and sufficient for both Send and Sync.

• src/mutex.rs:318: unsafe impl RawMutex for Mutex<(), R> 🔴

  • Proposed comment: // SAFETY: Mutex<(), R> guarantees mutual exclusion via atomic state transitions in InnerMutex, fulfilling the RawMutex contract.

• src/mutex.rs:333: self.force_unlock() inside unsafe fn unlock 🔴

  • Proposed comment: // SAFETY: By the RawMutex safety contract, unlock is contractually guaranteed to only be called when the current thread holds the lock.

• src/mutex/fair.rs:107-108: unsafe impl Sync / Send for FairMutex<T, R> 🟠

  • Proposed comment: // SAFETY: FairMutex<T, R> synchronizes access to UnsafeCell<T> via AtomicUsize. Since concurrent access is mutually exclusive, T: Send ensures safe transfer of T across threads.

• src/mutex/fair.rs:110-111: unsafe impl Sync / Send for FairMutexGuard<'_, T> 🟠

  • Proposed comment: // SAFETY: FairMutexGuard holds unique access (*mut T) to T. Moving across threads transfers exclusive access (requiring T: Send). Sharing &FairMutexGuard across threads allows shared access &T via Deref (requiring T: Sync).

• src/mutex/fair.rs:221: unsafe { &mut *self.data.get() } in lock 🔴

  • Proposed comment: // SAFETY: The thread has acquired the lock bit in self.lock, guaranteeing exclusive access to UnsafeCell<T>.

• src/mutex/fair.rs:280: unsafe { &mut *self.data.get() } in try_lock_starver 🔴

  • Proposed comment: // SAFETY: The atomic compare_exchange(0, LOCKED) succeeded, confirming absence of concurrent lock holders or starvers and securing exclusive access to UnsafeCell<T>.

• src/mutex/fair.rs:347: unsafe { &mut *self.data.get() } in get_mut 🟡

  • Proposed comment: // SAFETY: Exclusive borrow &mut self guarantees absence of concurrent borrows or active lock guards, making mutable dereference of UnsafeCell<T> sound.

• src/mutex/fair.rs:400: unsafe { &mut *this.data } in FairMutexGuard::leak 🟡

  • Proposed comment: // SAFETY: Wrapping guard in ManuallyDrop prevents unlock on drop, permanently locking the mutex and ensuring unique lifetime 'a access.

• src/mutex/fair.rs:420, 427: unsafe { &*self.data } and unsafe { &mut *self.data } in Deref / DerefMut 🟡

  • Proposed comment: // SAFETY: Existence of FairMutexGuard statically guarantees active exclusive lock tenure over self.data.

• src/mutex/fair.rs:553, 568: unsafe impl RawMutex and self.force_unlock() 🔴

  • Proposed comment: // SAFETY: FairMutex<(), R> adheres to mutual exclusion contracts. unlock is contractually called only by the lock holder.

• src/mutex/spin.rs:87-88: unsafe impl Sync / Send for SpinMutex<T, R> 🟠

  • Proposed comment: // SAFETY: SpinMutex<T, R> protects UnsafeCell<T> with an AtomicBool. Mutual exclusion guarantees safe cross-thread transfer of T when T: Send.

• src/mutex/spin.rs:90-91: unsafe impl Sync / Send for SpinMutexGuard<'_, T> 🟠

  • Proposed comment: // SAFETY: SpinMutexGuard represents unique access to T. Cross-thread transfer or sharing requires T: Send and T: Sync respectively.

• src/mutex/spin.rs:241, 261: unsafe { &mut *self.data.get() } in try_lock / try_lock_weak 🔴

  • Proposed comment: // SAFETY: Successful CAS transitioning lock from false to true establishes exclusive access to UnsafeCell<T>.

• src/mutex/spin.rs:285: unsafe { &mut *self.data.get() } in get_mut 🟡

  • Proposed comment: // SAFETY: &mut self statically guarantees unique ownership and absence of active lock guards.

• src/mutex/spin.rs:330: unsafe { &mut *this.data } in SpinMutexGuard::leak 🟡

  • Proposed comment: // SAFETY: ManuallyDrop prevents guard destruction, maintaining lock tenure indefinitely for lifetime 'a.

• src/mutex/spin.rs:350, 357: unsafe { &*self.data } and unsafe { &mut *self.data } in Deref / DerefMut 🟡

  • Proposed comment: // SAFETY: Existence of SpinMutexGuard guarantees active exclusive access to self.data.

• src/mutex/spin.rs:369, 384: unsafe impl RawMutex and self.force_unlock() 🔴

  • Proposed comment: // SAFETY: AtomicBool manipulation guarantees mutual exclusion. RawMutex contract ensures unlock is called only by the lock holder.

• src/mutex/ticket.rs:89-90: unsafe impl Sync / Send for TicketMutex<T, R> 🔴

  • Proposed comment: // SAFETY: Ticket ordering enforces mutually exclusive access to UnsafeCell<T>, requiring T: Send for safe cross-thread transfer.

• src/mutex/ticket.rs:328: unsafe { &mut *data } in TicketMutexGuard::leak 🔴

  • Proposed comment: // SAFETY: Forgetting the guard prevents next_serving from advancing, leaving TicketMutex locked permanently for unique lifetime 'a access.

• src/mutex/ticket.rs:365, 380: unsafe impl RawMutex and self.force_unlock() 🔴

  • Proposed comment: // SAFETY: Ticket sequence matching ensures mutual exclusion. RawMutex contract guarantees unlock is invoked only by the active serving thread.

• src/once.rs:54-55: unsafe impl Sync / Send for Once<T, R> 🟠

  • Proposed comment: // SAFETY: Once<T, R> shares &T across threads once initialized (requiring T: Sync). Moving Once across threads transfers T ownership (requiring T: Send).

• src/once.rs:294: return unsafe { Ok(self.force_get()) } 🔴

  • Proposed comment: // SAFETY: Atomic status was stored as Complete on line 291 and data was written on line 270, satisfying force_get preconditions.

• src/once.rs:334, 404: unsafe { self.force_get() } in poll / get 🟠

  • Proposed comment: // SAFETY: Confirming Status::Complete verifies data initialization, and Acquire ordering ensures visibility of initialized memory.

• src/once.rs:432, 460: unsafe { self.force_get_mut() } and unsafe { self.force_into_inner() } in get_mut / try_into_inner 🔴

  • Proposed comment: // SAFETY: Status::Complete confirms data initialization. Exclusive borrow or ownership of self guarantees unique access.

• src/once.rs:502: core::ptr::drop_in_place(...) in Drop 🔴

  • Proposed comment: // SAFETY: Status::Complete confirms MaybeUninit<T> contains a valid initialized value suitable for in-place dropping.

• src/rwlock.rs:111-121: unsafe impl Send / Sync for RwLock, RwLockWriteGuard, RwLockReadGuard, RwLockUpgradableGuard 🟠

  • Proposed comment: // SAFETY: RwLock enforces shared-read/exclusive-write access over UnsafeCell<T>. Sharing &RwLock across threads shares &T (requiring T: Sync). Guard transfer transfers &mut T or &T across threads (requiring T: Send and T: Sync as appropriate).

• src/rwlock.rs:310, 381, 428: unsafe { &*self.data.get() } and unsafe { &mut *self.data.get() } in try_read, try_write, try_upgradeable_read 🔴

  • Proposed comment: // SAFETY: Atomic lock state checks and CAS operations guarantee shared or exclusive access rights over UnsafeCell<T> before reference formation.

• src/rwlock.rs:452: unsafe { &mut *self.data.get() } in get_mut 🟡

  • Proposed comment: // SAFETY: Exclusive borrow &mut self guarantees absence of active read/write guards.

• src/rwlock.rs:495, 555, 612: Guard Deref and upgrade/downgrade data pointers 🔴

  • Proposed comment: // SAFETY: Guard lifecycle transitions atomically maintain required reader/writer counts to ensure valid shared or exclusive access.

• src/rwlock.rs:672, 705: Data dereference in RwLockWriteGuard::downgrade and downgrade_to_upgradeable 🔴

  • Proposed comment: // SAFETY: Reader or upgraded lock counts are atomically reserved before releasing exclusive write access.

• src/rwlock.rs:821, 874, 924, 935: RawRwLock, RawRwLockUpgrade, RawRwLockDowngrade, RawRwLockUpgradeDowngrade implementations 🔴

  • Proposed comment: // SAFETY: Atomic lock state manipulation adheres to lock_api contracts for shared, exclusive, and upgradable locking.

> [!NOTE] > This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification. In `src/rwlock.rs`, the implementation of `lock_api::RawRwLockUpgrade::try_upgrade` for `RwLock<(), R>` creates a temporary `RwLockUpgradableGuard` referencing `self` and calls `try_upgrade()` on it https://codeberg.org/zesterer/spin/src/commit/335981e742b9ff4a8b850f6203adc6df38311a97/src/rwlock.rs If the atomic upgrade fails (for instance, because concurrent shared readers are actively holding the lock), `try_upgrade_internal` returns `Err(tmp_guard)`. Because the `.map(|g| core::mem::forget(g))` combinator is skipped on `Err`, the temporary `tmp_guard` is dropped at the end of the statement. When `tmp_guard` drops, `<RwLockUpgradableGuard as Drop>::drop` executes `self.inner.lock.fetch_sub(UPGRADED, Ordering::AcqRel)`, which clears the `UPGRADED` bit from the atomic lock state. However, `RawRwLockUpgrade::try_upgrade` returns `false` to `lock_api::RwLockUpgradableReadGuard::try_upgrade`, which retains the caller's original upgradable read guard intact. Consequently, the underlying `RwLock` state no longer records that an upgradable read guard is active, allowing concurrent threads to immediately acquire new exclusive write locks (`RwLockWriteGuard`) or new upgradable read locks while the first thread still actively holds shared read access via its upgradable guard. This simultaneous shared (`&T`) and mutable (`&mut T`) aliasing violates core Rust borrowing invariants and leads directly to undefined behavior and data races. <details><summary>Minimal Reproduction (Miri)</summary> ```rust use lock_api::RwLockUpgradableReadGuard; use std::sync::Arc; type RwLock<T> = lock_api::RwLock<spin::rwlock::RwLock<()>, T>; fn main() { let lock = Arc::new(RwLock::new(42)); std::thread::scope(|s| { // Acquire a shared read guard first. This prevents the upgrade from succeeding. let shared = lock.try_read().expect("shared read should succeed"); // Acquire the upgradable read guard second. let upgradable = lock.upgradable_read(); // Attempt to upgrade the upgradable read guard. // Because `shared` is active, try_upgrade fails and returns Err(upgradable). let upgradable = match RwLockUpgradableReadGuard::try_upgrade(upgradable) { Ok(_) => panic!("upgrade unexpectedly succeeded"), Err(g) => g, }; // Drop the shared read guard so reader count becomes 0. drop(shared); // Due to premature release of the UPGRADED bit during try_upgrade failure, // another thread can now acquire an exclusive write lock while `upgradable` is still held. s.spawn(|| { let mut writer = lock.try_write().expect("try_write should succeed!"); *writer = std::hint::black_box(999); }); // Reading `*upgradable` while the concurrent thread mutates protected data triggers a data race UB. let _val = std::hint::black_box(*upgradable); // Prevent the original guard's destructor from running. // Since the UPGRADED bit was already cleared by the bug, letting the destructor run // would subtract UPGRADED again, causing an underflow and corrupting the lock state. core::mem::forget(upgradable); }); } ``` ```text error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) retag write of type `i32` on thread `unnamed-1` at alloc257+0x18 --> /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/lock_api-0.4.14/src/rwlock.rs:1823:18 | 1823 | unsafe { &mut *self.rwlock.data.get() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (2) just happened here | help: and (1) occurred earlier here --> src/bin/repro1.rs:34:41 | 34 | let _val = std::hint::black_box(*upgradable); | ``` </details> <details><summary>Suggested Fix</summary> When `tmp_guard.try_upgrade()` returns `Err(guard)`, `guard` must be explicitly forgotten via `core::mem::forget(guard)` so its `Drop` implementation does not execute and release the `UPGRADED` flag: ```rust #[inline(always)] unsafe fn try_upgrade(&self) -> bool { let tmp_guard = RwLockUpgradableGuard { inner: self, data: &(), phantom: PhantomData, }; match tmp_guard.try_upgrade() { Ok(g) => { core::mem::forget(g); true } Err(g) => { core::mem::forget(g); false } } } ``` </details> --- > [!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. <details><summary>Full Gemini Codebase Audit Report Appendix</summary> # Unsafe Rust Review: `spin` (`v0_10`) ## Overall Safety Assessment `spin` (`v0_10`) provides spin-based synchronization primitives (`Mutex`, `RwLock`, `Once`, `Lazy`, and `Barrier`) intended as `no_std`-compatible analogs to standard library synchronization types. The architecture relies heavily on interior mutability via `UnsafeCell`, atomic state transitions (`AtomicBool`, `AtomicUsize`, `AtomicU8`), and manual RAII guard lifecycle management. Overall, the core spinlock and mutex primitives (`SpinMutex`, `TicketMutex`, `FairMutex`, `Once`) demonstrate solid mechanical design with careful attention to memory ordering (such as pairing `Acquire` loads with `Release` stores to guarantee synchronization of protected data). However, the audit revealed a significant density of missing safety comments across internal unsafe blocks, particularly within RAII guard pointer dereferences and feature-flagged `lock_api` trait implementations. Most critically, a subtle soundness vulnerability exists in the `lock_api::RawRwLockUpgrade` implementation for `RwLock`, where dropping temporary error guards prematurely clears atomic lock flags while the caller still retains shared upgradable access. ## Critical Findings ### Soundness Vulnerability: Lock State Premature Release in `lock_api::RawRwLockUpgrade::try_upgrade` 🔴 🤦 - **Severity**: 🔴 High - **Threat Vector**: 🤦 Accidental Misuse - **Bug Type**: Premature Lock Release - **Location**: `src/rwlock.rs:909` (`unsafe impl<R: RelaxStrategy> lock_api_crate::RawRwLockUpgrade for RwLock<(), R>`) - **Mechanism**: The `lock_api::RawRwLockUpgrade::try_upgrade(&self) -> bool` contract requires attempting a non-blocking atomic upgrade from an upgradable shared state to an exclusive state. If the upgrade fails (e.g., due to active shared readers), the function must return `false` while leaving the lock in its original upgradable shared state. In `spin`'s implementation: ```rust #[inline(always)] unsafe fn try_upgrade(&self) -> bool { let tmp_guard = RwLockUpgradableGuard { inner: self, data: &(), phantom: PhantomData, }; tmp_guard .try_upgrade() .map(|g| core::mem::forget(g)) .is_ok() } ``` Here, `try_upgrade` synthesizes a temporary `tmp_guard` representing the caller's held upgradable guard. It invokes `RwLockUpgradableGuard::try_upgrade(tmp_guard)`, which executes a compare-and-swap (CAS) via `compare_exchange(&self.inner.lock, UPGRADED, WRITER, ...)`. If concurrent shared readers hold the lock (meaning the atomic state contains `UPGRADED | N * READER`), this CAS fails. On failure, `try_upgrade_internal` returns `Err(tmp_guard)`. The `.map(|g| core::mem::forget(g))` combinator is skipped on `Result::Err`, `.is_ok()` evaluates to `false`, and the temporary `Result::Err(tmp_guard)` is dropped at the end of the statement. When `tmp_guard` is dropped, `<RwLockUpgradableGuard as Drop>::drop` executes: ```rust self.inner.lock.fetch_sub(UPGRADED, Ordering::AcqRel); ``` This subtracts `UPGRADED` from the atomic state, clearing the upgradable lock flag. However, `RawRwLockUpgrade::try_upgrade` returns `false` to `lock_api::RwLockUpgradableReadGuard::try_upgrade`, which retains the caller's original RAII guard. Consequently, another thread can immediately acquire a new `RwLockUpgradableGuard` or exclusive `RwLockWriteGuard` while the first thread still holds active shared read access via its upgradable guard. This simultaneous shared and mutable aliasing violates Rust's core memory safety guarantees, resulting in data races and undefined behavior. - **Proof Obligation / Fix**: When `tmp_guard.try_upgrade()` returns `Err(guard)`, `guard` must be explicitly forgotten via `core::mem::forget(guard)` so its `Drop` implementation does not run and release the `UPGRADED` flag: ```rust #[inline(always)] unsafe fn try_upgrade(&self) -> bool { let tmp_guard = RwLockUpgradableGuard { inner: self, data: &(), phantom: PhantomData, }; match tmp_guard.try_upgrade() { Ok(g) => { core::mem::forget(g); true } Err(g) => { core::mem::forget(g); false } } } ``` ## Fishy Findings ### 1. Dead Feature-Flagged Trait Implementation 🟡 🤦 - **Severity**: 🟡 Low - **Threat Vector**: 🤦 Accidental Misuse - **Bug Type**: Dead Feature Flag - **Location**: `src/rwlock.rs:934` - **Description**: The codebase contains `#[cfg(feature = "lock_api1")] unsafe impl lock_api::RawRwLockUpgradeDowngrade for RwLock<()>`. The feature flag `lock_api1` is not defined in `Cargo.toml` and cannot be activated. This appears to be either a typo for `lock_api` or an intentional disabling workaround. If `RawRwLockUpgradeDowngrade` support is intended when `lock_api` is enabled, this dead code prevents expected functionality. ### 2. Omitted Strategy Generic Parameter `R` in `Sync` Implementation for `Lazy` 🟡 🤦 - **Severity**: 🟡 Low - **Threat Vector**: 🤦 Accidental Misuse - **Bug Type**: Missing Trait Implementation - **Location**: `src/lazy.rs:63` - **Description**: `Lazy<T, F, R>` defines three type parameters, defaulting `R = Spin`. The manual synchronization implementation is written as: ```rust unsafe impl<T, F: Send> Sync for Lazy<T, F> where Once<T>: Sync {} ``` Because `R` is omitted from the `impl` parameters, `Sync` is implemented solely for `Lazy<T, F, Spin>`. If a user instantiates `Lazy<T, F, CustomRelaxStrategy>`, `Lazy` will not be `Sync`, despite `Once<T, CustomRelaxStrategy>` being `Sync`. ### 3. Fragile Reference Synthesis in `lock_api` Guard Unlocking 🟡 🤸 - **Severity**: 🟡 Low - **Threat Vector**: 🤸 Deliberate Contortion - **Bug Type**: Fragile Code Pattern - **Location**: `src/rwlock.rs:842` - **Description**: In `lock_api::RawRwLock::unlock_exclusive`, the code unlocks by synthesizing a dummy RAII guard wrapping unit `()` and immediately dropping it: ```rust drop(RwLockWriteGuard { inner: self, data: &mut (), phantom: PhantomData }); ``` While sound for unit `T = ()` due to implicit reference-to-raw-pointer coercion (`&mut () -> *mut ()`), synthesizing temporary dummy RAII guards to invoke destructor state transitions is fragile compared to invoking direct atomic state helper methods (e.g., `self.force_write_unlock()`), which `Mutex` and `SpinMutex` implement directly. ## Missing Safety Comments The following locations contain `unsafe` blocks, traits, or functions that lack required `// SAFETY:` comments proving adherence to safety invariants: - `src/lazy.rs:63`: `unsafe impl<T, F: Send> Sync for Lazy<T, F>` 🟡 - Proposed comment: `// SAFETY: Lazy<T, F> synchronizes initialization of Cell<Option<F>> via Once<T>. Concurrent callers to Deref or force synchronize on Once::call_once, ensuring Option<F>::take and F invocation occur exactly once on a single thread before immutable &T access is shared across threads.` - `src/mutex.rs:117-118`: `unsafe impl Sync / Send for Mutex<T, R>` 🔴 - Proposed comment: `// SAFETY: Mutex<T, R> provides mutually exclusive access to T via InnerMutex<T, R>. Across threads, references &Mutex transfer exclusive access (&mut T or T) upon lock acquisition, making T: Send necessary and sufficient for both Send and Sync.` - `src/mutex.rs:318`: `unsafe impl RawMutex for Mutex<(), R>` 🔴 - Proposed comment: `// SAFETY: Mutex<(), R> guarantees mutual exclusion via atomic state transitions in InnerMutex, fulfilling the RawMutex contract.` - `src/mutex.rs:333`: `self.force_unlock()` inside `unsafe fn unlock` 🔴 - Proposed comment: `// SAFETY: By the RawMutex safety contract, unlock is contractually guaranteed to only be called when the current thread holds the lock.` - `src/mutex/fair.rs:107-108`: `unsafe impl Sync / Send for FairMutex<T, R>` 🟠 - Proposed comment: `// SAFETY: FairMutex<T, R> synchronizes access to UnsafeCell<T> via AtomicUsize. Since concurrent access is mutually exclusive, T: Send ensures safe transfer of T across threads.` - `src/mutex/fair.rs:110-111`: `unsafe impl Sync / Send for FairMutexGuard<'_, T>` 🟠 - Proposed comment: `// SAFETY: FairMutexGuard holds unique access (*mut T) to T. Moving across threads transfers exclusive access (requiring T: Send). Sharing &FairMutexGuard across threads allows shared access &T via Deref (requiring T: Sync).` - `src/mutex/fair.rs:221`: `unsafe { &mut *self.data.get() }` in `lock` 🔴 - Proposed comment: `// SAFETY: The thread has acquired the lock bit in self.lock, guaranteeing exclusive access to UnsafeCell<T>.` - `src/mutex/fair.rs:280`: `unsafe { &mut *self.data.get() }` in `try_lock_starver` 🔴 - Proposed comment: `// SAFETY: The atomic compare_exchange(0, LOCKED) succeeded, confirming absence of concurrent lock holders or starvers and securing exclusive access to UnsafeCell<T>.` - `src/mutex/fair.rs:347`: `unsafe { &mut *self.data.get() }` in `get_mut` 🟡 - Proposed comment: `// SAFETY: Exclusive borrow &mut self guarantees absence of concurrent borrows or active lock guards, making mutable dereference of UnsafeCell<T> sound.` - `src/mutex/fair.rs:400`: `unsafe { &mut *this.data }` in `FairMutexGuard::leak` 🟡 - Proposed comment: `// SAFETY: Wrapping guard in ManuallyDrop prevents unlock on drop, permanently locking the mutex and ensuring unique lifetime 'a access.` - `src/mutex/fair.rs:420, 427`: `unsafe { &*self.data }` and `unsafe { &mut *self.data }` in `Deref` / `DerefMut` 🟡 - Proposed comment: `// SAFETY: Existence of FairMutexGuard statically guarantees active exclusive lock tenure over self.data.` - `src/mutex/fair.rs:553, 568`: `unsafe impl RawMutex` and `self.force_unlock()` 🔴 - Proposed comment: `// SAFETY: FairMutex<(), R> adheres to mutual exclusion contracts. unlock is contractually called only by the lock holder.` - `src/mutex/spin.rs:87-88`: `unsafe impl Sync / Send for SpinMutex<T, R>` 🟠 - Proposed comment: `// SAFETY: SpinMutex<T, R> protects UnsafeCell<T> with an AtomicBool. Mutual exclusion guarantees safe cross-thread transfer of T when T: Send.` - `src/mutex/spin.rs:90-91`: `unsafe impl Sync / Send for SpinMutexGuard<'_, T>` 🟠 - Proposed comment: `// SAFETY: SpinMutexGuard represents unique access to T. Cross-thread transfer or sharing requires T: Send and T: Sync respectively.` - `src/mutex/spin.rs:241, 261`: `unsafe { &mut *self.data.get() }` in `try_lock` / `try_lock_weak` 🔴 - Proposed comment: `// SAFETY: Successful CAS transitioning lock from false to true establishes exclusive access to UnsafeCell<T>.` - `src/mutex/spin.rs:285`: `unsafe { &mut *self.data.get() }` in `get_mut` 🟡 - Proposed comment: `// SAFETY: &mut self statically guarantees unique ownership and absence of active lock guards.` - `src/mutex/spin.rs:330`: `unsafe { &mut *this.data }` in `SpinMutexGuard::leak` 🟡 - Proposed comment: `// SAFETY: ManuallyDrop prevents guard destruction, maintaining lock tenure indefinitely for lifetime 'a.` - `src/mutex/spin.rs:350, 357`: `unsafe { &*self.data }` and `unsafe { &mut *self.data }` in `Deref` / `DerefMut` 🟡 - Proposed comment: `// SAFETY: Existence of SpinMutexGuard guarantees active exclusive access to self.data.` - `src/mutex/spin.rs:369, 384`: `unsafe impl RawMutex` and `self.force_unlock()` 🔴 - Proposed comment: `// SAFETY: AtomicBool manipulation guarantees mutual exclusion. RawMutex contract ensures unlock is called only by the lock holder.` - `src/mutex/ticket.rs:89-90`: `unsafe impl Sync / Send for TicketMutex<T, R>` 🔴 - Proposed comment: `// SAFETY: Ticket ordering enforces mutually exclusive access to UnsafeCell<T>, requiring T: Send for safe cross-thread transfer.` - `src/mutex/ticket.rs:328`: `unsafe { &mut *data }` in `TicketMutexGuard::leak` 🔴 - Proposed comment: `// SAFETY: Forgetting the guard prevents next_serving from advancing, leaving TicketMutex locked permanently for unique lifetime 'a access.` - `src/mutex/ticket.rs:365, 380`: `unsafe impl RawMutex` and `self.force_unlock()` 🔴 - Proposed comment: `// SAFETY: Ticket sequence matching ensures mutual exclusion. RawMutex contract guarantees unlock is invoked only by the active serving thread.` - `src/once.rs:54-55`: `unsafe impl Sync / Send for Once<T, R>` 🟠 - Proposed comment: `// SAFETY: Once<T, R> shares &T across threads once initialized (requiring T: Sync). Moving Once across threads transfers T ownership (requiring T: Send).` - `src/once.rs:294`: `return unsafe { Ok(self.force_get()) }` 🔴 - Proposed comment: `// SAFETY: Atomic status was stored as Complete on line 291 and data was written on line 270, satisfying force_get preconditions.` - `src/once.rs:334, 404`: `unsafe { self.force_get() }` in `poll` / `get` 🟠 - Proposed comment: `// SAFETY: Confirming Status::Complete verifies data initialization, and Acquire ordering ensures visibility of initialized memory.` - `src/once.rs:432, 460`: `unsafe { self.force_get_mut() }` and `unsafe { self.force_into_inner() }` in `get_mut` / `try_into_inner` 🔴 - Proposed comment: `// SAFETY: Status::Complete confirms data initialization. Exclusive borrow or ownership of self guarantees unique access.` - `src/once.rs:502`: `core::ptr::drop_in_place(...)` in `Drop` 🔴 - Proposed comment: `// SAFETY: Status::Complete confirms MaybeUninit<T> contains a valid initialized value suitable for in-place dropping.` - `src/rwlock.rs:111-121`: `unsafe impl Send / Sync` for `RwLock`, `RwLockWriteGuard`, `RwLockReadGuard`, `RwLockUpgradableGuard` 🟠 - Proposed comment: `// SAFETY: RwLock enforces shared-read/exclusive-write access over UnsafeCell<T>. Sharing &RwLock across threads shares &T (requiring T: Sync). Guard transfer transfers &mut T or &T across threads (requiring T: Send and T: Sync as appropriate).` - `src/rwlock.rs:310, 381, 428`: `unsafe { &*self.data.get() }` and `unsafe { &mut *self.data.get() }` in `try_read`, `try_write`, `try_upgradeable_read` 🔴 - Proposed comment: `// SAFETY: Atomic lock state checks and CAS operations guarantee shared or exclusive access rights over UnsafeCell<T> before reference formation.` - `src/rwlock.rs:452`: `unsafe { &mut *self.data.get() }` in `get_mut` 🟡 - Proposed comment: `// SAFETY: Exclusive borrow &mut self guarantees absence of active read/write guards.` - `src/rwlock.rs:495, 555, 612`: Guard `Deref` and upgrade/downgrade data pointers 🔴 - Proposed comment: `// SAFETY: Guard lifecycle transitions atomically maintain required reader/writer counts to ensure valid shared or exclusive access.` - `src/rwlock.rs:672, 705`: Data dereference in `RwLockWriteGuard::downgrade` and `downgrade_to_upgradeable` 🔴 - Proposed comment: `// SAFETY: Reader or upgraded lock counts are atomically reserved before releasing exclusive write access.` - `src/rwlock.rs:821, 874, 924, 935`: `RawRwLock`, `RawRwLockUpgrade`, `RawRwLockDowngrade`, `RawRwLockUpgradeDowngrade` implementations 🔴 - Proposed comment: `// SAFETY: Atomic lock state manipulation adheres to lock_api contracts for shared, exclusive, and upgradable locking.` </details>

zesterer commented 2026年06月23日 21:21:06 +02:00

Thanks for reporting this. I've fixed the issue (along with a more minor oversight mentioned in the report) in #190.

If you were not intending to open an advisory on rustsec/advisory-db, I can open one myself tomorrow. Let me know.

FWIW, this flaw affects every release since and including 0.6.0.

Although the LLM decided that the severity is 'high', I will likely report it as medium: I have good reason to believe that the lock_api interface, upgradeable locks, and try_upgrade are all very seldom used features - I suspect it's unlikely that more than a small handful of downstream consumers even touch the affected codepath given that it requires the confluence of all three.

Thanks for reporting this. I've fixed the issue (along with a more minor oversight mentioned in the report) in #190. If you were not intending to open an advisory on `rustsec/advisory-db`, I can open one myself tomorrow. Let me know. FWIW, this flaw affects every release since and including `0.6.0`. Although the LLM decided that the severity is 'high', I will likely report it as medium: I have good reason to believe that the `lock_api` interface, upgradeable locks, and `try_upgrade` are all very seldom used features - I suspect it's unlikely that more than a small handful of downstream consumers even touch the affected codepath given that it requires the confluence of all three.

zesterer commented 2026年06月23日 21:28:50 +02:00

0.12.1 has been released and includes the fix for this issue.

`0.12.1` has been released and includes the fix for this issue.

Manishearth commented 2026年06月24日 10:47:39 +02:00

Although the LLM decided that the severity is 'high',

Oh, ignore that, it's a different ontology. I needed a way to prioritize by severity, and in general bugs in unsafe Rust tend to not be very severe at an absolute level, so things that are not that big a deal are categorized as "High". The "🤦 Accidental Misuse" threat vector makes it rare for this type of bug to be something that is actually high severity.

That audit report was generated with me as the audience, basically :)

If you are going through it, though, i would recommend going through its "fishy" findings as well as its suggested safety comment improvements. These are things I don't have the time to action, but if you feel motivated they might be nice to fix.

> Although the LLM decided that the severity is 'high', Oh, ignore that, it's a different ontology. I needed a way to prioritize by severity, and in general bugs in unsafe Rust tend to *not be very severe at an absolute level*, so things that are not that big a deal are categorized as "High". The "🤦 Accidental Misuse" threat vector makes it rare for this type of bug to be something that is *actually* high severity. That audit report was generated with me as the audience, basically :) If you are going through it, though, i would recommend going through its "fishy" findings as well as its suggested safety comment improvements. These are things I don't have the time to action, but if you feel motivated they might be nice to fix.

zesterer commented 2026年06月24日 21:30:59 +02:00

I've opened a PR on rustsec/advisory-db for this issue. At time of writing, it's not yet been assigned a CVE number.

When I get time, I plan to go over the entire crate afresh: there's a lot of old code in there - some of it from the pre-1.0 days, even - and Rust's memory model has come a long way. I think there's some of it that plays a little fast-and-loose with reference liveness and so on. I've already done quite a bit of work to tighten this up but there's still more to do. Unfortunately that will have to wait until I get some time off.

Given (a) the number of affected versions, (b) that nobody has noticed this issue for 6 years (and this report was driven by an automated review, not by a user crashing into it), and (c) the fact that hitting it requires using a very niche combination of features, I'm choosing to not yank the affected crate versions - I know from first-hand experience how much extra work and frustration yanking causes downstream users. If anybody feels differently about this decision, feel free to comment here.

Thanks for opening this issue @Manishearth.

I've opened [a PR](https://github.com/rustsec/advisory-db/pull/2994) on `rustsec/advisory-db` for this issue. At time of writing, it's not yet been assigned a CVE number. When I get time, I plan to go over the entire crate afresh: there's a lot of old code in there - some of it from the pre-1.0 days, even - and Rust's memory model has come a long way. I think there's some of it that plays a little fast-and-loose with reference liveness and so on. I've already done quite a bit of work to tighten this up but there's still more to do. Unfortunately that will have to wait until I get some time off. Given (a) the number of affected versions, (b) that nobody has noticed this issue for 6 years (and this report was driven by an automated review, not by a user crashing into it), and (c) the fact that hitting it requires using a very niche combination of features, I'm choosing to not yank the affected crate versions - I know from first-hand experience how much extra work and frustration yanking causes downstream users. If anybody feels differently about this decision, feel free to comment here. Thanks for opening this issue @Manishearth.