Fix observable intermediate state in thread::add_spawn_hook

library/std/src/thread/spawnhook.rs

@@ -1,7 +1,7 @@
 use super::thread::Thread;
 use crate::cell::Cell;
 use crate::iter;
-use crate::sync::Arc;
+use crate::sync::{Arc, UniqueArc};
 
 crate::thread_local! {
     /// A thread local linked list of spawn hooks.
@@ -44,10 +44,12 @@ struct SpawnHook {
 /// In other words, adding a hook has no effect on already running threads (other than the current
 /// thread) and the threads they might spawn in the future.
 ///
-/// Hooks can only be added, not removed.
-///
 /// The hooks will run in reverse order, starting with the most recently added.
 ///
+/// Hooks can only be added, not removed.
+/// Note that this does *not* mean that they are guaranteed to run. See [`Builder::no_hooks`].
+/// You cannot rely on a hook running for soundness.
+///
 /// # Usage
 ///
 /// ```
@@ -88,19 +90,23 @@ struct SpawnHook {
 ///     assert_eq!(X.get(), 123);
 /// }).join().unwrap();
 /// ```
+///
+/// [`Builder::no_hooks`]: crate::thread::Builder::no_hooks
 #[unstable(feature = "thread_spawn_hook", issue = "132951")]
 pub fn add_spawn_hook<F, G>(hook: F)
 where
     F: 'static + Send + Sync + Fn(&Thread) -> G,
     G: 'static + Send + FnOnce(),
 {
     SPAWN_HOOKS.with(|h| {
-        let mut hooks = h.take();
-        let next = hooks.first.take();
-        hooks.first = Some(Arc::new(SpawnHook {
+        // Perform all fallible operations before taking the current hooks (see #159923)
+        let mut new_first = UniqueArc::new(SpawnHook {
             hook: Box::new(move |thread| Box::new(hook(thread))),
-            next,
-        }));
+            next: None,
+        });
+        let mut hooks = h.take();
+        new_first.next = hooks.first.take();
+        hooks.first = Some(UniqueArc::into_arc(new_first));
         h.set(hooks);
     });
 }

tests/ui/std/add-spawn-hook-reentrancy-159923.rs

@@ -0,0 +1,61 @@
+// https://github.com/rust-lang/rust/issues/159923
+//@ edition:2024
+//@ run-pass
+//@ needs-threads
+
+#![feature(alloc_error_hook, thread_spawn_hook)]
+
+use std::{
+    alloc::{GlobalAlloc, Layout, System, set_alloc_error_hook},
+    sync::atomic::{AtomicBool, AtomicU32, Ordering},
+    thread,
+};
+
+struct FailingGlobalAlloc;
+unsafe impl GlobalAlloc for FailingGlobalAlloc {
+    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+        if FAIL_NEXT_ALLOC.swap(false, Ordering::Relaxed) {
+            return std::ptr::null_mut();
+        }
+        unsafe { System.alloc(layout) }
+    }
+    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+        unsafe { System.dealloc(ptr, layout) }
+    }
+}
+#[global_allocator]
+static ALLOC: FailingGlobalAlloc = FailingGlobalAlloc;
+
+static FAIL_NEXT_ALLOC: AtomicBool = AtomicBool::new(false);
+
+fn spawn_and_join() {
+    thread::scope(|scope| {
+        scope.spawn(|| {});
+    });
+}
+
+fn main() {
+    static COUNT: AtomicU32 = AtomicU32::new(0);
+
+    thread::add_spawn_hook(|_| || _ = COUNT.fetch_add(1, Ordering::Relaxed));
+    thread::add_spawn_hook(|_| || _ = COUNT.fetch_add(1, Ordering::Relaxed));
+
+    spawn_and_join();
+    spawn_and_join();
+    assert_eq!(COUNT.swap(0, Ordering::Relaxed), 4);
+
+    set_alloc_error_hook(|_| {
+        spawn_and_join();
+        assert_eq!(COUNT.swap(0, Ordering::Relaxed), 2);
+        panic!()
+    });
+
+    FAIL_NEXT_ALLOC.store(true, Ordering::Relaxed);
+    std::panic::catch_unwind(|| {
+        std::thread::add_spawn_hook(|_| || {});
+    })
+    .unwrap_err();
+
+    spawn_and_join();
+    assert_eq!(COUNT.swap(0, Ordering::Relaxed), 2);
+}