Only run the destructors hook when not running in a fiber

Author: ohadravid

library/std/src/sys/pal/windows/c/bindings.txt

@@ -2264,6 +2264,7 @@ IPV6_DROP_MEMBERSHIP
 IPV6_MREQ
 IPV6_MULTICAST_LOOP
 IPV6_V6ONLY
+IsThreadAFiber
 LINGER
 listen
 LocalFree

library/std/src/sys/pal/windows/c/windows_sys.rs

@@ -72,6 +72,7 @@ windows_targets::link!("kernel32.dll" "system" fn GetWindowsDirectoryW(lpbuffer
 windows_targets::link!("kernel32.dll" "system" fn InitOnceBeginInitialize(lpinitonce : *mut INIT_ONCE, dwflags : u32, fpending : *mut BOOL, lpcontext : *mut *mut core::ffi::c_void) -> BOOL);
 windows_targets::link!("kernel32.dll" "system" fn InitOnceComplete(lpinitonce : *mut INIT_ONCE, dwflags : u32, lpcontext : *const core::ffi::c_void) -> BOOL);
 windows_targets::link!("kernel32.dll" "system" fn InitializeProcThreadAttributeList(lpattributelist : LPPROC_THREAD_ATTRIBUTE_LIST, dwattributecount : u32, dwflags : u32, lpsize : *mut usize) -> BOOL);
+windows_targets::link!("kernel32.dll" "system" fn IsThreadAFiber() -> BOOL);
 windows_targets::link!("kernel32.dll" "system" fn LocalFree(hmem : HLOCAL) -> HLOCAL);
 windows_targets::link!("kernel32.dll" "system" fn LockFileEx(hfile : HANDLE, dwflags : LOCK_FILE_FLAGS, dwreserved : u32, nnumberofbytestolocklow : u32, nnumberofbytestolockhigh : u32, lpoverlapped : *mut OVERLAPPED) -> BOOL);
 windows_targets::link!("kernel32.dll" "system" fn MoveFileExW(lpexistingfilename : PCWSTR, lpnewfilename : PCWSTR, dwflags : MOVE_FILE_FLAGS) -> BOOL);

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

@@ -36,6 +36,11 @@ unsafe fn set(key: Key, ptr: *const c_void) {
     }
 }
 
+fn is_thread_a_fiber() -> bool {
+    let res = unsafe { c::IsThreadAFiber() };
+    res == c::TRUE
+}
+
 static KEY: AtomicU32 = AtomicU32::new(FLS_OUT_OF_INDEXES);
 
 pub fn enable() {
@@ -63,25 +68,24 @@ pub fn enable() {
             }
         };
 
-        // Setting the key's value to non-zero will cause the dtor callback to be called when the thread or fiber exits.
-        // We only set the key once per thread, so the destructors are guaranteed to run at most once.
-        // We still need to verify that we won't run the destructors _before_ the thread exits,
-        // so we need to check what happens when a fiber terminates (fibers cannot be moved between threads).
-        //
-        // According to [Fibers entry in MSDN](https://learn.microsoft.com/en-us/windows/win32/procthread/fibers):
-        // > If your fiber function returns, the thread running the fiber exits.
-        // > ..
-        // > If the currently running fiber calls DeleteFiber, its thread calls ExitThread and terminates.
-        // > However, if the selected fiber of a thread is deleted by a fiber running in another thread,
-        // > the thread with the deleted fiber is likely to terminate abnormally because the fiber stack has been freed.
-        //
-        // Meaning a fiber's termination results in thread termination or unrecoverable failure,
-        // so destructors cannot run while the thread is still executing user code.
+        // Setting the key's value to non-zero will cause the dtor callback to be called when the thread exits.
+        // We only set the key once per thread, so the destructors are guaranteed to run at most once (fibers cannot be moved between threads).
         unsafe { set(key, ptr::without_provenance(1)) };
     }
 }
 
 unsafe extern "system" fn cleanup(_ptr: *const c_void) {
+    // Avoid running the hook if we are in a fiber.
+    //
+    // We need to verify that we won't run the destructors *before* the thread exits,
+    // but if the fiber that registered the callback is deleted, other fibers can still be running in the thread.
+    //
+    // By checking that we are not running in a fiber here, we are guaranteed that the hook is only running at the thread teardown.
+    // See also the `fiber_does_not_trigger_dtor` test.
+    if is_thread_a_fiber() {
+        return;
+    }
+
     unsafe {
         #[cfg(target_thread_local)]
         super::super::destructors::run();

library/std/tests/thread_local/tests.rs

@@ -21,6 +21,12 @@ impl Signal {
             set = cvar.wait(set).unwrap();
         }
     }
+
+    fn is_set(&self) -> bool {
+        let (set, _cvar) = &*self.0;
+        let set = set.lock().unwrap();
+        *set
+    }
 }
 
 struct NotifyOnDrop(Signal);
@@ -97,6 +103,7 @@ fn smoke_dtor() {
             });
         });
         signal.wait();
+        assert!(signal.is_set());
         t.join().unwrap();
     }
 }
@@ -393,3 +400,51 @@ fn thread_current_in_dtor() {
     let name = name.as_ref().unwrap();
     assert_eq!(name, "test");
 }
+
+// Test that if a thread uses fibers while the dtors callback is running,
+// we do NOT trigger dtors.
+// This prevents a UAF if a fiber is the first to use Tls and is deleted,
+// like in the example here:
+// https://github.com/rust-lang/rust/pull/148799#issuecomment-3731806901
+#[cfg(target_os = "windows")]
+#[test]
+fn fiber_does_not_trigger_dtor() {
+    use core::ffi::c_void;
+    use std::ptr;
+
+    unsafe extern "system" {
+        fn ConvertFiberToThread() -> i32;
+        fn ConvertThreadToFiber(lpParameter: *const c_void) -> *mut c_void;
+    }
+
+    thread_local!(static FOO: UnsafeCell<Option<NotifyOnDrop>> = UnsafeCell::new(None));
+    let signal = Signal::default();
+
+    let signal2 = signal.clone();
+    let t = thread::spawn(move || unsafe {
+        let mut signal = Some(signal2);
+        FOO.with(|f| {
+            *f.get() = Some(NotifyOnDrop(signal.take().unwrap()));
+        });
+        let _main = ConvertThreadToFiber(ptr::null());
+        // A user can then switch to a new fiber and delete the main fiber.
+        // If destructors are triggered when the fiber is deleted,
+        // the new fiber will be able to observe already-destructed values.
+    });
+    t.join().unwrap();
+    assert!(!signal.is_set());
+
+    // 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);
+        FOO.with(|f| {
+            *f.get() = Some(NotifyOnDrop(signal.take().unwrap()));
+        });
+        let _ = ConvertThreadToFiber(ptr::null());
+        let _ = ConvertFiberToThread();
+    });
+    signal.wait();
+    assert!(signal.is_set());
+    t.join().unwrap();
+}

src/tools/miri/src/shims/windows/foreign_items.rs

@@ -531,6 +531,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // Return success (`1`).
                 this.write_int(1, dest)?;
             }
+            "IsThreadAFiber" => {
+                let [] = this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                
+                // Return FALSE, as this thread is not a fiber.
+                this.write_int(0, dest)?;
+            }
 
             // Access to command-line arguments
             "GetCommandLineW" => {