Rollup merge of #141248 - RalfJung:reentrant-lock-race, r=joboet

matthiaskrgr · web-flow · commit 6e784f842a93 · 2025-05-19T18:08:42.000+02:00
fix data race in ReentrantLock fallback for targets without 64bit atomics See [Zulip](https://rust-lang.zulipchat.com/#narrow/channel/269128-miri/topic/reentrant.20lock.20failure.20on.20musl) for details: the address used to identify a thread might get lazily allocated inside `tls_addr()`, so if we call that *after* doing the `tls_addr.load()` it is too late to establish synchronization with prior threads that used the same address -- the `load()` thus races with the `store()` by that prior thread, and might hence see outdated values, and then the entire logic breaks down. r? `@joboet`
diff --git a/library/std/src/sync/reentrant_lock.rs b/library/std/src/sync/reentrant_lock.rs
@@ -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>,
@@ -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()
             }
 
diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md
@@ -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):
 
diff --git a/src/tools/miri/tests/many-seeds/reentrant-lock.rs b/src/tools/miri/tests/many-seeds/reentrant-lock.rs
@@ -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);
+        });
+    }
+}
