3 ms·
The tagged pointer example says, in the safety notes: > Pointer Dereferencing: Pointers are only dereferenced after successful CAS operations I think a rather
by amluto 2y ago
The tagged pointer example says, in the safety notes:
> Pointer Dereferencing: Pointers are only dereferenced after successful CAS operations
I think a rather stronger argument should be needed. But the code doesn’t even follow what the notes say:
let current = self.head.load(Ordering::Acquire);
if current.ptr.is_null() {
return None;
}
let next = unsafe { (*current.ptr).next };
No CAS. And this sure looks wrong — the current head could be popped and freed before being dereferenced.
edit: put the correct code in. Thanks, LegionMammal978
- LegionMammal978 2y agoThose lines don't actually dereference anything, except for (*new_node) which is owned by the function. Immediately following them is the CAS to ensure that current is still at self.head. If it gets deallocated, then the CAS fails, and it tries again with the new self.head.
- amluto 2y agoWhoops, I pasted entirely the wrong code! let current = self.head.load(Ordering::Acquire); if current.ptr.is_null() { return None; } let next = unsafe { (*current.ptr).next }; On the one hand, one might say “well, it’s fine if we read the wrong value here if the old head was popped in the mean time”, but it’s still UB, and it can even segfault if the old head is freed and unmapped.
- empath75 2y agoMiri does catch a problem here: > error: Undefined Behavior: memory access failed: expected a pointer to 8 bytes of memory, but got 0x2f5f40[noalloc] which is a dangling pointer (it has no provenance) --> src/main.rs:263:33 | 263 | let next = unsafe { (*current.ptr).next }; | ^^^^^^^^^^^^^^^^^^^ memory access failed: expected a pointer to 8 bytes of memory, but got 0x2f5f40[noalloc] which is a dangling pointer (it has no provenance) | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html https://doc.rust-lang.org/nightly/reference/behavior-conside... for further information = note: BACKTRACE on thread `unnamed-2`: = note: inside `LockFreeStack::pop` at src/main.rs:263:33: 263:52
- amluto 2y agoI think this is an unrelated problem: https://doc.rust-lang.org/std/ptr/index.html#provenance https://doc.rust-lang.org/std/ptr/index.html#provenance Fixing it cleanly seems like it might need a new AtomicPtrAndU64 or the ability to have an atomic 128-bit struct, and I have no idea whether the latter is even conceptually compatible with miri.