threading: finish a WaitGroup without holding a reference into it past the release

.github/workflows/miri.yml

@@ -8,7 +8,7 @@ on:
   workflow_dispatch:
   pull_request:
     paths:
-      # The FFI-free crate set covered by MIRI_CRATES in scripts/rust-miri.ts
+      # The crate set covered by MIRI_CRATES in scripts/rust-miri.ts
       - "src/ast/**"
       - "src/base64/**"
       - "src/clap/**"
@@ -22,6 +22,7 @@ on:
       - "src/ptr/**"
       - "src/resolve_builtins/**"
       - "src/shell_parser/**"
+      - "src/threading/**"
       - "src/wyhash/**"
       - "scripts/rust-miri.ts"
       - "Cargo.toml"

scripts/rust-miri.ts

@@ -1,11 +1,12 @@
 #!/usr/bin/env bun
 /**
- * `cargo miri test` for the FFI-free crate set.
+ * `cargo miri test` for the crates Miri can interpret end to end.
  *
  * Miri interprets MIR and catches UB (use-after-free, out-of-bounds,
  * uninit reads, data races, aliasing violations) at runtime. It cannot call
- * foreign functions, so this only covers the pure-Rust corner of the
- * workspace — which is also where `unsafe` density is highest.
+ * foreign functions beyond the libc subset it ships shims for, so this only
+ * covers the (nearly) pure-Rust corner of the workspace — which is also where
+ * `unsafe` density is highest.
  *
  * Aliasing model: `-Zmiri-tree-borrows`, not the default Stacked Borrows.
  * Stacked Borrows invalidates every raw pointer derived from `&mut self` the
@@ -27,9 +28,10 @@ import { resolve } from "node:path";
 const repo = resolve(import.meta.dirname, "..");
 
 // Crates that pass `cargo miri test` under Tree Borrows. To add one it must
-// (a) have at least one `#[test]`, (b) compile under `--cfg test`, (c) not
-// call into `extern "C"` at test runtime — Miri reports
-// `unsupported operation: can't call foreign function` if it does.
+// (a) have at least one `#[test]`, (b) compile under `--cfg test`, (c) at test
+// runtime only call `extern "C"` functions Miri has shims for (libc's futex and
+// thread APIs are; vendored C is not) — otherwise Miri reports
+// `unsupported operation: can't call foreign function`.
 const MIRI_CRATES = [
   "bun_ast",
   "bun_base64",
@@ -44,6 +46,7 @@ const MIRI_CRATES = [
   "bun_ptr",
   "bun_resolve_builtins",
   "bun_shell_parser",
+  "bun_threading",
   "bun_wyhash",
 ];
 

src/bundler/LinkerContext.rs

@@ -1366,9 +1366,14 @@ impl SourceMapDataTask {
         // pointee outlives every task (joined via `line_offset_wait_group`).
         let ctx = task.ctx.expect("SourceMapDataTask.ctx");
         scopeguard::defer! {
-            // Both `&self` methods (atomic ops) — safe via `ParentRef::Deref`.
             ctx.mark_pending_task_done();
-            ctx.source_maps.line_offset_wait_group.finish();
+            // SAFETY: live until this lets the linker's `wait()` return; the linker then frees
+            // the tasks at once (`generate_chunks_in_parallel`), and nothing below touches `ctx`.
+            unsafe {
+                WaitGroup::finish_raw(
+                    &raw const (*ctx.as_const_ptr()).source_maps.line_offset_wait_group,
+                )
+            };
         }
 
         // SAFETY: ctx is BundleV2.linker; container_of recovers the parent. We
@@ -1403,9 +1408,13 @@ impl SourceMapDataTask {
         // pointee outlives every task (joined via `quoted_contents_wait_group`).
         let ctx = task.ctx.expect("SourceMapDataTask.ctx");
         scopeguard::defer! {
-            // Both `&self` methods (atomic ops) — safe via `ParentRef::Deref`.
             ctx.mark_pending_task_done();
-            ctx.source_maps.quoted_contents_wait_group.finish();
+            // SAFETY: as in `run_line_offset`, for `quoted_contents_wait_group`.
+            unsafe {
+                WaitGroup::finish_raw(
+                    &raw const (*ctx.as_const_ptr()).source_maps.quoted_contents_wait_group,
+                )
+            };
         }
 
         // SAFETY: see `run_line_offset` — raw-ptr container_of, no `&mut`

src/sys/lib.rs

@@ -5535,8 +5535,8 @@ pub mod linux {
     // ThreadPool worker panics inside its idle wait.
     #[inline]
     pub unsafe fn futex_3arg(uaddr: *const u32, op: FutexOp, val: u32) -> isize {
-        // SAFETY: caller contract — `uaddr` points to a live, suitably-aligned
-        // `u32` for the syscall's duration.
+        // SAFETY: caller contract — `uaddr` is `u32`-aligned; a WAKE only uses it as
+        // a key, so it need not point to live memory.
         let rc = unsafe { libc::syscall(libc::SYS_futex, uaddr, op.raw(), val) };
         if rc == -1 {
             -(errno() as isize)

src/threading/Futex.rs

@@ -55,6 +55,13 @@ pub fn wait_forever(ptr: &AtomicU32, expect: u32) {
 /// Unblocks at most `max_waiters` callers blocked in a `wait()` call on `ptr`.
 #[cold]
 pub fn wake(ptr: &AtomicU32, max_waiters: u32) {
+    wake_raw(core::ptr::from_ref(ptr), max_waiters);
+}
+
+/// [`wake`] for a word the woken side may already have freed (`Mutex::unlock_raw`): every
+/// backend keys on the address and never reads the word, so at worst this wakes spuriously.
+#[cold]
+pub(crate) fn wake_raw(ptr: *const AtomicU32, max_waiters: u32) {
     // Avoid calling into the OS if there's nothing to wake up.
     if max_waiters == 0 {
         return;
@@ -106,7 +113,7 @@ mod unsupported_impl {
         unsupported()
     }
 
-    pub(super) fn wake(_ptr: &AtomicU32, _max_waiters: u32) {
+    pub(super) fn wake(_ptr: *const AtomicU32, _max_waiters: u32) {
         unsupported()
     }
 
@@ -162,11 +169,11 @@ mod windows_impl {
         }
     }
 
-    pub(super) fn wake(ptr: &AtomicU32, max_waiters: u32) {
-        let address: *const c_void = ptr.as_ptr().cast();
+    pub(super) fn wake(ptr: *const AtomicU32, max_waiters: u32) {
+        let address: *const c_void = ptr.cast();
         debug_assert!(max_waiters != 0);
 
-        // SAFETY: address points at a live AtomicU32.
+        // SAFETY: RtlWakeAddress* only key on `address`; it need not be live (`super::wake_raw`).
         unsafe {
             match max_waiters {
                 1 => windows::ntdll::RtlWakeAddressSingle(address),
@@ -261,7 +268,7 @@ mod darwin_impl {
         }
     }
 
-    pub(super) fn wake(ptr: &AtomicU32, max_waiters: u32) {
+    pub(super) fn wake(ptr: *const AtomicU32, max_waiters: u32) {
         let flags = c::UL {
             op: c::ULOp::COMPARE_AND_WAIT,
             no_errno: true,
@@ -270,8 +277,8 @@ mod darwin_impl {
         };
 
         loop {
-            let addr: *const c_void = ptr.as_ptr().cast();
-            // SAFETY: addr points at a live AtomicU32.
+            let addr: *const c_void = ptr.cast();
+            // SAFETY: __ulock_wake only keys on `addr`; it need not be live (`super::wake_raw`).
             let status = unsafe { c::__ulock_wake(flags, addr, 0) };
 
             if status >= 0 {
@@ -346,16 +353,17 @@ mod linux_impl {
         }
     }
 
-    pub(super) fn wake(ptr: &AtomicU32, max_waiters: u32) {
+    pub(super) fn wake(ptr: *const AtomicU32, max_waiters: u32) {
         use bun_sys::linux;
         let val: u32 = match i32::try_from(max_waiters) {
             Ok(v) => v as u32,
             Err(_) => i32::MAX as u32,
         };
-        // SAFETY: ptr.as_ptr() is a valid *const u32 for the duration of the call.
+        // SAFETY: a private FUTEX_WAKE only keys on the address (`get_futex_key`); it
+        // need not be live (see `super::wake_raw`).
         let rc = unsafe {
             linux::futex_3arg(
-                ptr.as_ptr().cast(),
+                ptr.cast(),
                 linux::FutexOp {
                     cmd: linux::FutexCmd::WAKE,
                     private: true,
@@ -367,7 +375,7 @@ mod linux_impl {
         match linux::E::init(rc) {
             linux::E::SUCCESS => {} // successful wake up
             linux::E::INVAL => {}   // invalid futex_wait() on ptr done elsewhere
-            linux::E::FAULT => panic!("futex_wake() returned EFAULT unexpectedly"), // pointer became invalid while doing the wake
+            linux::E::FAULT => {}   // word already freed (Miri reports this; see `super::wake_raw`)
             _ => panic!("Unexpected futex_wake() return code"),
         }
     }
@@ -427,15 +435,15 @@ mod freebsd_impl {
         }
     }
 
-    pub(super) fn wake(ptr: &AtomicU32, max_waiters: u32) {
+    pub(super) fn wake(ptr: *const AtomicU32, max_waiters: u32) {
         // The kernel reads n_wake as `int`; passing maxInt(u32) truncates to
         // -1 and umtxq_signal_queue's `++ret >= n_wake` returns after one
         // wakeup. _umtx_op(2): "Specify INT_MAX to wake up all waiters."
         let n: c_ulong = max_waiters.min(c_int::MAX as u32) as c_ulong;
-        // SAFETY: ptr.as_ptr() is valid for the duration of the call.
+        // SAFETY: a private WAKE only keys on the address; it need not be live (`super::wake_raw`).
         let rc = unsafe {
             libc::_umtx_op(
-                ptr.as_ptr().cast::<c_void>(),
+                ptr.cast::<c_void>().cast_mut(),
                 libc::UMTX_OP_WAKE_PRIVATE,
                 n,
                 core::ptr::null_mut(), // there is no timeout struct
@@ -480,14 +488,15 @@ mod wasm_impl {
         }
     }
 
-    pub fn wake(ptr: &AtomicU32, max_waiters: u32) {
+    pub fn wake(ptr: *const AtomicU32, max_waiters: u32) {
         #[cfg(not(target_feature = "atomics"))]
         compile_error!("WASI target missing cpu feature 'atomics'");
 
         debug_assert!(max_waiters != 0);
-        // SAFETY: ptr.as_ptr() is a valid aligned *mut i32 (AtomicU32 has the same layout).
+        // SAFETY: memory.atomic.notify only keys on the aligned address, and linear memory is
+        // never unmapped (see `super::wake_raw`); AtomicU32 has the layout of i32.
         let woken_count = unsafe {
-            core::arch::wasm32::memory_atomic_notify(ptr.as_ptr().cast::<i32>(), max_waiters)
+            core::arch::wasm32::memory_atomic_notify(ptr.cast::<i32>().cast_mut(), max_waiters)
         };
         let _ = woken_count; // can be 0 when linker flag 'shared-memory' is not enabled
     }

src/threading/Mutex.rs

@@ -64,7 +64,18 @@ impl Mutex {
     /// Releases the mutex which was previously acquired with `lock()` or `try_lock()`.
     /// It is undefined behavior if the mutex is unlocked from a different thread that it was locked from.
     pub fn unlock(&self) {
-        self.impl_.unlock()
+        // SAFETY: held by this thread (fn contract) and live for the whole call.
+        unsafe { Self::unlock_raw(self) }
+    }
+
+    /// [`unlock`](Self::unlock) for a release that lets another thread free the mutex
+    /// (`WaitGroup::finish_raw`): the releasing store is the last access to `*this`.
+    ///
+    /// # Safety
+    /// `this` must be held by this thread and stay live until the lock is released.
+    pub(crate) unsafe fn unlock_raw(this: *const Self) {
+        // SAFETY: the lock is still held, so `*this` is live (fn contract).
+        unsafe { Impl::unlock_raw(&raw const (*this).impl_) }
     }
 
     /// Debug-only check that the calling thread already holds this mutex.
@@ -185,11 +196,15 @@ impl DebugImpl {
         self.locking_thread.store(current_id, Ordering::Relaxed);
     }
 
+    /// See [`Mutex::unlock_raw`] for the contract.
     #[inline]
-    fn unlock(&self) {
-        debug_assert!(self.locking_thread.load(Ordering::Relaxed) == current_thread_id());
-        self.locking_thread.store(0, Ordering::Relaxed);
-        self.impl_.unlock();
+    unsafe fn unlock_raw(this: *const Self) {
+        // SAFETY: the lock is still held, so `*this` is live (fn contract).
+        unsafe {
+            debug_assert!((*this).locking_thread.load(Ordering::Relaxed) == current_thread_id());
+            (*this).locking_thread.store(0, Ordering::Relaxed);
+            ReleaseImpl::unlock_raw(&raw const (*this).impl_);
+        }
     }
 }
 
@@ -242,10 +257,14 @@ impl WindowsImpl {
         AcquireSRWLockExclusive(&self.srwlock)
     }
 
-    fn unlock(&self) {
-        // SAFETY: caller acquired the lock on this thread (`Mutex::unlock`
-        // contract); releasing without ownership is documented UB on Windows.
-        unsafe { bun_sys::windows::kernel32::ReleaseSRWLockExclusive(self.srwlock.get()) }
+    /// See [`Mutex::unlock_raw`] for the contract.
+    unsafe fn unlock_raw(this: *const Self) {
+        // SAFETY: held by this thread (fn contract), so `*this` is live until the release
+        // inside the call; releasing without ownership is documented UB on Windows.
+        unsafe {
+            let srwlock = core::cell::UnsafeCell::raw_get(&raw const (*this).srwlock);
+            bun_sys::windows::kernel32::ReleaseSRWLockExclusive(srwlock)
+        }
     }
 }
 
@@ -277,12 +296,13 @@ pub(crate) struct OsUnfairLock {
 // The type encodes the only pointer-validity precondition, and Apple's runtime
 // detects misuse (recursive lock / unowned unlock) by aborting — which is safe
 // — so `safe fn` discharges the link-time proof and callers need no `unsafe`.
+// `os_unfair_lock_unlock` takes the address instead: see `Mutex::unlock_raw`.
 #[cfg(target_vendor = "apple")]
 unsafe extern "C" {
     #[cfg(debug_assertions)]
     safe fn os_unfair_lock_trylock(lock: &core::cell::UnsafeCell<OsUnfairLock>) -> bool;
     safe fn os_unfair_lock_lock(lock: &core::cell::UnsafeCell<OsUnfairLock>);
-    safe fn os_unfair_lock_unlock(lock: &core::cell::UnsafeCell<OsUnfairLock>);
+    fn os_unfair_lock_unlock(lock: *mut OsUnfairLock);
 }
 
 #[cfg(target_vendor = "apple")]
@@ -302,8 +322,11 @@ impl DarwinImpl {
         os_unfair_lock_lock(&self.oul)
     }
 
-    fn unlock(&self) {
-        os_unfair_lock_unlock(&self.oul)
+    /// See [`Mutex::unlock_raw`] for the contract.
+    unsafe fn unlock_raw(this: *const Self) {
+        // SAFETY: held by this thread (fn contract), so `*this` is live until the release
+        // inside the call.
+        unsafe { os_unfair_lock_unlock(core::cell::UnsafeCell::raw_get(&raw const (*this).oul)) }
     }
 }
 
@@ -382,19 +405,24 @@ impl FutexImpl {
         }
     }
 
-    fn unlock(&self) {
+    /// See [`Mutex::unlock_raw`] for the contract.
+    unsafe fn unlock_raw(this: *const Self) {
         // Unlock the mutex and wake up a waiting thread if any.
         //
         // A waiting thread will acquire with `contended` instead of `locked`
         // which ensures that it wakes up another thread on the next unlock().
         //
         // Release barrier ensures the critical section happens before we let go of the lock
         // and that our critical section happens before the next lock holder grabs the lock.
-        let state = self.state.swap(Self::UNLOCKED, Ordering::Release);
+        //
+        // SAFETY: the lock is still held, so `*this` is live (fn contract).
+        let state_ptr = unsafe { &raw const (*this).state };
+        // SAFETY: as above; this swap is the release, and the last access to `*this`.
+        let state = unsafe { (*state_ptr).swap(Self::UNLOCKED, Ordering::Release) };
         debug_assert!(state != Self::UNLOCKED);
 
         if state == Self::CONTENDED {
-            Futex::wake(&self.state, 1);
+            Futex::wake_raw(state_ptr, 1);
         }
     }
 }
@@ -410,7 +438,7 @@ unsafe extern "C" fn Bun__lock(ptr: *mut ReleaseImpl) {
 #[unsafe(no_mangle)]
 unsafe extern "C" fn Bun__unlock(ptr: *mut ReleaseImpl) {
     // SAFETY: C caller passes a valid, initialized ReleaseImpl pointer that this thread locked.
-    unsafe { (*ptr).unlock() }
+    unsafe { ReleaseImpl::unlock_raw(ptr) }
 }
 
 #[unsafe(no_mangle)]