Skip to content

Commit 4733345

Browse files
authored
Rollup merge of #157483 - RalfJung:windows-gnu-tls-leak, r=ChrisDenton
fix windows-gnu TLS leak Running the std tests on windows-gnu [has a memory leak](https://rust-lang.zulipchat.com/#narrow/channel/269128-miri/topic/Miri.20test-libstd.20Failure.20.282026-06.29/near/599991435). After lots of false leads, here's what I found: - The `guard::enable` function needs to be called on each thread; it the ensures that when the thread finishes, cleanup is performed. - The windows `LazyKey` calls `guard::enable` in its `init` function. That means it only gets called on the first thread that accesses this key! - We have a `guard::enable` call in the `thread::spawn` path. However, when running tests, there are two copies of std, so there are two guards. `crate::thread::spawn` sets up the guard for the current crate (the one built with `cfg(test)`), but does not set up the guard for `realstd` (the std crate in the sysroot). This is a regression introduced by #148799. Previously, `guard::enable` was pretty much a NOP on Windows so not calling it didn't cause problems. rust-lang/miri-test-libstd#123 has confirmed that this indeed fixes the leak. The first commit is a drive-by fix where I found a function name kind of confusing. r? @ChrisDenton Cc @joboet
2 parents b3b3d54 + d047870 commit 4733345

10 files changed

Lines changed: 60 additions & 15 deletions

File tree

library/std/src/sys/thread_local/guard/windows.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ impl Drop for EnableGuard {
106106
}
107107
}
108108

109+
/// Set up the current thread to invoke `cleanup` when it finishes.
109110
pub fn enable() {
110111
let registered = if cfg!(target_thread_local) {
111112
#[thread_local]

library/std/src/sys/thread_local/key/windows.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,12 @@ impl LazyKey {
6060

6161
#[inline]
6262
pub fn force(&'static self) -> Key {
63+
if self.dtor.is_some() {
64+
// Needs to be called on all threads where the key might have a non-null value!
65+
// Otherwise, `run_dtors` might not be called on this thread.
66+
guard::enable();
67+
}
68+
6369
match self.key.load(Acquire) {
6470
0 => unsafe { self.init() },
6571
key => key - 1,
@@ -88,6 +94,8 @@ impl LazyKey {
8894
}
8995

9096
unsafe {
97+
// Add ourselves to the `DTORS` list, so that when `run_dtors` gets called,
98+
// our dtor is invoked.
9199
register_dtor(self);
92100
}
93101

@@ -144,8 +152,6 @@ static DTORS: Atomic<*mut LazyKey> = AtomicPtr::new(ptr::null_mut());
144152
/// Should only be called once per key, otherwise loops or breaks may occur in
145153
/// the linked list.
146154
unsafe fn register_dtor(key: &'static LazyKey) {
147-
guard::enable();
148-
149155
let this = <*const LazyKey>::cast_mut(key);
150156
// Use acquire ordering to pass along the changes done by the previously
151157
// registered keys when we store the new head with release ordering.

library/std/src/sys/thread_local/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ pub(crate) mod destructors {
8181

8282
/// This module provides a way to schedule the execution of the destructor list
8383
/// and the [runtime cleanup](crate::rt::thread_cleanup) function. Calling `enable`
84-
/// should ensure that these functions are called at the right times.
84+
/// sets up the current thread to ensure that these functions are called at the right times.
8585
pub(crate) mod guard {
8686
cfg_select! {
8787
all(target_thread_local, target_vendor = "apple") => {

library/std/src/thread/lifecycle.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ where
6666
let rust_start = move || {
6767
let f = f.into_inner();
6868
let try_result = panic::catch_unwind(panic::AssertUnwindSafe(|| {
69-
crate::sys::backtrace::__rust_begin_short_backtrace(|| hooks.run());
69+
crate::sys::backtrace::__rust_begin_short_backtrace(|| hooks.inherit_and_run());
7070
crate::sys::backtrace::__rust_begin_short_backtrace(f)
7171
}));
7272
// SAFETY: `their_packet` as been built just above and moved by the

library/std/src/thread/spawnhook.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ pub(super) struct ChildSpawnHooks {
144144

145145
impl ChildSpawnHooks {
146146
// This is run on the newly spawned thread, directly at the start.
147-
pub(super) fn run(self) {
147+
pub(super) fn inherit_and_run(self) {
148148
SPAWN_HOOKS.set(self.hooks);
149149
for run in self.to_run {
150150
run();

library/std/tests/thread_local/tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ fn thread_current_in_dtor() {
408408
// https://github.com/rust-lang/rust/pull/148799#issuecomment-3731806901
409409
#[cfg(target_os = "windows")]
410410
#[test]
411+
#[cfg_attr(miri, ignore)] // Miri does not support fibers
411412
fn fiber_does_not_trigger_dtor() {
412413
use core::ffi::c_void;
413414
use std::ptr;

tests/run-make/dynamic-loading-cdylib/foo.rs

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,36 @@ pub extern "C" fn extern_fn_1(a: u32, b: u32) -> u32 {
66
a + b
77
}
88

9-
struct NotifyOnDrop;
9+
#[derive(Default)]
10+
struct NotifyOnDrop {
11+
last_result: std::cell::Cell<u32>,
12+
}
13+
14+
impl NotifyOnDrop {
15+
fn save_and_print(&self, result: u32) {
16+
self.last_result.set(result);
17+
}
18+
}
1019

1120
impl Drop for NotifyOnDrop {
1221
fn drop(&mut self) {
13-
println!("drop");
22+
println!("dropping, last result: {}", self.last_result.get());
1423
}
1524
}
1625

26+
thread_local! {
27+
static FOO: NotifyOnDrop = NotifyOnDrop::default();
28+
}
29+
1730
#[no_mangle]
1831
pub extern "C" fn extern_fn_2(a: u32, b: u32) -> u32 {
19-
thread_local!(static FOO: NotifyOnDrop = NotifyOnDrop);
20-
FOO.with(|_foo| {});
21-
println!("extern_fn_2");
22-
a * b
32+
let result = a * b;
33+
34+
FOO.with(|foo| {
35+
foo.save_and_print(result);
36+
});
37+
38+
println!("extern_fn_2({a}, {b})");
39+
40+
result
2341
}

tests/run-make/dynamic-loading-cdylib/load_and_unload.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,15 @@ fn main() {
101101
let result = unsafe { extern_fn_2(2, 3) };
102102
println!("result of extern_fn_2(2, 3): {}", result);
103103

104+
println!("spawning thread");
105+
std::thread::spawn(move || {
106+
let result = unsafe { extern_fn_2(4, 5) };
107+
println!("result of extern_fn_2(4, 5) in other thread: {}", result);
108+
})
109+
.join()
110+
.expect("Thread panicked");
111+
println!("thread joined");
112+
104113
libloading::unload(foo);
105114
println!("unloaded library");
106115
}
Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
loaded library
22
extern_fn_1
33
result of extern_fn_1(2, 3): 5
4-
extern_fn_2
4+
extern_fn_2(2, 3)
55
result of extern_fn_2(2, 3): 6
6+
spawning thread
7+
extern_fn_2(4, 5)
8+
result of extern_fn_2(4, 5) in other thread: 20
9+
dropping, last result: 20
10+
thread joined
611
unloaded library
7-
drop
12+
dropping, last result: 6
Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
loaded library
22
extern_fn_1
33
result of extern_fn_1(2, 3): 5
4-
extern_fn_2
4+
extern_fn_2(2, 3)
55
result of extern_fn_2(2, 3): 6
6-
drop
6+
spawning thread
7+
extern_fn_2(4, 5)
8+
result of extern_fn_2(4, 5) in other thread: 20
9+
dropping, last result: 20
10+
thread joined
11+
dropping, last result: 6
712
unloaded library

0 commit comments

Comments
 (0)