Skip to content

fix data race in ReentrantLock fallback for targets without 64bit atomics #141248

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

Merged
merged 2 commits into from
May 19, 2025
Merged
Show file tree
Hide file tree
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
8 changes: 6 additions & 2 deletions library/std/src/sync/reentrant_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ cfg_if!(
// we only ever read from the tid if `tls_addr` matches the current
// TLS address. In that case, either the tid has been set by
// the current thread, or by a thread that has terminated before
// the current thread was created. In either case, no further
// the current thread's `tls_addr` was allocated. In either case, no further
// synchronization is needed (as per <https://github.com/rust-lang/miri/issues/3450>)
tls_addr: Atomic<usize>,
tid: UnsafeCell<u64>,
Expand All @@ -154,8 +154,12 @@ cfg_if!(
// NOTE: This assumes that `owner` is the ID of the current
// thread, and may spuriously return `false` if that's not the case.
fn contains(&self, owner: ThreadId) -> bool {
// We must call `tls_addr()` *before* doing the load to ensure that if we reuse an
// earlier thread's address, the `tls_addr.load()` below happens-after everything
// that thread did.
let tls_addr = tls_addr();
// SAFETY: See the comments in the struct definition.
self.tls_addr.load(Ordering::Relaxed) == tls_addr()
self.tls_addr.load(Ordering::Relaxed) == tls_addr
&& unsafe { *self.tid.get() } == owner.as_u64().get()
}

Expand Down
1 change: 1 addition & 0 deletions src/tools/miri/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,7 @@ Definite bugs found:
* [Weak-memory-induced memory leak in Windows thread-local storage](https://github.com/rust-lang/rust/pull/124281)
* [A bug in the new `RwLock::downgrade` implementation](https://rust-lang.zulipchat.com/#narrow/channel/269128-miri/topic/Miri.20error.20library.20test) (caught by Miri before it landed in the Rust repo)
* [Mockall reading unintialized memory when mocking `std::io::Read::read`, even if all expectations are satisfied](https://github.com/asomers/mockall/issues/647) (caught by Miri running Tokio's test suite)
* [`ReentrantLock` not correctly dealing with reuse of addresses for TLS storage of different threads](https://github.com/rust-lang/rust/pull/141248)

Violations of [Stacked Borrows] found that are likely bugs (but Stacked Borrows is currently just an experiment):

Expand Down
19 changes: 19 additions & 0 deletions src/tools/miri/tests/many-seeds/reentrant-lock.rs
Copy link
Member Author

Choose a reason for hiding this comment

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

This test will not be run by bors, only by the Miri test suite on the next sync. I verified it locally.

However we anyway don't have mips-unknown-linux-gnu in the Miri test suite... is there any tier 1 target without 64bit atomics?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah there is not. rust-lang/miri#4333 adds this mips target so that we will cover this in the future.

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![feature(reentrant_lock)]
//! This is a regression test for
//! <https://rust-lang.zulipchat.com/#narrow/channel/269128-miri/topic/reentrant.20lock.20failure.20on.20musl>.
use std::cell::Cell;
use std::sync::ReentrantLock;
use std::thread;

static LOCK: ReentrantLock<Cell<i32>> = ReentrantLock::new(Cell::new(0));

fn main() {
for _ in 0..20 {
thread::spawn(move || {
let val = LOCK.lock();
val.set(val.get() + 1);
drop(val);
});
}
}
Loading