Rollup merge of rust-lang#161451 - ohadravid:windows-tls-avoid-registering-dtors-in-fibers, r=ChrisDenton

Avoid arming the Windows TLS destructor guard in fibers

After rust-lang#157645, we use FLS to trigger destructors for thread locals.

In bytecodealliance/wasmtime#14184, it turned out that there's an edge case we didn't cover: if `thread_local/guard/windows.rs::enable` is called from a fiber and later, in a different fiber: (1) the fiber is converted back to a thread and (2) the thread deletes the original fiber, we incorrectly triggers the FLS destructors prematurely.

The fix is simple - avoid arming the FLS slot (setting it to 1) if the calling thread is a fiber.

The issue happened on mingw because it does have target thread local. Tested locally by forcing `registered = false` and checked that the new test fails without the fix.

r? @ChrisDenton

Author: JonathanBrouwer

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

@@ -176,6 +176,12 @@ pub fn enable() {
             }
         };
 
+        // We must not set the key if we are in a fiber, since deleting that fiber from a thread
+        // will cause the destructors to run before thread exit.
+        if is_thread_a_fiber() {
+            return;
+        }
+
         // Setting the key's value to non-zero will cause the dtor callback to be called when the thread exits.
         unsafe { set(key, ptr::without_provenance(1)) };
     }

library/std/src/thread/local.rs

@@ -98,17 +98,16 @@ use crate::fmt;
 ///    run on the thread that causes the process to exit. This is because the
 ///    other threads may be forcibly terminated.
 ///
-///    If a thread is [converted into a fiber], destructors will not be run unless
-///    the fiber is [converted back into a thread] before the underlying thread exits.
+///    TLS destructors may be leaked if a thread exits while [converted into a fiber],
+///    or if Rust TLS destructor support is first needed while running in a fiber.
 ///
 ///    If a process loads a Rust `cdylib`, it must not cause the Rust TLS destructor support
-//     to be initialized for the first time during process shutdown.
+///    to be initialized for the first time during process shutdown.
 ///
 ///    When dynamically unloading a Rust `cdylib`, pending TLS destructors may run
-//     during the unload or may be leaked.
+///    during the unload or may be leaked.
 ///
 /// [converted into a fiber]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-convertthreadtofiber
-/// [converted back into a thread]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-convertfibertothread
 /// [loader lock]: https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-best-practices
 /// [`with`]: LocalKey::with
 #[cfg_attr(not(test), rustc_diagnostic_item = "LocalKey")]

library/std/tests/thread_local/tests.rs

@@ -416,9 +416,17 @@ fn fiber_does_not_trigger_dtor() {
     unsafe extern "system" {
         fn ConvertFiberToThread() -> i32;
         fn ConvertThreadToFiber(lpParameter: *const c_void) -> *mut c_void;
+        fn CreateFiber(
+            dwStackSize: usize,
+            lpStartAddress: unsafe extern "system" fn(*mut c_void),
+            lpParameter: *mut c_void,
+        ) -> *mut c_void;
+        fn DeleteFiber(lpFiber: *mut c_void);
+        fn SwitchToFiber(lpFiber: *mut c_void);
     }
 
     thread_local!(static FOO: UnsafeCell<Option<NotifyOnDrop>> = UnsafeCell::new(None));
+
     let signal = Signal::default();
 
     let signal2 = signal.clone();
@@ -438,13 +446,49 @@ fn fiber_does_not_trigger_dtor() {
     // As long as we stop using fibers before thread teardown, everything works as expected.
     let signal2 = signal.clone();
     let t = thread::spawn(move || unsafe {
-        let mut signal = Some(signal2);
-        let _ = ConvertThreadToFiber(ptr::null());
-        FOO.with(|f| {
-            *f.get() = Some(NotifyOnDrop(signal.take().unwrap()));
-        });
-        let _ = ConvertFiberToThread();
+        struct FiberData {
+            main: *mut c_void,
+            signal: Signal,
+        }
+
+        unsafe extern "system" fn fiber_start(data: *mut c_void) {
+            let data = unsafe { &mut *data.cast::<FiberData>() };
+
+            // Set the value while this fiber is current.
+            // This must NOT arm the FLS cleanup guard for the fiber.
+            FOO.with(|f| unsafe {
+                *f.get() = Some(NotifyOnDrop(data.signal.clone()));
+            });
+
+            unsafe {
+                SwitchToFiber(data.main);
+            }
+        }
+
+        let main = ConvertThreadToFiber(ptr::null());
+        assert!(!main.is_null());
+
+        let mut data = FiberData { main, signal: signal2.clone() };
+        let foo = CreateFiber(0, fiber_start, ptr::from_mut(&mut data).cast());
+        assert!(!foo.is_null());
+
+        // Run `foo`, which sets FOO while `foo` is the current fiber,
+        // then switches back to main.
+        SwitchToFiber(foo);
+
+        // Convert main back to a thread before deleting `foo`.
+        assert_ne!(ConvertFiberToThread(), 0);
+
+        // Deleting `foo` must not trigger dtors like a thread teardown.
+        DeleteFiber(foo);
+        assert!(!signal2.is_set());
+
+        // Arm the guard now from the normal thread.
+        // `FOO`'s destructor is already registered, so it will run when the thread exits.
+        thread_local!(static BAR: UnsafeCell<Option<NotifyOnDrop>> = UnsafeCell::new(None));
+        BAR.with(|_| {});
     });
+
     signal.wait();
     assert!(signal.is_set());
     t.join().unwrap();