Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rwlock: avoid Stacked Borrows violation #121630

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions library/std/src/sys/locks/rwlock/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,12 @@ impl Node {
}

/// Prepare this node for waiting.
fn prepare(&mut self) {
fn prepare(&self) {
// 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably either make this a relaxed store, or make the function unsafe.

}

/// Wait until this node is marked as completed.
Expand Down Expand Up @@ -316,9 +316,13 @@ impl RwLock {
#[cold]
fn lock_contended(&self, write: bool) {
let update = if write { write_lock } else { read_lock };
let mut node = Node::new(write);
let node = Node::new(write);
let mut state = self.state.load(Relaxed);
let mut count = 0;
// Aliasing rules require us to not mix direct accesses to `node` and accesses through a
// reference. So let's make sure we only use the reference. Fixes
// <https://github.com/rust-lang/rust/issues/121626>.
let node = &node;
loop {
if let Some(next) = update(state) {
// The lock is available, try locking it.
Expand All @@ -343,9 +347,10 @@ impl RwLock {
// pointer to the next node in the queue. Otherwise set it to
// the lock count if the state is read-locked or to zero if it
// is write-locked.
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.
Copy link
Member Author

@RalfJung RalfJung Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unsafe { node.next.0.as_ptr().write(state.mask(MASK).cast()) };
unsafe { node.prev.0.as_ptr().write(ptr::null_mut()) };
let mut next = ptr::from_ref(node)
.map_addr(|addr| addr | QUEUED | (state.addr() & LOCKED))
as State;

Expand All @@ -354,7 +359,7 @@ impl RwLock {
// the node itself to ensure there is a current `tail` field in
// the queue (invariants 1 and 2). This needs to use `set` to
// avoid invalidating the new pointer.
node.tail.set(Some(NonNull::from(&node)));
node.tail.set(Some(NonNull::from(node)));
} else {
// Otherwise, the tail of the queue is not known.
node.tail.set(None);
Expand Down
Loading