rwlock: avoid Stacked Borrows violation #121630
Conversation
rustbot
added
S-waiting-on-review
| node.next.0 = AtomicPtr::new(state.mask(MASK).cast()); | ||
| node.prev = AtomicLink::new(None); | ||
| let mut next = ptr::from_ref(&node) | ||
| // These writes can't race. |
There was a problem hiding this comment.
I'm not actually sure why we know that they can't race. After all, as the Miri error shows, an alias can clearly still exist and be used again in the future when these writes happen. This assumption is not new; we've been using non-atomic writes here already before this PR. Previously this was safe code only because rustc did not realize that the NonNull::from(&node) creates a borrow that could last into the next loop iteration.
| // Fall back to creating an unnamed `Thread` handle to allow locking in | ||
| // TLS destructors. | ||
| self.thread | ||
| .get_or_init(|| thread_info::current_thread().unwrap_or_else(|| Thread::new(None))); | ||
| self.completed = AtomicBool::new(false); | ||
| unsafe { self.completed.as_ptr().write(false) }; // can't have a race here |
There was a problem hiding this comment.
I should probably either make this a relaxed store, or make the function unsafe.
|
The weird thing is that the code actually doesn't make the assumption you mention; the shared reference that supposedly is invalidated is only created after all mutable accesses have occurred. Or so I thought... |
|
It seems Miri found an interleaving where an old shared reference gets used after the next loop iteration did some mutable accesses to the same node again. If that is itself a bug then this PR is the wrong fix. In that case the SB error in Miri would just mask a data race error. |
|
That is in itself a bug, yes, because the loop is only repeated when the node is not in a queue, otherwise you could get use-after-frees and things like that. |
|
r? @joboet |
|
This is the wrong fix, we need to figure out where the problem lies in the concurrency logic. |
Fixes #121626. Changes code recently added in #110211.
It seems like RwLock assumes that code like this is legal:
However, Stacked Borrows makes this illegal. That may be overeager, and indeed Tree Borrows allows this code, but Tree Borrows might also allow too much code, so until we are more sure about the future of the Rust aliasing model we should avoid code like this.
For the RwLock, this means not mixing direct write access to the mutable local variable
node, with access through (interior mutable) shared references.r? @m-ou-se @joboet