mystor / atomic_ref Goto Github PK

AtomicRef's methods happily accept the Relaxed memory ordering, but this is actually unsound.

Consider a situation in which Thread 1 passes a shared reference of type &T to Thread 2 via AtomicRef<T>, and Thread 2 reads the referenced T (let's call this R). This might look safe because mutating T through &T would require another synchronization primitive anyway. However, it's also possible that Thread 1 created &T by downgrading &mut T, on which Thread 1 might have written something (let's call this W). If the Relaxed memory ordering is used, W and R aren't synchronized properly, causing an undefined behavior.

Demonstration

The following code segfaults on an AArch64 machine (but not on x86_64, which has a stronger memory model).

#![deny(unsafe_code)]
use atomic_ref::AtomicRef;
use std::sync::atomic::Ordering;

fn main() {
    let channel = &*Box::leak(Box::new(AtomicRef::<&'static u32>::new(None)));

    std::thread::spawn(move || loop {
        if let Some(ptr) = channel.load(Ordering::Relaxed) {
            assert_eq!(**ptr, 42);  // read *b
        }
    });

    for i in 0..10000000 {
        let b = Box::leak(Box::new(&42u32));    // write to *b
        channel.store(Some(b), Ordering::Relaxed);
    }
}

     Running `target/release/test-atomic_ref`
fish: 'cargo run --release' terminated by signal SIGSEGV (Address boundary error)

Proposed solution

Require Release for all stores and Acquire for all loads, including individual suboperations in RMW operations (e.g., AcqRel for successful swap operations). Ignore the memory ordering requested by the caller silently if it's too weak. (Or, such cases could be handled by panicking, but that would be a compatibility-breaking change.)