Soundness: Unsynchronized data races and aliasing violations in AtomicCell fallback path

[Manishearth]

Note

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

The Issue

In crossbeam_utils v0_8, AtomicCell provides atomic operations on types T. For types that are not lock-free, AtomicCell falls back to acquiring a global sharded lock.

However, in atomic_load at crossbeam-utils/src/atomic/atomic_cell.rs#L1048-L1084, the method attempts an optimistic read first:

if let Some(stamp) = lock.optimistic_read() {
    // ...
    let val = ptr::read_volatile(src.cast::<MaybeUninit<T>>());

This performs an unsynchronized volatile read from src without holding the fallback lock. Concurrently, another thread executing atomic_store, atomic_swap, or fetch_* acquires the lock and performs unsynchronized writes (ptr::write) or constructs an exclusive reference (&mut T).

This is a data race, and UB. Furthermore, constructing &mut T during fetch_add while another thread performs an unsynchronized read violates reference exclusivity invariants. Even though the optimistic read discards its value if stamp validation fails, the data race and aliasing violation occur instantaneously upon execution.

Minimal Reproduction (Miri)

use crossbeam_utils::atomic::AtomicCell;
use std::sync::Arc;
use std::thread;

#[derive(Clone, Copy)]
struct BigStruct([u64; 4]); // > 8 bytes, forces fallback lock path

fn main() {
    let cell = Arc::new(AtomicCell::new(BigStruct([0; 4])));

    let cell_clone = cell.clone();
    let writer = thread::spawn(move || {
        for i in 0..1000 {
            cell_clone.store(BigStruct([i; 4]));
        }
    });

    for _ in 0..1000 {
        let _val = cell.load();
    }

    writer.join().unwrap();
}

error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-1` at alloc221+0x10
    --> /google/src/cloud/manishearth/verify-unsafe-rust-bugs/google3/third_party/rust/crossbeam_utils/v0_8/src/atomic/atomic_cell.rs:1100:13
     |
1100 |             ptr::write(dst, val);
     |             ^^^^^^^^^^^^^^^^^^^^ (2) just happened here
     |
help: and (1) occurred earlier here
    --> src/repro1.rs:19:20
     |
  19 |         let _val = cell.load();
     |                    ^^^^^^^^^^^
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: this is on thread `unnamed-1`

Suggested Fix

Optimistic reads using ptr::read_volatile cannot be safely used as a fallback mechanism alongside non-atomic writes or &mut T references under global locks. For non-lock-free fallback types, atomic_load should directly acquire a read lock (lock.read()) before copying the value.

---

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.

Full Gemini Unsafe Code Audit Report

Unsafe Rust Review: crossbeam_utils (v0_8)

Overall Safety Assessment

The crossbeam_utils crate version v0_8 contains several crucial thread-safety utilities (AtomicCell, ShardedLock, Parker, etc.) which rely heavily on unsafe Rust and direct atomic manipulation. While the implementation of scoped threads and parking is generally sound and elegant (matching standard patterns), we have identified critical correctness and soundness issues in AtomicCell's fallback path (for types that are not lock-free). In addition, there is a total lack of safety comments (the style is either generic comments or completely missing), which does not satisfy the strict proof obligations of the safety review standard.

Critical Findings

1. AtomicCell Data Race Undefined Behavior in Fallback Path 🔴 🤦

• Severity: 🔴 High
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Data Race

In src/atomic/atomic_cell.rs (lines 1048-1084), when executing atomic_load for types that are not lock-free (which fall back to global locks):

            // Try doing an optimistic read first.
            if let Some(stamp) = lock.optimistic_read() {
                // ...
                let val = ptr::read_volatile(src.cast::<MaybeUninit<T>>());

                if lock.validate_read(stamp) {
                    return val.assume_init();
                }
            }

The optimistic read performs a volatile read (ptr::read_volatile) from src without acquiring the fallback lock. Concurrently, another thread can write to the same cell using atomic_store or atomic_swap, which calls ptr::write or ptr::replace under the fallback lock (i.e. without using atomics or volatile writes).

Under the Rust Memory Model, a concurrent unsynchronized read and write on the same memory location where at least one is not an atomic operation is a data race, which is Undefined Behavior. Even though the read value is discarded if the stamp validation fails, the data race itself has already occurred and can be exploited by compiler optimizations.

2. AtomicCell Exclusive Borrow (&mut T) Aliasing Violation in Fallback Path 🔴 🤦

• Severity: 🔴 High
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Aliasing Violation

In src/atomic/atomic_cell.rs (specifically in methods like fetch_add, fetch_sub, etc. on lines 371, 394, 934), the fallback implementation creates a mutable reference &mut T to the underlying value under the lock:

                let _guard = lock(self.as_ptr() as usize).write();
                let value = unsafe { &mut *(self.as_ptr()) };

Because atomic_load performs an optimistic read using ptr::read_volatile on the raw pointer without holding the lock, a reader thread can access the memory concurrently while the writer thread holds &mut T. This violates the absolute exclusivity requirement of &mut T (no other references or accesses to the memory may exist for its duration), leading to Undefined Behavior.

Fishy Findings

1. ShardedLock Self-Referential Design Relying on Heap Allocation Stability 🟡 🤦

• Severity: 🟡 Low
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Missing Invariant Documentation

In src/sync/sharded_lock.rs, the ShardedLock type contains a slice of shards: shards: Box<[CachePadded<Shard>]>. The Shard struct contains a write_guard field of type UnsafeCell<Option<RwLockWriteGuard<'static, ()>>>.

During write locking, the lock write guards (which borrow Shard::lock) are transmuted to 'static and stored in write_guard. This is a self-referential struct design.

This self-referential pattern is sound only because shards is stored inside a Box, meaning it is heap-allocated and its memory address remains stable even if ShardedLock is moved.

However, there is no documentation or safety comment explaining this crucial invariant. If a future maintainer changes Box<[CachePadded<Shard>]> to an inline array (e.g. [CachePadded<Shard>; NUM_SHARDS]) to avoid heap allocation, it would lead to silent memory corruption and use-after-free when ShardedLock is moved.

Missing Safety Comments

1. src/thread.rs 🔴

• unsafe impl Sync for Scope<'_> {} (line 224)

Proposed safety comment:

  // SAFETY: `Scope` only exposes thread-safe operations. The list of join
  // handles is wrapped in `Arc<Mutex<Vec<...>>>` and `wait_group` is a thread-safe
  // `WaitGroup`. Therefore, it is safe to share `Scope` across threads and spawn
  // threads concurrently.

• unsafe impl<T> Send/Sync for ScopedJoinHandle<'_, T> {} (lines
  489-490)

Proposed safety comment:

  // SAFETY: `ScopedJoinHandle` behaves like a standard `JoinHandle` but operates on
  // scoped data. The thread handle and result fields are thread-safe (`Arc<Mutex<...>>`
  // and `thread::Thread` are Send/Sync). Although `T` may not be Sync, the result
  // is protected by a Mutex, ensuring safe concurrent access. Since `spawn` requires
  // `T: Send`, it is safe to send the join handle itself across threads.

• Erasing 'env bound on closure (lines 463-466)

Proposed safety comment:

  // SAFETY: We transmute the closure's lifetime to `'static` to allow it to be passed
  // to `std::thread::spawn`. This is safe because:
  // 1. The parent scope blocks on `wg.wait()` until all spawned threads (which hold
  //    clones of the scope's `wait_group`) finish execution.
  // 2. Any remaining threads are joined inside the `scope` function before it returns.
  // This guarantees that no thread spawned in this scope can outlive the `'env` lifetime,
  // preventing any use-after-free of references borrowed by the closure.

2. src/sync/parker.rs 🔴

• unsafe impl Send for Parker and unsafe impl Send/Sync for Unparker
  (lines 58, 221-222)

Proposed safety comment:

  // SAFETY: `Parker` and `Unparker` synchronize thread parking using `Arc<Inner>`,
  // which contains `AtomicUsize`, `Mutex`, and `Condvar`. All of these types are
  // Send and Sync, making the raw thread parking primitives thread-safe.

• Unparker::from_raw / Parker::from_raw (lines 202, 289)

Proposed safety comment:

  // SAFETY: The caller guarantees that `ptr` was obtained from `into_raw`.
  // `into_raw` constructs `ptr` by calling `Arc::into_raw` on the `Unparker`'s inner `Arc`.
  // Reconstructing the `Arc` using `Arc::from_raw` is safe because it recovers exactly
  // one reference count from the original `Arc` instance, which was consumed by `into_raw`.

3. src/sync/sharded_lock.rs 🔴

• unsafe impl Send/Sync for ShardedLock and guards (lines 86-87,
  492, 523)

Proposed safety comment:

  // SAFETY: `ShardedLock` is thread-safe because access to the value is guarded
  // by a set of reader-writer locks (`RwLock`). If `T` is `Send`, the lock can be
  // transferred to another thread. If `T` is `Send + Sync`, concurrent reads are
  // safe since multiple threads can access `&T` simultaneously.

• get_mut dereference (line 193)

Proposed safety comment:

  // SAFETY: The method takes `&mut self`, which guarantees exclusive access to the
  // `ShardedLock`. No other thread can hold a read or write lock, making it safe to
  // mutably dereference the underlying `UnsafeCell`.

• transmute of RwLockWriteGuard in write/try_write (lines
  350, 426)

Proposed safety comment:

  // SAFETY: We transmute the guard's lifetime to `'static` because it must be stored
  // inside the `Shard` itself (self-referential structure). This is safe because:
  // 1. The shards are heap-allocated inside a `Box`, meaning they will not move and
  //    their memory addresses are stable.
  // 2. The `ShardedLockWriteGuard` borrows `self` for its lifetime, preventing
  //    `ShardedLock` from being dropped or moved while the guards are active.
  // 3. The `ShardedLockWriteGuard` destructor drops these guards in reverse order,
  //    releasing the locks before the borrow ends.

• Guard Deref / DerefMut implementations (lines 498, 556, 562)

Proposed safety comment:

  // SAFETY: The guard's existence proves that the thread has successfully acquired
  // the appropriate (read or write) lock on the shards. For read guards, no writers
  // can run concurrently. For write guards, exclusive access is guaranteed. Thus,
  // dereferencing the `UnsafeCell` is safe.

4. src/cache_padded.rs 🔴

• unsafe impl Send/Sync for CachePadded (lines 152-153)

Proposed safety comment:

  // SAFETY: `CachePadded<T>` is a transparent wrapper that only adds alignment and
  // padding to `T`. It has no other fields and does not affect the thread-safety
  // characteristics of `T`. Therefore, it is `Send`/`Sync` if and only if `T` is.

5. src/sync/once_lock.rs 🔴

• unsafe impl Send/Sync for OnceLock (lines 16-17)

Proposed safety comment:

  // SAFETY: `OnceLock` synchronizes access to the `UnsafeCell` using `std::sync::Once`.
  // Once initialized, the value is immutable. `Sync` requires `T: Sync + Send` because
  // the value can be initialized on one thread and dropped on another. `Send` requires
  // `T: Send` for the same reason.

• slot.write in initialize (line 68)

Proposed safety comment:

  // SAFETY: `call_once` guarantees that this closure is executed exactly once
  // across all threads. No other thread can access the slot concurrently. Since
  // the cell starts as uninitialized, we can safely overwrite it without dropping
  // an old value.

• get_unchecked dereference (line 77)

Proposed safety comment:

  // SAFETY: The caller guarantees that the value is initialized. `MaybeUninit<T>`
  // has the same layout and alignment as `T`. Dereferencing the cast pointer is
  // safe since the value is valid.

• assume_init_drop in Drop (line 85)

Proposed safety comment:

  // SAFETY: `is_completed` is true, which guarantees the inner value is initialized.
  // Since we are in `drop(&mut self)`, we have exclusive access to `self`, making it
  // safe to drop the initialized value in place.

[taiki-e]

Thanks for investigating our code.

This is a very well-known UB that has been described in the code comment and registered in the unsafe-code-guidelines as an issue that cannot be addressed in the current Rust without inline assembly, and also there is already a PR to address this (#1219).

followed by human review and verification.

I'm sorry, but I thought that "human review and verification" in this context meant confirming that this wasn’t a duplicate of a known issue. That said, since Linus only pointed this out relatively recently, is the general policy still to "open a report anyway as long as it’s valid"?

[Manishearth]

#1219

[Manishearth]

Apologies!

I'm sorry, but I thought that "human review and verification" in this context meant confirming that this wasn’t a duplicate of a known issue

I performed human review/verification of the issue itself: going through it to make sure it was valid, running miri. I've been spot-checking against open issues, but not in depth, and not against PRs, which I think I shall start doing now. I'm doing this process somewhat slowly to catch stuff like this.

In this case the failing miri test made it a slam dunk for me. I did notice the comment, and I probably should have mentioned it, but even for known issues personally I appreciate having an open issue to track the problem. As someone doing (usually fully human) unsafe review, having issues already filed for unsafe stuff is good. E.g. I have left this yoke theoretical soundness issue open even though we won't be able to fix it until Rust adds APIs it is planning to add as they figure this out. It's good to have a place to track it.

That said, since Linus only pointed this out relatively recently, is the general policy still to "open a report anyway as long as it’s valid"?

I don't think there's a "general policy" here. I don't think Rust crates have been receiving a barrage of unsafe bugs, so I'm worried about validity more than I'm worried about duplicates. So far as a maintainer I've received a handful of useful safety bugs but nothing overwhelming. (On the other hand, as a member of the Rust security WG, we are experiencing a massive uptick in reports, especially invalid ones)