atomic-rs
atomic-rs copied to clipboard
Footgun: compare_exchange may fail if T contains unused bytes
Atomic::compare_exchange compares the bitvalues of the contents and its first argument. In Rust, even values of simple types can be considered equal without being bitwise-equal. I ran into this with Atomic<Option<NonNull<[f32]>>>.
Option<NonNull<[f32]>> is 128 bits large. The old value had a bit pattern of 0x0000009ace77f1030000000000000000, which corresponds to the value None. The value of None that I supplied had a bit pattern of all zeros, so the compare_exchange failed.
Maybe it's obvious that compare_exchange is not reliable for types without primitive bitwise equality, but I think it should still be documented. I can open a PR, but I'm not completely sure how to explain this in the docs.
We could also try to provide a solution, adding a function somewhat like this:
fn compare_exchange_careful(&self, mut current: T, new: T, success: Ordering, failure: Ordering) -> Result<T, T> {
loop {
match self.compare_exchange(current, new, success, failure) {
Ok(v) => return Ok(v),
Err(old_value) => if old_value == current {
current = old_value;
continue;
} else {
return Err(old_value);
}
}
}
}
Yes, the docs should definitely clarify that bitwise comparisons are used and give your example as a potential pitfall. In fact, the docs should recommend sticking to primitive types: in your case you should probably used Atomic<(*mut f32, usize)>
instead which doesn't have this issue.
Isn't this unsound undefined behavior? Since you're casting padding bytes (uninitialized bytes) to an integer type (where all bytes are expected to be initialized), you're practically calling MaybeUninit::uninit().assume_injt()
, which is bad news.
I know that this is the primary motivation behind atomic-maybe-uninit
. It might be worth it to have a NoPaddingBytes
marker trait, or to only take types that are bytemuck::NoUninit
.
I just had a look at atomic-maybe-uninit
and it looks great. I would happily accept a PR which changes this crate to use atomic-maybe-uninit
instead of the core atomic types.
Unfortunately I don't have the bandwidth to make a PR here for the foreseeable future