diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index 970c61bbe591..c00125c172a8 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -553,11 +553,28 @@ impl SingleHTTPChannel { response_buffer: core::ptr::null_mut(), } } - fn write_item(&self, item: HTTPClientResult<'static>) { - let mut g = self.slot.lock(); - *g = Some(item); - self.cv.notify_one(); + /// Publishes the result to [`read_item`](Self::read_item). `send_sync` + /// frees the channel as soon as `read_item` returns, which it can do the + /// moment the lock release inside this call lands, so the channel is taken + /// by pointer and that release is the last access to it (a `&self` would + /// still assert the allocation live while `send_sync` frees it). + /// + /// # Safety + /// `this` must be the live channel `send_sync` registered; nothing may + /// touch it after this returns. + unsafe fn write_item(this: *const Self, item: HTTPClientResult<'static>) { + // SAFETY: `*this` is live until the release inside `with_lock_raw` (fn + // contract); the signal is inside the closure so it happens while the + // lock, and therefore the channel, is still held. + unsafe { + bun_threading::Guarded::with_lock_raw(&raw const (*this).slot, |slot| { + *slot = Some(item); + (*this).cv.notify_one(); + }); + } } + /// The owner's side: `send_sync` frees the channel only after this returns, + /// so `&self` is fine here. fn read_item(&self) -> HTTPClientResult<'static> { let mut g = self.slot.lock(); loop { @@ -595,12 +612,14 @@ fn send_sync_callback( real.err = async_http.err; real.elapsed = async_http.elapsed; } - // SAFETY: `this` is the heap `SingleHTTPChannel` from `send_sync`; - // `response_buffer` is the caller's `&mut MutableString` which outlives - // `read_item`. + // SAFETY: `this` is the heap `SingleHTTPChannel` from `send_sync`, live + // until `write_item` publishes the result (`send_sync` may free it the + // moment that lands, so `write_item` is the last thing here that touches + // it); `response_buffer` is the caller's `&mut MutableString`, which + // outlives `read_item`, and it is filled in before the result is published. unsafe { result.body_into(&mut (*(*this).response_buffer).list); - (*this).write_item(result.detach_lifetime()); + SingleHTTPChannel::write_item(this, result.detach_lifetime()); } } @@ -624,13 +643,18 @@ impl<'a> AsyncHTTP<'a> { self.schedule(&mut batch); crate::HTTPThread::schedule(batch); - // `ctx` is a live heap allocation we own; the HTTP thread only touches - // it inside `send_sync_callback`, whose final action is `write_item`, - // so by the time `read_item` returns the callback has finished and no - // other reference remains. `read_item` takes `&self` (channel internals - // are interior-mutable), so a `ParentRef` shared deref is sufficient. + // `ctx` is a live heap allocation we own. The HTTP thread touches it + // only inside `send_sync_callback`, which ends by publishing the result + // through `write_item`; `read_item` cannot return before the lock + // release inside that publish, and that release is the HTTP thread's + // last access to the channel (it keeps no reference into it past that + // point), so once `read_item` returns nothing but `ctx` refers to the + // allocation. `read_item` takes `&self` (channel internals are + // interior-mutable), so a `ParentRef` shared deref is sufficient; the + // borrow ends before the free below. let result = bun_ptr::ParentRef::from(ctx).read_item(); - // SAFETY: see above — sole owner, callback completed. + // SAFETY: see above; `ctx` is the sole remaining pointer to the + // allocation `Box::new` produced above. drop(unsafe { bun_core::heap::take(ctx.as_ptr()) }); if let Some(err) = result.fail { return Err(err); diff --git a/src/threading/Futex.rs b/src/threading/Futex.rs index 3d2b23767fe2..e038d360cc87 100644 --- a/src/threading/Futex.rs +++ b/src/threading/Futex.rs @@ -55,6 +55,19 @@ 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 that may be freed before this runs: a mutex's unlock +/// tail (`Mutex::unlock_raw`), where the store that released the lock has to +/// be the primitive's last access to its own memory because the thread it +/// released may free the primitive at once. A `&AtomicU32` would assert the +/// word live until this returns. Every backend's wake side uses the address +/// only as a key and never reads the word, so this needs no `unsafe`: the +/// worst a freed or reused address yields is a spurious wakeup, which every +/// `wait()` loop tolerates. +#[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 +119,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 +175,12 @@ 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 look `address` up in the waiter table; + // they never access the memory, so it need not be live (see `super::wake_raw`). unsafe { match max_waiters { 1 => windows::ntdll::RtlWakeAddressSingle(address), @@ -261,7 +275,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 +284,10 @@ 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 the waiter lookup on `addr`; it never + // accesses the memory (hence no EFAULT below), so it need not be live + // (see `super::wake_raw`). let status = unsafe { c::__ulock_wake(flags, addr, 0) }; if status >= 0 { @@ -346,16 +362,18 @@ 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 keys the waiter lookup on the address + // alone (`get_futex_key` does not touch the memory), so `ptr` need not + // point to live memory (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 +385,13 @@ 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 + // The kernel only reports this for an address outside user space. + #[cfg(not(miri))] + linux::E::FAULT => panic!("futex_wake() returned EFAULT unexpectedly"), + // Miri reports it for a word that has already been freed, which + // `super::wake_raw` allows. + #[cfg(miri)] + linux::E::FAULT => {} _ => panic!("Unexpected futex_wake() return code"), } } @@ -427,15 +451,16 @@ 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 the waiter lookup on the address; it + // never accesses the memory, so it need not be live (see `super::wake_raw`). let rc = unsafe { libc::_umtx_op( - ptr.as_ptr().cast::(), + ptr.cast::().cast_mut(), libc::UMTX_OP_WAKE_PRIVATE, n, core::ptr::null_mut(), // there is no timeout struct @@ -480,14 +505,16 @@ 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, in-bounds) + // address; linear memory is never unmapped, so a freed word is still a + // valid key (see `super::wake_raw`). AtomicU32 has the layout of i32. let woken_count = unsafe { - core::arch::wasm32::memory_atomic_notify(ptr.as_ptr().cast::(), max_waiters) + core::arch::wasm32::memory_atomic_notify(ptr.cast::().cast_mut(), max_waiters) }; let _ = woken_count; // can be 0 when linker flag 'shared-memory' is not enabled } diff --git a/src/threading/Mutex.rs b/src/threading/Mutex.rs index c4378a0455a8..b036f6af3b81 100644 --- a/src/threading/Mutex.rs +++ b/src/threading/Mutex.rs @@ -64,7 +64,24 @@ 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: `self` is held by this thread (fn contract) and live for the + // whole call. + unsafe { Self::unlock_raw(self) } + } + + /// [`unlock`](Self::unlock) for a critical section whose exit is what lets + /// another thread free the mutex's owner (`WaitGroup::finish_raw`). A `&self` + /// argument would assert the mutex's storage, padding included, until this + /// returns; here the store that releases the lock is the last access to + /// `*this`, and the futex wake that may follow it goes by address only + /// ([`Futex::wake_raw`](crate::futex::wake_raw)). + /// + /// # Safety + /// `this` must point to a mutex this thread holds. It stays valid until the + /// lock is released; from then on another thread may free it. + 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 +202,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 +263,15 @@ 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: this thread holds the lock (fn contract), so `*this` is live + // up to 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 +303,15 @@ 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` is the exception: a reference would assert the word +// live until the call returns, but once it releases the lock another thread +// may free the word (`Mutex::unlock_raw`), so it takes the address. #[cfg(target_vendor = "apple")] unsafe extern "C" { #[cfg(debug_assertions)] safe fn os_unfair_lock_trylock(lock: &core::cell::UnsafeCell) -> bool; safe fn os_unfair_lock_lock(lock: &core::cell::UnsafeCell); - safe fn os_unfair_lock_unlock(lock: &core::cell::UnsafeCell); + fn os_unfair_lock_unlock(lock: *mut OsUnfairLock); } #[cfg(target_vendor = "apple")] @@ -302,8 +331,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: this thread holds the lock (fn contract), so `*this` is live + // up to the release inside the call, which is its last access. + unsafe { os_unfair_lock_unlock(core::cell::UnsafeCell::raw_get(&raw const (*this).oul)) } } } @@ -382,7 +414,8 @@ 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` @@ -390,11 +423,17 @@ impl FutexImpl { // // 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; the swap is what releases the lock, and the last + // access to `*this`. The wake below goes by address because the thread + // the swap releases may have freed the mutex by the time it runs. + 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 +449,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)] diff --git a/src/threading/guarded.rs b/src/threading/guarded.rs index 1dae2da61193..01ed28bf952d 100644 --- a/src/threading/guarded.rs +++ b/src/threading/guarded.rs @@ -54,6 +54,44 @@ impl GuardedBy { mutex: Mutex::new(), } } + + /// Locks `*this`, runs `f` on the protected value and releases the lock, + /// for a critical section whose release is what lets another thread free + /// the `Guarded` (typically together with the struct holding it): the + /// writer side of a one-shot handoff whose reader frees the slot as soon + /// as it has taken the value (`SingleHTTPChannel` in bun_http). + /// + /// [`lock`](Self::lock) is not usable for that: the guard's `Drop` and + /// [`Mutex::unlock`] hold references into the allocation until they + /// return, which is after the release, and a reference argument asserts + /// its memory live for the whole call (rustc marks it `dereferenceable`; + /// under the aliasing models the reader's free is rejected). Here the + /// store that releases the lock, inside [`Mutex::unlock_raw`], is the + /// last access to `*this`. + /// + /// `f` runs with the lock held, so `*this` (and whatever contains it) is + /// still live inside it; anything that has to happen before the reader + /// may free the slot, such as signalling a condition variable, belongs in + /// `f`. Nothing may touch `*this` once this returns. + /// + /// # Safety + /// `this` must point to a live `Guarded` that stays live until this + /// function releases the lock, and the calling thread must not hold it. + pub unsafe fn with_lock_raw(this: *const Self, f: impl FnOnce(&mut Value) -> R) -> R { + // SAFETY: `*this` is live (fn contract); the reference only lives for + // the `lock()` call. + unsafe { (*this).mutex.lock() }; + // SAFETY: the lock taken above serializes this against every other + // access to the value, and keeps `*this` live (fn contract) until the + // release below; `f`'s `&mut Value` does not outlive `f`. + let result = + f(unsafe { &mut *UnsafeCell::raw_get(&raw const (*this).unsynchronized_value) }); + // SAFETY: this thread holds the lock (taken above), so `*this` is live + // up to the releasing store inside the call, which is the last access + // to it. + unsafe { Mutex::unlock_raw(&raw const (*this).mutex) }; + result + } } impl GuardedBy { @@ -134,3 +172,81 @@ impl RawMutex for Mutex { Mutex::unlock(self) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::Condition; + + /// The shape of `SingleHTTPChannel` (bun_http): the reader frees the + /// channel as soon as it has taken the value, so the writer may not hold + /// anything pointing into it once `with_lock_raw` has released the lock. + struct Channel { + slot: Guarded>, + cv: Condition, + /// Plain (non-interior-mutable) field, like the channel's + /// `response_buffer` pointer: the `&self` of a by-reference writer + /// covers it too, so that shape is rejected at the reader's free even + /// though every byte the writer touches is in an `UnsafeCell`. + owner_data: usize, + } + + struct SendPtr(*const Channel); + // SAFETY: `Channel` is `Sync`; the pointer is only dereferenced while the + // pointee is live (see the test). + unsafe impl Send for SendPtr {} + + #[test] + fn reader_may_free_the_channel_once_the_raw_release_has_landed() { + // Under miri every iteration is a full interpreted thread spawn; the + // `&self` + guard shape this replaces is rejected within the first few + // dozen iterations on every seed tried. + let iterations: u32 = if cfg!(miri) { 300 } else { 10_000 }; + for i in 0..iterations { + let ch = Box::into_raw(Box::new(Channel { + slot: Guarded::new(None), + cv: Condition::new(), + owner_data: i as usize, + })); + let p = SendPtr(ch); + let writer = std::thread::spawn(move || { + let p = p; + // SAFETY: the reader can only take the value, and so free the + // channel, after this releases the lock; up to that point the + // channel is live, and after it nothing here refers to it (the + // property under test). The signal is inside the closure + // because it has to happen while the channel is still live. + unsafe { + Guarded::with_lock_raw(&raw const (*p.0).slot, |slot| { + *slot = Some(i); + (*p.0).cv.notify_one(); + }); + } + }); + // SAFETY: `ch` is live until the `Box` drop below; the writer's + // last access to it is its lock release, which `read` waits for. + let (value, owner_data) = unsafe { ((*ch).read(), (*ch).owner_data) }; + assert_eq!(value, i); + assert_eq!(owner_data, i as usize); + // SAFETY: `ch` came from `Box::into_raw`; the writer is done with it + // (it published the value we just took), so this is the sole owner. + drop(unsafe { Box::from_raw(ch) }); + writer.join().unwrap(); + } + } + + impl Channel { + /// `SingleHTTPChannel::read_item`: the reader side takes `&self` + /// because it is the owner, which frees the channel only after this + /// returns. + fn read(&self) -> u32 { + let mut slot = self.slot.lock(); + loop { + if let Some(value) = slot.take() { + return value; + } + self.cv.wait_guarded(&mut slot); + } + } + } +} diff --git a/test/internal/source-lints/handoff-publish-raw.test.ts b/test/internal/source-lints/handoff-publish-raw.test.ts new file mode 100644 index 000000000000..4da5fdbea5fe --- /dev/null +++ b/test/internal/source-lints/handoff-publish-raw.test.ts @@ -0,0 +1,264 @@ +import { file } from "bun"; +import { describe, expect, test } from "bun:test"; +import path from "path"; + +// A handoff publisher is a function whose lock release is what lets the +// thread blocked on the primitive return, and that thread then frees the +// primitive: `AsyncHTTP::send_sync` heap-allocates a `SingleHTTPChannel`, +// blocks in `read_item`, and frees the channel on the next line, so the HTTP +// thread's publish may find the channel gone the instant its release lands. +// +// Under both aliasing models a reference passed as a function argument +// (`&self` included) is protected until that function returns, and freeing +// memory a protected reference covers is rejected even if the callee never +// touches it again (self-receiver-reclaim.test.ts describes the same rule for +// frees; rustc also marks such arguments `dereferenceable`). A publisher +// written as `fn write_item(&self)` with a guard, or anything that ends in +// `Mutex::unlock(&self)`, therefore still holds references into the channel +// after the release, while the owner frees it. `cargo miri test -p +// bun_threading guarded::` rejects that shape within a few dozen iterations +// of the model in src/threading/guarded.rs. +// +// So each publisher below takes the object as a raw pointer, keeps no +// reference to it in a binding, and releases through the raw-release helper +// named in `releasesVia`, whose releasing store is the last access to the +// object; the frame that calls the publisher makes that call its last +// statement. A new handoff of this shape (a primitive freed by the thread its +// release wakes) adds its publisher, and its caller, to these tables. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); + +interface Publisher { + file: string; + /** Function name; must be defined exactly once in `file`. */ + fn: string; + /** The raw-release call the body has to go through. */ + releasesVia: string; +} + +const PUBLISHERS: Publisher[] = [ + // The primitive-level publisher: locks, mutates, then `Mutex::unlock_raw`. + { file: "src/threading/guarded.rs", fn: "with_lock_raw", releasesVia: "Mutex::unlock_raw" }, + // `send_sync`'s one-shot result slot; `send_sync` frees the channel as soon + // as `read_item` returns. + { file: "src/http/AsyncHTTP.rs", fn: "write_item", releasesVia: "Guarded::with_lock_raw" }, +]; + +interface Caller { + file: string; + /** Function name; must be defined exactly once in `file`. */ + fn: string; + /** The publisher it calls; nothing may follow that call in the body. */ + publishes: string; +} + +const CALLERS: Caller[] = [{ file: "src/http/AsyncHTTP.rs", fn: "send_sync_callback", publishes: "write_item" }]; + +// `this: *const T` / `this: *mut T` as the first parameter. A `self` receiver +// of any spelling fails this. +const RAW_THIS_PARAM = /^\s*this\s*:\s*\*\s*(?:const|mut)\b/; +// A binding that keeps a reference to the object for the rest of the frame: +// `let ch = &*this;`, `let ch = unsafe { &mut *this };`. Statement-scoped +// `(*this).field` accesses are the intended shape and do not match. +const REF_BINDING = /\blet\b[^;=]*=\s*(?:unsafe\s*\{\s*)?&\s*(?:mut\s+)?\*\s*this\b/; +// Reference-based releases: `Mutex::unlock(&self)` as a method call or a path +// call (`unlock_raw(` does not match: `(` has to follow `unlock` directly), and +// the guards whose `Drop` calls it. +const REF_RELEASE = /(?:\.|::)unlock\s*\(|\block_guard\s*\(|\bGuardedLock\b/; + +interface Fn { + params: string; + body: string; +} + +function stripComments(content: string): string { + return content.replace(/^[ \t]*\/\/.*$/gm, ""); +} + +/** Index just past the delimiter matching the opener at `open`. */ +function matchDelimiter(text: string, open: number, openCh: string, closeCh: string): number { + let depth = 0; + for (let i = open; i < text.length; i++) { + if (text[i] === openCh) depth++; + else if (text[i] === closeCh && --depth === 0) return i + 1; + } + throw new Error(`unbalanced ${openCh}${closeCh} starting at offset ${open}`); +} + +/** The single definition of `fn name` in comment-stripped Rust source. */ +function findFn(stripped: string, name: string): Fn | null { + const headers = [...stripped.matchAll(new RegExp(String.raw`\bfn\s+${name}\s*(?:<[^>]*>)?\s*\(`, "g"))]; + if (headers.length !== 1) return null; + const paramsOpen = headers[0].index + headers[0][0].length - 1; + const paramsEnd = matchDelimiter(stripped, paramsOpen, "(", ")"); + const bodyOpen = stripped.indexOf("{", paramsEnd); + const bodyEnd = matchDelimiter(stripped, bodyOpen, "{", "}"); + return { + params: stripped.slice(paramsOpen + 1, paramsEnd - 1), + // Without the outer braces. + body: stripped.slice(bodyOpen + 1, bodyEnd - 1), + }; +} + +function publisherProblems(stripped: string, { fn, releasesVia }: Publisher): string[] { + const def = findFn(stripped, fn); + if (def === null) return [`\`fn ${fn}\` is not defined exactly once`]; + const problems: string[] = []; + if (!RAW_THIS_PARAM.test(def.params)) { + problems.push(`takes \`${def.params.split(",")[0].trim()}\`; the object must arrive as \`this: *const _\``); + } + if (!def.body.includes(`${releasesVia}(`)) problems.push(`does not release through \`${releasesVia}\``); + const binding = REF_BINDING.exec(def.body); + if (binding !== null) problems.push(`keeps a reference to the object: \`${binding[0].trim()}\``); + const release = REF_RELEASE.exec(def.body); + if (release !== null) problems.push(`releases through a reference: \`${release[0].trim()}\``); + return problems; +} + +function callerProblems(stripped: string, { fn, publishes }: Caller): string[] { + const def = findFn(stripped, fn); + if (def === null) return [`\`fn ${fn}\` is not defined exactly once`]; + const call = def.body.lastIndexOf(`${publishes}(`); + if (call === -1) return [`never calls \`${publishes}\``]; + const callEnd = matchDelimiter(def.body, call + publishes.length, "(", ")"); + // After the publish only the statement's `;` and the closing braces of the + // blocks it sits in may follow: the object may already be freed by then. + const tail = def.body.slice(callEnd); + return /^\s*;?[\s}]*$/.test(tail) + ? [] + : [`\`${publishes}\` is not its last statement; this follows it: ${tail.trim()}`]; +} + +describe("publishers take the object by pointer and release through the raw path", () => { + for (const publisher of PUBLISHERS) { + test(`${publisher.file}: ${publisher.fn}`, async () => { + const stripped = stripComments(await file(path.join(root, publisher.file)).text()); + expect(publisherProblems(stripped, publisher)).toEqual([]); + }); + } +}); + +describe("the frame that publishes does nothing after the publish", () => { + for (const caller of CALLERS) { + test(`${caller.file}: ${caller.fn}`, async () => { + const stripped = stripComments(await file(path.join(root, caller.file)).text()); + expect(callerProblems(stripped, caller)).toEqual([]); + }); + } +}); + +describe("the checks recognize the shapes they claim to", () => { + const publisher: Publisher = { file: "", fn: "write_item", releasesVia: "Guarded::with_lock_raw" }; + + test("the raw shape passes", () => { + const source = ` + unsafe fn write_item(this: *const Self, item: Item) { + unsafe { + bun_threading::Guarded::with_lock_raw(&raw const (*this).slot, |slot| { + *slot = Some(item); + (*this).cv.notify_one(); + }); + } + } + `; + expect(publisherProblems(source, publisher)).toEqual([]); + }); + + test("the guard shape this replaced is reported", () => { + const source = ` + fn write_item(&self, item: Item) { + let mut g = self.slot.lock(); + *g = Some(item); + self.cv.notify_one(); + } + `; + expect(publisherProblems(source, publisher)).toEqual([ + "takes `&self`; the object must arrive as `this: *const _`", + "does not release through `Guarded::with_lock_raw`", + ]); + }); + + test("a raw receiver that still releases through a reference is reported", () => { + const source = ` + unsafe fn write_item(this: *const Self, item: Item) { + let channel = unsafe { &*this }; + let mut g = channel.slot.lock(); + *g = Some(item); + channel.cv.notify_one(); + drop(g); + Guarded::with_lock_raw(&raw const (*this).slot, |_| {}); + channel.mutex.unlock(); + } + `; + expect(publisherProblems(source, publisher)).toEqual([ + "keeps a reference to the object: `let channel = unsafe { &*this`", + "releases through a reference: `.unlock(`", + ]); + }); + + test.each([ + ["Mutex::unlock(&(*this).mutex);", "::unlock("], + ["let _guard = (*this).mutex.lock_guard();", "lock_guard("], + ["let g: GuardedLock<'_, Item, Mutex> = (*this).slot.lock();", "GuardedLock"], + ])("%s is a release through a reference", (statement, reported) => { + const source = ` + unsafe fn write_item(this: *const Self, item: Item) { + ${statement} + Guarded::with_lock_raw(&raw const (*this).slot, |_| {}); + } + `; + expect(publisherProblems(source, publisher)).toEqual([`releases through a reference: \`${reported}\``]); + }); + + test("the raw release itself is not mistaken for one", () => { + const source = ` + pub unsafe fn write_item(this: *const Self) { + unsafe { (*this).mutex.lock() }; + unsafe { Mutex::unlock_raw(&raw const (*this).mutex) }; + Guarded::with_lock_raw(&raw const (*this).slot, |_| {}); + } + `; + expect(publisherProblems(source, publisher)).toEqual([]); + }); + + test("a missing or duplicated definition is reported", () => { + expect(publisherProblems("fn read_item(&self) {}", publisher)).toEqual([ + "`fn write_item` is not defined exactly once", + ]); + expect( + publisherProblems("fn write_item(this: *const Self) {}\nfn write_item(this: *const Self) {}", publisher), + ).toEqual(["`fn write_item` is not defined exactly once"]); + }); + + const caller: Caller = { file: "", fn: "send_sync_callback", publishes: "write_item" }; + + test("a publish as the last statement passes, through nested blocks and with a wrapped argument list", () => { + const source = ` + fn send_sync_callback(this: *mut Channel, mut result: Result<'_>) { + unsafe { + result.body_into(&mut (*(*this).response_buffer).list); + Channel::write_item( + this, + result.detach_lifetime(), + ); + } + } + `; + expect(callerProblems(source, caller)).toEqual([]); + }); + + test("anything after the publish is reported", () => { + const source = ` + fn send_sync_callback(this: *mut Channel, result: Result<'_>) { + unsafe { + Channel::write_item(this, result); + (*this).done.store(true, Ordering::Release); + } + } + `; + expect(callerProblems(source, caller)).toEqual([ + expect.stringContaining("`write_item` is not its last statement; this follows it: ;"), + ]); + expect(callerProblems(source, caller)[0]).toContain("(*this).done.store(true, Ordering::Release);"); + }); +}); diff --git a/test/internal/threading-guarded-miri.test.ts b/test/internal/threading-guarded-miri.test.ts new file mode 100644 index 000000000000..551ad1342f3f --- /dev/null +++ b/test/internal/threading-guarded-miri.test.ts @@ -0,0 +1,68 @@ +/** + * `Guarded::with_lock_raw` (src/threading/guarded.rs) exists for the writer + * side of a handoff whose reader frees the `Guarded` as soon as it has taken + * the value: `AsyncHTTP::send_sync` heap-allocates a `SingleHTTPChannel`, + * blocks in `read_item` and frees the channel on the next line, so the HTTP + * thread's publish may find the channel gone the instant its lock release + * lands. A publisher written as `fn write_item(&self)` with a guard (the shape + * this replaced) still holds references into the channel at that point, and + * the aliasing models reject the reader's free for it; natively the only work + * left after the releasing store is frames returning (plus a futex wake keyed + * by the dead address when the unlock was contended), so nothing a bun-level + * test can observe distinguishes the two shapes. The discriminator is miri on + * the crate's model of the handoff, `guarded::tests`, which this runs under + * the Tree Borrows model `bun run rust:miri` pins; the `&self` shape fails it + * within a few dozen iterations. + * + * Scoped to `guarded::` rather than the whole crate because the crate's own + * `wait_group` test is still rejected on main (the same bug class, fixed + * separately); once the whole crate is clean this can become a crate-wide run. + * + * Skipped where miri is not installed or the cargo workspace is not resolvable + * (test-only CI lanes run a prebuilt binary and lack vendor/lolhtml); same + * prerequisite check as linear-fifo.test.ts and scripts/rust-miri.ts. + */ +import { expect, test } from "bun:test"; +import { existsSync } from "node:fs"; +import path from "node:path"; + +const cargoBin = Bun.which("cargo"); +const repoRoot = path.resolve(import.meta.dir, "..", ".."); +const workspaceResolvable = + existsSync(path.join(repoRoot, "vendor", "lolhtml", "Cargo.toml")) && + existsSync(path.join(repoRoot, "build", "debug", "codegen", "build_options.rs")); +const miriAvailable = + !!cargoBin && + workspaceResolvable && + Bun.spawnSync({ + cmd: [cargoBin, "miri", "--version"], + cwd: repoRoot, + stdout: "ignore", + stderr: "ignore", + timeout: 30_000, + }).exitCode === 0; + +test.skipIf(!miriAvailable)( + "the reader of a Guarded handoff may free it once with_lock_raw has released it (Tree Borrows miri)", + async () => { + await using proc = Bun.spawn({ + cmd: [cargoBin!, "miri", "test", "--locked", "-p", "bun_threading", "--", "guarded::"], + cwd: repoRoot, + env: { ...process.env, MIRIFLAGS: "-Zmiri-tree-borrows" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + if (exitCode !== 0) { + // Surface miri's diagnostic so the gate/CI log shows the actual UB. + console.error(stderr || stdout); + } + expect(stderr).not.toContain("Undefined Behavior"); + // A filter that matches nothing exits 0 too; the model has to have run. + expect(stdout).toContain("test guarded::tests::reader_may_free_the_channel_once_the_raw_release_has_landed ... ok"); + expect(exitCode).toBe(0); + }, + // Compiles the crate's dependencies for miri, then interprets 300 thread + // spawns: about 10s on a warm tree, around a minute on a cold one. + 180_000, +);