3 ms·

Those 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 cur

Those 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 ago
Whoops, 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 ago
  Miri 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 ago
    I 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.

amluto 2y ago

Whoops, 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 ago
Miri 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 ago
  I 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.