-
Notifications
You must be signed in to change notification settings - Fork 3
Description
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.)