Closed wang384670111 closed 8 months ago
Oh dear, that’s big if true, thanks! I think the same code has been running in std for the past four years?
It does look reasonable on the first glance: that compare exchange is an rmw (read-modify-write) operation, and, if we care to Relase w, we will probably want to Acquire r…
The linage of this ordering traces back to https://github.com/rust-lang/rust/pull/65719, where SeqCst orderings were first relaxed.
Will take a close look once I properly wake-up 🤣
Hm, having properly woken up, I think the code is correct. The reason for that is the call site of this function:
We actually don't need wait
to have Acquire semantics, because we are going to just re-load with Acquire anyway?
It appears to be a false positive from my detector. Upon closer examination of the code, I realized that there is no dereferencing operation involved in the exchange
, the strict::addr(new_queue)
operates on the location of the pointer itself, rather than the address to which the pointer points, so using Relaxed
here is acceptable. However, if dereferencing of the exchange
is involved, Acquire
should be used in this scenario.
https://github.com/matklad/once_cell/blob/c48d3c2c01de926228aea2ac1d03672b4ce160c1/src/imp_std.rs#L226-L232
It seems that I have to modify the detection pattern😂.
Hm, having properly woken up, I think the code is correct. The reason for that is the call site of this function:
We actually don't need
wait
to have Acquire semantics, because we are going to just re-load with Acquire anyway?
I agree, Acquire is fine here.
After lying in bed for a while, I feel much more awake now! The ordering in the code is fine, and I've figured out how to modify the detector pattern. I think we can consider this issue closed. Good night! 😴
I developed a static analysis tool to detect issues related to memory ordering, thread performance, and security. During my examination of several crates, I encountered alarms triggered by the following code: https://github.com/matklad/once_cell/blob/c48d3c2c01de926228aea2ac1d03672b4ce160c1/src/imp_std.rs#L213-L232 The meaning of the code should be that we need to have a successful initialization operation in Line222, but the compare exchange also entails reading the atomic pointer to the
exchange
. Therefore, it's necessary to useAcqRel
andAcquire
to observe any other modifications to the memory that the atomic pointer references, both when the compare exchange succeeds and when it fails.I think compare exchange for
queue
usesAcqRel
for the success case andAcquire
for the fail case.(happy to make a PR if this looks reasonable)