Skip to content

Commit 24fef3f

Browse files
authored
Unrolled build for rust-lang#141248
Rollup merge of rust-lang#141248 - RalfJung:reentrant-lock-race, r=joboet 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`
2 parents e5a2a6a + 26ea763 commit 24fef3f

File tree

3 files changed

+26
-2
lines changed

3 files changed

+26
-2
lines changed

library/std/src/sync/reentrant_lock.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ cfg_if!(
136136
// we only ever read from the tid if `tls_addr` matches the current
137137
// TLS address. In that case, either the tid has been set by
138138
// the current thread, or by a thread that has terminated before
139-
// the current thread was created. In either case, no further
139+
// the current thread's `tls_addr` was allocated. In either case, no further
140140
// synchronization is needed (as per <https://github.com/rust-lang/miri/issues/3450>)
141141
tls_addr: Atomic<usize>,
142142
tid: UnsafeCell<u64>,
@@ -154,8 +154,12 @@ cfg_if!(
154154
// NOTE: This assumes that `owner` is the ID of the current
155155
// thread, and may spuriously return `false` if that's not the case.
156156
fn contains(&self, owner: ThreadId) -> bool {
157+
// We must call `tls_addr()` *before* doing the load to ensure that if we reuse an
158+
// earlier thread's address, the `tls_addr.load()` below happens-after everything
159+
// that thread did.
160+
let tls_addr = tls_addr();
157161
// SAFETY: See the comments in the struct definition.
158-
self.tls_addr.load(Ordering::Relaxed) == tls_addr()
162+
self.tls_addr.load(Ordering::Relaxed) == tls_addr
159163
&& unsafe { *self.tid.get() } == owner.as_u64().get()
160164
}
161165

src/tools/miri/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -580,6 +580,7 @@ Definite bugs found:
580580
* [Weak-memory-induced memory leak in Windows thread-local storage](https://github.com/rust-lang/rust/pull/124281)
581581
* [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)
582582
* [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)
583+
* [`ReentrantLock` not correctly dealing with reuse of addresses for TLS storage of different threads](https://github.com/rust-lang/rust/pull/141248)
583584

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#![feature(reentrant_lock)]
2+
//! This is a regression test for
3+
//! <https://rust-lang.zulipchat.com/#narrow/channel/269128-miri/topic/reentrant.20lock.20failure.20on.20musl>.
4+
5+
use std::cell::Cell;
6+
use std::sync::ReentrantLock;
7+
use std::thread;
8+
9+
static LOCK: ReentrantLock<Cell<i32>> = ReentrantLock::new(Cell::new(0));
10+
11+
fn main() {
12+
for _ in 0..20 {
13+
thread::spawn(move || {
14+
let val = LOCK.lock();
15+
val.set(val.get() + 1);
16+
drop(val);
17+
});
18+
}
19+
}

0 commit comments

Comments
 (0)