Skip to content

AtomicRef accepts a memory ordering weaker than what would be safe #5

@yvt

Description

@yvt

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.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions