From 0004f1b07a2a1e9896747caa51a1120462005b6b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:50 +0000 Subject: [PATCH 01/27] threading: `Semaphore` and `Channel` keep their state in a `Guarded` `Semaphore` had a `Mutex` next to an `UnsafeCell`, and `Channel` a `Mutex` next to an `UnsafeCell` and a `Cell`. Every access to an `UnsafeCell` was an `unsafe` dereference whose reason was "the mutex is held", and three `unsafe impl Send`/`Sync` repeated it for the types. `Guarded` is that pair as one type: the same `bun_threading::Mutex`, the value behind its lock guard, and `Condition::wait_guarded` waits on the guard's own mutex. The lock is taken on entry, given up and retaken inside the wait, and released on return, as before, and the signals are still sent while it is held. `Channel::read_item` was the only reader. It went through `read_all` and `read_items` with a `[MaybeUninit; 1]` cast to `&mut [T; 1]`. It takes the `Option` of `LinearFifo::read_item` instead, which is `None` exactly when the fifo is empty, so the separate length test, the two helpers and the `debug_assert` on the count are gone. `Send` and `Sync` are derived. For `Channel` they ask for `B: Send` as well as `T: Send`; the one user has a `StaticBuffer`. The fields of `Semaphore` are laid out in another order. In the release assembly `post` differs in field offsets. `wait` goes from 54 to 61 instructions with the same branches, compares and calls: one `lea` before its wait loop, because `cond` is not at offset 0, and one more callee-saved register to save and restore. `Channel>` grows from 1064 to 1072 bytes in Linux and macOS release builds. No field is added: `State` and `Guarded` are each padded to a multiple of 8, where the flat struct was padded once. The bytes that `init_static` zeroes are no longer one run, so it has one store more. The one user makes one channel per call of `prefetch_remote_images`, a cold path. In a scratch crate `write_items` differed in field offsets only, and `read_item` made the same calls in the same order. The two `[bun_threading]` entries of `mordant-baseline.toml` counted the removed impls. They are deleted, and the section with them. --- mordant-baseline.toml | 4 -- src/threading/Semaphore.rs | 35 ++++-------- src/threading/channel.rs | 110 ++++++++++--------------------------- 3 files changed, 38 insertions(+), 111 deletions(-) diff --git a/mordant-baseline.toml b/mordant-baseline.toml index a9a5f528f695..dcaec62b4347 100644 --- a/mordant-baseline.toml +++ b/mordant-baseline.toml @@ -237,10 +237,6 @@ [bun_text_diff] "generic_body_not_generic:src/text_diff/lib.rs" = 3 -[bun_threading] -"unsafe_send_covers_more_than_pointers:src/threading/Semaphore.rs" = 1 -"unsafe_send_covers_more_than_pointers:src/threading/channel.rs" = 1 - [bun_uws_sys] "unused_pub:src/uws_sys/Loop.rs" = 2 diff --git a/src/threading/Semaphore.rs b/src/threading/Semaphore.rs index a3f41f7d19ce..f9b1f75172d5 100644 --- a/src/threading/Semaphore.rs +++ b/src/threading/Semaphore.rs @@ -5,21 +5,13 @@ //! Only the subset Bun uses is ported (`wait`/`post`); `timedWait` is omitted //! because no Rust caller needs it and it would pull in a monotonic timer. -use core::cell::UnsafeCell; - -use crate::{Condition, Mutex}; +use crate::{Condition, Guarded}; pub struct Semaphore { - mutex: Mutex, cond: Condition, - /// Guarded by `mutex`. `UnsafeCell` because `wait`/`post` take `&self`. - permits: UnsafeCell, + permits: Guarded, } -// SAFETY: `permits` is only read/written while `mutex` is held; `Mutex` and -// `Condition` are themselves `Sync`/`Send`. -unsafe impl Sync for Semaphore {} - impl Default for Semaphore { fn default() -> Self { Self::new() @@ -30,37 +22,30 @@ impl Semaphore { /// Const-init with zero permits. pub(crate) const fn new() -> Self { Self { - mutex: Mutex::new(), cond: Condition::new(), - permits: UnsafeCell::new(0), + permits: Guarded::new(0), } } /// Blocks until a permit is available, then consumes one. pub fn wait(&self) { - self.mutex.lock(); - scopeguard::defer! { self.mutex.unlock(); } + let mut permits = self.permits.lock(); - // SAFETY: `mutex` is held for every access to `permits` below. - while unsafe { *self.permits.get() } == 0 { - self.cond.wait(&self.mutex); + while *permits == 0 { + self.cond.wait_guarded(&mut permits); } - // SAFETY: `mutex` is still held (released only by the scopeguard on return). - unsafe { *self.permits.get() -= 1 }; - // SAFETY: `mutex` is still held; this is the sole accessor of `permits`. - if unsafe { *self.permits.get() } > 0 { + *permits -= 1; + if *permits > 0 { self.cond.signal(); } } /// Adds one permit and wakes one waiter. pub fn post(&self) { - self.mutex.lock(); - scopeguard::defer! { self.mutex.unlock(); } + let mut permits = self.permits.lock(); - // SAFETY: `mutex` is held. - unsafe { *self.permits.get() += 1 }; + *permits += 1; self.cond.signal(); } } diff --git a/src/threading/channel.rs b/src/threading/channel.rs index 93c6b7d3cec8..f1e0fd8b00ac 100644 --- a/src/threading/channel.rs +++ b/src/threading/channel.rs @@ -1,14 +1,11 @@ // This file contains code derived from the following source: // https://gist.github.com/kprotty/0d2dc3da4840341d6ff361b27bdac7dc#file-sync2-zig -use core::cell::{Cell, UnsafeCell}; -use core::mem::MaybeUninit; - use bun_collections::LinearFifo; use bun_collections::linear_fifo::{DynamicBuffer, LinearFifoBuffer, StaticBuffer}; use crate::Condition; -use crate::Mutex; +use crate::Guarded; #[derive(thiserror::Error, strum::IntoStaticStr, Debug, Clone, Copy, PartialEq, Eq)] pub enum ChannelError { @@ -20,27 +17,20 @@ pub enum ChannelError { bun_core::oom_from_alloc!(ChannelError); -// Channel is shared across threads through `&self`, so `buffer` is wrapped in -// `UnsafeCell` and `is_closed` in `Cell`, both accessed only while `mutex` is -// held. The buffer strategy is a `LinearFifoBuffer` trait param +struct State> { + buffer: LinearFifo, + is_closed: bool, +} + +// The buffer strategy is a `LinearFifoBuffer` trait param // (`Channel>`) with per-buffer inherent constructors // below. pub struct Channel = DynamicBuffer> { - mutex: Mutex, + state: Guarded>, putters: Condition, getters: Condition, - buffer: UnsafeCell>, - // `Cell` (not `UnsafeCell`): `bool` is `Copy`, so safe `.get()/.set()` are - // exactly the non-atomic load/store the mutex already serializes. The - // `unsafe impl Sync` below is where the cross-thread safety burden lives. - is_closed: Cell, } -// SAFETY: all interior-mutable state is guarded by `mutex`. -unsafe impl> Send for Channel {} -// SAFETY: all interior-mutable state is guarded by `mutex`. -unsafe impl> Sync for Channel {} - // Rust cannot dispatch a single `init` ident to different signatures based on // a type-level discriminant. Callers pick the matching constructor directly. @@ -56,11 +46,12 @@ impl Channel> { impl> Channel { fn with_buffer(buffer: LinearFifo) -> Self { Self { - mutex: Mutex::default(), + state: Guarded::new(State { + buffer, + is_closed: false, + }), putters: Condition::default(), getters: Condition::default(), - buffer: UnsafeCell::new(buffer), - is_closed: Cell::new(false), } } @@ -69,12 +60,18 @@ impl> Channel { } pub fn read_item(&self) -> Result { - let mut items: [MaybeUninit; 1] = [MaybeUninit::uninit()]; - // SAFETY: see try_read_item. - let slice = unsafe { &mut *items.as_mut_ptr().cast::<[T; 1]>() }; - self.read_all(slice)?; - // SAFETY: read_all() filled all slots. - Ok(unsafe { items[0].assume_init_read() }) + let mut state = self.state.lock(); + + loop { + if let Some(item) = state.buffer.read_item() { + self.putters.signal(); + return Ok(item); + } + if state.is_closed { + return Err(ChannelError::Closed); + } + self.getters.wait_guarded(&mut state); + } } pub(crate) fn write_all(&self, items: &[T]) -> Result<(), ChannelError> { @@ -83,29 +80,16 @@ impl> Channel { Ok(()) } - pub(crate) fn read_all(&self, items: &mut [T]) -> Result<(), ChannelError> { - let n = self.read_items(items, true)?; - debug_assert!(n == items.len()); - Ok(()) - } - fn write_items(&self, items: &[T], should_block: bool) -> Result { - let _guard = self.mutex.lock_guard(); + let mut state = self.state.lock(); let mut pushed: usize = 0; while pushed < items.len() { - // Re-derive the `&mut buffer` each iteration: `Condition::wait` - // below releases the mutex, so a long-lived `&mut buffer` held - // across wait() would alias another thread's `&mut` (UB). - // `is_closed` is a `Cell` so `.get()` is already a fresh load each - // iteration (cannot be hoisted past the interior-mutable wait). let did_push = 'blk: { - if self.is_closed.get() { + if state.is_closed { return Err(ChannelError::Closed); } - // SAFETY: mutex is held; this &mut does not live across wait(). - let buffer = unsafe { &mut *self.buffer.get() }; - match buffer.write(items) { + match state.buffer.write(items) { Ok(()) => {} Err(err) => { if B::DYNAMIC { @@ -121,9 +105,7 @@ impl> Channel { if did_push { pushed += 1; } else if should_block { - // wait() releases the mutex while parked, reacquires before - // returning. No long-lived UnsafeCell borrows are live here. - self.putters.wait(&self.mutex); + self.putters.wait_guarded(&mut state); } else { break; } @@ -131,40 +113,4 @@ impl> Channel { Ok(pushed) } - - fn read_items(&self, items: &mut [T], should_block: bool) -> Result { - let _guard = self.mutex.lock_guard(); - - let mut popped: usize = 0; - while popped < items.len() { - // See write_items: re-derive UnsafeCell refs each iteration so no - // borrow lives across `getters.wait()` (which releases the mutex). - let new_item: Option = 'blk: { - // SAFETY: mutex is held; this &mut does not live across wait(). - let buffer = unsafe { &mut *self.buffer.get() }; - // Buffer can contain null items but readItem will return null if the buffer is empty. - // we need to check if the buffer is empty before trying to read an item. - if buffer.readable_length() == 0 { - if self.is_closed.get() { - return Err(ChannelError::Closed); - } - break 'blk None; - } - let item = buffer.read_item(); - self.putters.signal(); - break 'blk item; - }; - - if let Some(item) = new_item { - items[popped] = item; - popped += 1; - } else if should_block { - self.getters.wait(&self.mutex); - } else { - break; - } - } - - Ok(popped) - } } From 4f928f37cb1c7d2eb820576915442cc81034956b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:51 +0000 Subject: [PATCH 02/27] threading: `Task::from_node` is a cast, and `Batch::pop` reads its own `len` `Task::from_node` was an `unsafe fn` around `from_field_ptr!(Task, node, ..)`. `node` is the first field of the `repr(C)` `Task`, which `node_of` already asserts at compile time for the opposite direction, so the subtraction is by zero. It is a safe pointer cast with the same assertion, and its three callers lose their blocks. `Thread::run` and `drain_idle_events` still dereference the result in `unsafe`. `Batch::pop` only stores it in `self.head`, and the next `pop` dereferences it. `Batch::pop(&mut self)` viewed `self.len` as an `AtomicUsize` to load it. `&mut self` is exclusive, so it is a plain read. The release assembly of `bun_threading` is the same before and after. --- src/threading/ThreadPool.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/threading/ThreadPool.rs b/src/threading/ThreadPool.rs index 4b7b695f0d22..e3138dbecf6f 100644 --- a/src/threading/ThreadPool.rs +++ b/src/threading/ThreadPool.rs @@ -455,9 +455,9 @@ impl Default for Task { impl Task { #[inline] - unsafe fn from_node(node: *mut Node) -> *mut Task { - // SAFETY: caller guarantees `node` points to the `node` field of a `Task`. - unsafe { bun_core::from_field_ptr!(Task, node, node) } + fn from_node(node: *mut Node) -> *mut Task { + const _: () = assert!(core::mem::offset_of!(Task, node) == 0); + node.cast::() } /// Project `NonNull` → `NonNull` for the intrusive `node` field. @@ -482,8 +482,7 @@ pub struct Batch { impl Batch { pub fn pop(&mut self) -> Option> { - // SAFETY: `len` is only read here for the fast-path zero check. - let len = unsafe { (*(&raw const self.len).cast::()).load(Ordering::Relaxed) }; + let len = self.len; if len == 0 { return None; } @@ -491,8 +490,7 @@ impl Batch { // SAFETY: head is non-null per the unwrap above; tasks form an intrusive list. let next = unsafe { (*Task::node_of(task).as_ptr()).next }; if !next.is_null() { - // SAFETY: next points to the `node` field of the following Task. - self.head = NonNull::new(unsafe { Task::from_node(next) }); + self.head = NonNull::new(Task::from_node(next)); } else { if task != self.tail.unwrap() { unreachable!(); @@ -1404,8 +1402,7 @@ impl Thread { } is_waking = false; - // SAFETY: result.node points to the `node` field of a Task. - let task = unsafe { Task::from_node(result.node.as_ptr()) }; + let task = Task::from_node(result.node.as_ptr()); let task_start = if stats { now_ns() } else { 0 }; // SAFETY: task is a live scheduled Task; callback contract is `unsafe fn(*mut Task)`. unsafe { ((*task).callback)(task) }; @@ -1430,8 +1427,7 @@ impl Thread { }; let mut ran = 0; while let Some(node) = consumer.pop() { - // SAFETY: node points to the `node` field of a Task. - let task = unsafe { Task::from_node(node) }; + let task = Task::from_node(node); // SAFETY: `task` was dequeued from this thread's idle queue; it is a // live scheduled `Task` whose `callback` was set by the producer. unsafe { ((*task).callback)(task) }; From daed865678cd817275e77e8c83c01262c36e926c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:51 +0000 Subject: [PATCH 03/27] threading: `IntrusiveWorkTask` and `OwnedTask` are safe traits Both were `unsafe trait`s without a promise of their own. `IntrusiveWorkTask` said its contract is inherited from its supertrait `IntrusiveField`, which stays `unsafe` and which `intrusive_field!` implements with `offset_of!`. The trait only adds two provided methods that forward to the supertrait. `OwnedTask::__callback` passes what `from_task_ptr` returns to `Box::from_raw`, so a type must not be able to replace that method in safe code: there is one blanket impl for every `IntrusiveField` instead of an impl per type, and both arms of `intrusive_work_task!` expand to `intrusive_field!` alone. `OwnedTask` said that `run` executes on a worker thread, which is the `Send` bound. `schedule_owned` gives the box away and relies on nothing else that an implementor writes: a replaced `__callback` can only leak the box unless it has `unsafe` of its own. The `unsafe impl Send` in `owned_task!` stays. No code changes: the same methods are called on the same types. --- src/io/lib.rs | 3 +-- src/threading/work_pool.rs | 30 ++++++++++-------------------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/io/lib.rs b/src/io/lib.rs index 3b8c09b99b55..f432ded0df86 100644 --- a/src/io/lib.rs +++ b/src/io/lib.rs @@ -2248,9 +2248,8 @@ pub mod closer { #[cfg(not(windows))] bun_threading::intrusive_work_task!(Closer, task); - // SAFETY: `Closer` is `Send` (`Fd` + intrusive `Task`). #[cfg(not(windows))] - unsafe impl bun_threading::work_pool::OwnedTask for Closer { + impl bun_threading::work_pool::OwnedTask for Closer { fn run(self: Box) { use bun_sys::FdExt; self.fd.close(); diff --git a/src/threading/work_pool.rs b/src/threading/work_pool.rs index fab82cce3710..efa98b213b1d 100644 --- a/src/threading/work_pool.rs +++ b/src/threading/work_pool.rs @@ -11,13 +11,9 @@ pub struct WorkPool; /// offset of that field once and provides the canonical container-of recovery /// used by every `fn(task: *mut Task)` thread-pool trampoline. /// -/// Implement via [`intrusive_work_task!`]; the trait carries the safety +/// Implement via [`intrusive_work_task!`]; the supertrait carries the safety /// contract so call sites need only assert "scheduled via this field". -/// -/// # Safety -/// Inherited from [`bun_core::IntrusiveField`]: `OFFSET` MUST equal -/// `core::mem::offset_of!(Self, )`. -pub unsafe trait IntrusiveWorkTask: bun_core::IntrusiveField { +pub trait IntrusiveWorkTask: bun_core::IntrusiveField { /// Safe accessor for the intrusive `task: Task` field /// (`&mut self.task`); [`WorkPool::schedule_owned`] uses this to install /// the callback without raw byte-offset arithmetic. @@ -33,21 +29,23 @@ pub unsafe trait IntrusiveWorkTask: bun_core::IntrusiveField { /// allocation, with provenance covering the whole allocation. #[inline(always)] unsafe fn from_task_ptr(task: *mut Task) -> *mut Self { - // SAFETY: caller upholds the trait safety contract above. + // SAFETY: caller upholds the fn contract above. unsafe { Self::from_field_ptr(task) } } } +// The only impl: `OwnedTask::__callback` trusts `from_task_ptr`, so no type may override it. +impl> IntrusiveWorkTask for T {} + /// An [`IntrusiveWorkTask`] that the [`WorkPool`] takes ownership of by value /// (`Box`). [`WorkPool::schedule_owned`] performs the `Box` → /// raw-pointer hand-off and [`__callback`](OwnedTask::__callback) recovers /// `Box` via [`IntrusiveWorkTask::from_task_ptr`], so call sites never /// touch `Box::into_raw`/`from_raw` directly. /// -/// # Safety /// [`run`](OwnedTask::run) executes on an arbitrary worker thread (hence the /// `Send` bound). -pub unsafe trait OwnedTask: IntrusiveWorkTask + Send + 'static { +pub trait OwnedTask: IntrusiveWorkTask + Send + 'static { /// Run the task. Receives ownership of the heap allocation; dropping /// `self` frees it. fn run(self: Box); @@ -68,9 +66,7 @@ pub unsafe trait OwnedTask: IntrusiveWorkTask + Send + 'static { } /// Implements [`IntrusiveWorkTask`] for a struct that embeds an intrusive -/// `task: Task` field. Expands to [`bun_core::intrusive_field!`] + a marker -/// impl; brings [`IntrusiveWorkTask::from_task_ptr`] into scope for the -/// type's `fn(*mut Task)` trampolines. +/// `task: Task` field. Expands to [`bun_core::intrusive_field!`]. /// /// ```ignore /// intrusive_work_task!(ReadFile, task); @@ -84,13 +80,9 @@ macro_rules! intrusive_work_task { // and hard-errors. ([$($gen:tt)*] $ty:ty, $field:ident) => { ::bun_core::intrusive_field!([$($gen)*] $ty, $field: $crate::work_pool::Task); - // SAFETY: `IntrusiveField` impl above supplies the offset/field. - unsafe impl<$($gen)*> $crate::work_pool::IntrusiveWorkTask for $ty {} }; ($ty:ty, $field:ident) => { ::bun_core::intrusive_field!($ty, $field: $crate::work_pool::Task); - // SAFETY: `IntrusiveField` impl above supplies the offset/field. - unsafe impl $crate::work_pool::IntrusiveWorkTask for $ty {} }; } @@ -112,8 +104,7 @@ macro_rules! owned_task { $crate::intrusive_work_task!([$($gen)*] $ty, $field); // SAFETY: see macro doc — the type is moved to a worker thread by design. unsafe impl<$($gen)*> ::core::marker::Send for $ty {} - // SAFETY: `run` forwards to the inherent `run_owned`. - unsafe impl<$($gen)*> $crate::work_pool::OwnedTask for $ty { + impl<$($gen)*> $crate::work_pool::OwnedTask for $ty { #[inline] fn run(self: ::std::boxed::Box) { <$ty>::run_owned(self) } } @@ -122,8 +113,7 @@ macro_rules! owned_task { $crate::intrusive_work_task!($ty, $field); // SAFETY: see macro doc — the type is moved to a worker thread by design. unsafe impl ::core::marker::Send for $ty {} - // SAFETY: `run` forwards to the inherent `run_owned`. - unsafe impl $crate::work_pool::OwnedTask for $ty { + impl $crate::work_pool::OwnedTask for $ty { #[inline] fn run(self: ::std::boxed::Box) { <$ty>::run_owned(self) } } From 650cee12e17dc60ed64837b23d40a2ee1bb3c105 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:51 +0000 Subject: [PATCH 04/27] alloc: `secure_zero` takes `&mut [u8]` `secure_zero(p: *mut u8, len: usize)` was an `unsafe fn`, and three of its four callers took apart bytes they held safely to call it: a `&mut [u8]` (`Data::zdeinit`), a `&mut Box<[u8]>` (`zero_sensitive`) and a `Box<[T]>` (`free_sensitive`). It takes the slice and is safe. `<[u8]>::fill` is specialised to `write_bytes`, in debug builds too, so it is still one `memset`, followed by the same `black_box` and fence. `Data::zdeinit` passes the slice it has, and `zero_sensitive` its box, which coerces. `free_sensitive` was generic over `T: Copy` and cast to bytes; its one caller has a `Box<[u8]>`, so it takes that. `free_sensitive_cstr` has only a C string, and builds the slice inside the `unsafe` block it already had. Without its type parameter `free_sensitive` is compiled once, in `bun_alloc` (20 instructions in release mode), and `Sensitive::drop` in `bun_runtime` calls it where it held a copy (21 to 6 instructions). The release assembly of `bun_sql` and `bun_s3_signing` is the same before and after. --- src/bun_alloc/lib.rs | 31 ++++++++++++------------------- src/s3_signing/credentials.rs | 3 +-- src/sql/shared/Data.rs | 4 +--- 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index 3814a283573a..79f708d5e1a0 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -907,22 +907,18 @@ pub fn range_of_slice_in_buffer(slice: &[u8], buffer: &[u8]) -> Option<[u32; 2]> } /// Zeros -/// `len` bytes at `p` in a way the optimizer cannot elide. Uses bulk +/// `bytes` in a way the optimizer cannot elide. `fill` on bytes is a bulk /// `write_bytes` (lowers to `memset`) instead of a per-byte volatile loop so /// debug builds don't pay O(len) iteration overhead — the SSLConfig leak test /// secure-zeros ~300 MiB of cert material across 1200 iterations and the /// per-byte loop alone took ~3 s in debug. `black_box` on the pointer after /// the memset forces the compiler to assume the zeroed region is observed, /// preventing dead-store elimination in release builds. -/// -/// # Safety -/// `p` must be valid for writes of `len` bytes. #[inline] -pub unsafe fn secure_zero(p: *mut u8, len: usize) { - // SAFETY: caller contract. - unsafe { core::ptr::write_bytes(p, 0, len) }; - // Treat `p` as escaped so the preceding stores cannot be eliminated. - core::hint::black_box(p); +pub fn secure_zero(bytes: &mut [u8]) { + bytes.fill(0); + // Treat the pointer as escaped so the preceding stores cannot be eliminated. + core::hint::black_box(bytes.as_mut_ptr()); core::sync::atomic::compiler_fence(core::sync::atomic::Ordering::SeqCst); } @@ -931,14 +927,8 @@ pub unsafe fn secure_zero(p: *mut u8, len: usize) { /// allocator reuses it. Zero it before dropping. /// /// Uses [`secure_zero`] so the zeroing cannot be elided by the optimizer. -pub fn free_sensitive(mut slice: Box<[T]>) { - // SAFETY: `slice` is exclusively owned; writing `size_of_val` zero bytes - // over its storage is sound for `T: Copy` (no drop glue, no invariants on - // the bit pattern we're discarding). - unsafe { - let len = core::mem::size_of_val::<[T]>(&slice); - secure_zero(slice.as_mut_ptr().cast::(), len); - } +pub fn free_sensitive(mut slice: Box<[u8]>) { + secure_zero(&mut slice); drop(slice); } @@ -956,10 +946,13 @@ pub unsafe fn free_sensitive_cstr(p: *const core::ffi::c_char) { // SAFETY: p is a NUL-terminated `default_alloc::malloc`'d buffer per // `dupe_z` contract. An interior NUL truncating `strlen` only shortens the // zero pass — the free is still exact (`mi_free`/`libc::free` are - // size-agnostic). + // size-agnostic). The caller gives the allocation up, so the `&mut [u8]` is exclusive. unsafe { let len = libc::strlen(p); - secure_zero(p.cast::().cast_mut(), len); + secure_zero(core::slice::from_raw_parts_mut( + p.cast::().cast_mut(), + len, + )); crate::default_alloc::free(p.cast::().cast_mut()); } } diff --git a/src/s3_signing/credentials.rs b/src/s3_signing/credentials.rs index f588da3e1361..eaeb065840f9 100644 --- a/src/s3_signing/credentials.rs +++ b/src/s3_signing/credentials.rs @@ -1027,8 +1027,7 @@ impl Drop for SignResult { #[inline] fn zero_sensitive(b: &mut Box<[u8]>) { - // SAFETY: `b` is exclusively borrowed; `len` bytes are valid for writes. - unsafe { bun_core::secure_zero(b.as_mut_ptr(), b.len()) }; + bun_core::secure_zero(b); } // ────────────────────────────────────────────────────────────────────────── diff --git a/src/sql/shared/Data.rs b/src/sql/shared/Data.rs index 8019d54adef8..6a4eaffa4ae3 100644 --- a/src/sql/shared/Data.rs +++ b/src/sql/shared/Data.rs @@ -55,9 +55,7 @@ impl Data { match self { Data::Owned(owned) => { // Zero bytes before freeing. - let s = owned.slice_mut(); - // SAFETY: `s` is an exclusive `&mut [u8]`; `len` bytes valid for writes. - unsafe { bun_alloc::secure_zero(s.as_mut_ptr(), s.len()) }; + bun_alloc::secure_zero(owned.slice_mut()); owned.clear_and_free(); } Data::Temporary(_) => {} From 9500c266573df0b2a4c4eda551f21aa42f2098fa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:52 +0000 Subject: [PATCH 05/27] collections: drop `VecExt::from_bump_slice` and `VecExt::writable_slice` The two `unsafe fn`s had three callers between them, all in the bundler, and `Vec` serves each of them. The sqlite loader in `ParseTask` filled two arena slices with defaults, overwrote them, and had `from_bump_slice` copy them into new `Vec`s. It builds the `Vec`s from the values: `init_one` for the property and `from_arena_slice` for the two arguments, the form `require_embedded_asset` above uses for its one argument. The `Expr::init` calls keep their order. Two arena allocations, their default values and the drop of the default property are gone. `generate_symbol_import_and_use` took an uninitialised tail from `writable_slice` and assigned to it in a zip loop. `extend` over a mapped slice iterator is `TrustedLen`: one reserve, raw writes and one store of the length, which comes after the loop instead of before it. The closure is `move` so that it holds the index by value even where `extend` is not inlined. `Dependency.source_index` takes `source_index_to_import_from` as it is: `crate::Index` is `bun_ast::Index`, so `Index::init(x.get())` was an identity. The parameter was `impl Into`, and every caller passes an `Index`, so it is `Index`. In release mode the function goes from 257 to 269 instructions, with the same 29 branches, 3 compares more and one call fewer: the loops are vectorised and unrolled as before with other induction variables, and the panic for a slice index out of range is gone. Eleven comments named the two methods: six in `vec_ext.rs` and one each in `ast_alloc.rs`, `bun_core/lib.rs`, `computeCrossChunkDependencies.rs`, `renameSymbolsInChunk.rs` and `p.rs`. The last two were already wrong about the code below them. --- src/bun_alloc/ast_alloc.rs | 5 +- src/bun_core/lib.rs | 2 - src/bundler/LinkerGraph.rs | 25 ++----- src/bundler/ParseTask.rs | 13 ++-- .../computeCrossChunkDependencies.rs | 4 +- .../linker_context/renameSymbolsInChunk.rs | 2 +- src/collections/vec_ext.rs | 70 ++----------------- src/js_parser/p.rs | 2 - 8 files changed, 19 insertions(+), 104 deletions(-) diff --git a/src/bun_alloc/ast_alloc.rs b/src/bun_alloc/ast_alloc.rs index c8321d11fc71..33f17a2a13f4 100644 --- a/src/bun_alloc/ast_alloc.rs +++ b/src/bun_alloc/ast_alloc.rs @@ -519,10 +519,7 @@ impl AstAlloc { v } - /// Move `items` element-wise into a fresh AST-heap allocation. Replaces - /// both `VecExt::from_owned_slice` (`Box<[T]>` → `Vec`) and - /// `VecExt::from_bump_slice` (leaked `&mut [T]` → `Vec`): in either case - /// the source storage is on the wrong heap, so a copy is unavoidable. + /// Moves the items of `iter` into a fresh AST-heap allocation. #[inline] pub fn vec_from_iter>(iter: I) -> AstVec { let iter = iter.into_iter(); diff --git a/src/bun_core/lib.rs b/src/bun_core/lib.rs index 4be2f185b9f0..801c75d93fe3 100644 --- a/src/bun_core/lib.rs +++ b/src/bun_core/lib.rs @@ -372,8 +372,6 @@ pub mod vec { /// Reserve `additional`, advance `len` by `additional`, and return the /// newly-exposed (uninitialized) tail. - /// Generic free-fn form of `bun_collections::VecExt::writable_slice` so - /// `bun_core::string` can call it without a `bun_collections` edge. /// /// # Safety /// Caller must fully write the returned slice before any read of diff --git a/src/bundler/LinkerGraph.rs b/src/bundler/LinkerGraph.rs index 8607257a3b40..2204c2f60b6a 100644 --- a/src/bundler/LinkerGraph.rs +++ b/src/bundler/LinkerGraph.rs @@ -412,12 +412,8 @@ pub(crate) fn generate_symbol_import_and_use( part_index: u32, ref_: Ref, use_count: u32, - // Note: callers are split between `crate::Index` (options_types) - // and the structurally identical `bun_ast::Index` until the two newtypes - // unify. Accept either via `Into` and normalize once. - source_index_to_import_from: impl Into, + source_index_to_import_from: Index, ) -> Result<(), bun_alloc::AllocError> { - let source_index_to_import_from: Index = source_index_to_import_from.into(); if use_count == 0 { return Ok(()); } @@ -471,19 +467,10 @@ pub(crate) fn generate_symbol_import_and_use( ); let dependencies = &mut parts[source_index as usize].as_mut_slice()[part_index as usize].dependencies; - // SAFETY: every element of `new_dependencies` is overwritten in the - // zip-loop immediately below before any read/drop. - let new_dependencies = unsafe { dependencies.writable_slice(part_ids.len()) }; - debug_assert_eq!(part_ids.len(), new_dependencies.len()); - for (part_id, dependency) in part_ids.iter().zip(new_dependencies.iter_mut()) { - *dependency = Dependency { - // Note: `Dependency.source_index` is the structurally - // identical `bun_ast::Index`; convert by value until the - // two `Index` newtypes unify. - source_index: bun_ast::Index::init(source_index_to_import_from.get()), - part_index: *part_id, // already u32 - }; - } + dependencies.extend(part_ids.iter().map(move |&part_id| Dependency { + source_index: source_index_to_import_from, + part_index: part_id, + })); Ok(()) } @@ -593,7 +580,7 @@ impl<'a> LinkerGraph<'a> { part_index: u32, ref_: Ref, use_count: u32, - source_index_to_import_from: impl Into, + source_index_to_import_from: Index, ) -> Result<(), bun_alloc::AllocError> { let ast = self.ast.split_mut(); let meta = self.meta.split_mut(); diff --git a/src/bundler/ParseTask.rs b/src/bundler/ParseTask.rs index 3cd6fdc6dcd3..951939729e1b 100644 --- a/src/bundler/ParseTask.rs +++ b/src/bundler/ParseTask.rs @@ -1124,10 +1124,7 @@ pub mod parse_worker { }, Loc { start: 0 }, ); - let require_args = bump.alloc_slice_fill_default::(2); - require_args[0] = import_path; - let object_properties = bump.alloc_slice_fill_default::(1); - object_properties[0] = G::Property { + let object_property = G::Property { key: Some(Expr::init( E::String { data: b"type".into(), @@ -1144,10 +1141,9 @@ pub mod parse_worker { )), ..Default::default() }; - require_args[1] = Expr::init( + let object = Expr::init( E::Object { - // SAFETY: bump-owned slice; never grown via this Vec. - properties: unsafe { G::PropertyList::from_bump_slice(object_properties) }, + properties: G::PropertyList::init_one(object_property), is_single_line: true, ..Default::default() }, @@ -1156,8 +1152,7 @@ pub mod parse_worker { let require_call = Expr::init( E::Call { target: require_property, - // SAFETY: bump-owned slice; never grown via this Vec. - args: unsafe { bun_ast::ExprNodeList::from_bump_slice(require_args) }, + args: bun_ast::ExprNodeList::from_arena_slice(&[import_path, object]), ..Default::default() }, Loc { start: 0 }, diff --git a/src/bundler/linker_context/computeCrossChunkDependencies.rs b/src/bundler/linker_context/computeCrossChunkDependencies.rs index 3d52a49378c5..7cb7eb956ab8 100644 --- a/src/bundler/linker_context/computeCrossChunkDependencies.rs +++ b/src/bundler/linker_context/computeCrossChunkDependencies.rs @@ -555,9 +555,7 @@ fn compute_cross_chunk_dependencies_with_chunk_metas( index_sort::sort_slice_unstable_by(lazy_chunk_indices, |a, b| a.cmp(b)); let chunk = &mut chunks[chunk_index]; - // `ChunkImport.import_kind` is a `#[repr(u8)]` enum (validity - // invariant), so `writable_slice` would form `&mut [T]` over - // invalid bit patterns. Push into reserved capacity instead. + // Pushed, not written into an uninitialised `&mut [T]`: `import_kind` is an enum. chunk.cross_chunk_imports.reserve(lazy_chunk_indices.len()); for &lazy_chunk_index in lazy_chunk_indices.iter() { chunk.cross_chunk_imports.push(chunk::ChunkImport { diff --git a/src/bundler/linker_context/renameSymbolsInChunk.rs b/src/bundler/linker_context/renameSymbolsInChunk.rs index 4e21dd4c3083..7841dcacca2d 100644 --- a/src/bundler/linker_context/renameSymbolsInChunk.rs +++ b/src/bundler/linker_context/renameSymbolsInChunk.rs @@ -119,7 +119,7 @@ pub(crate) unsafe fn rename_symbols_in_chunk( }; // `symbol::Map` is not `Clone`/`Copy`. Build a non-owning shallow view via - // `from_bump_slice` so the renamer's `Map` does not free graph storage on + // `from_borrowed_slice_dangerous` so the renamer's `Map` does not free graph storage on // drop. // SAFETY: `c.graph.symbols` outlives the returned `ChunkRenamer` (both are // owned by the link step). No growth is performed on the view. Raw `*mut` diff --git a/src/collections/vec_ext.rs b/src/collections/vec_ext.rs index e6c2c1717f70..cd6b4a96d228 100644 --- a/src/collections/vec_ext.rs +++ b/src/collections/vec_ext.rs @@ -29,44 +29,11 @@ pub trait VecExt: Sized { T: Clone; fn move_from_list(list: Vec) -> Self; fn from_owned_slice(items: Box<[T]>) -> Self; - /// Arena-builder → owned `Vec`. The linker always calls - /// `transfer_ownership` afterwards (full copy), so doing the copy up-front - /// here is no worse and lets the arena round-trip disappear. - /// - /// # Safety - /// Bitwise-**moves** every element out of `items` into a fresh allocation. - /// `items` must be a leaked bump-arena slice (`into_bump_slice_mut` / - /// `alloc_slice_*`) that will *never* have its elements read or dropped - /// again — i.e. no live `Vec`/`BumpVec` may still own them. Passing - /// a slice borrowed from a container that runs element destructors yields - /// a double-drop (PTR_AUDIT.md class #1: bitwise-copy of Drop-carrying - /// type while source is still live). - unsafe fn from_bump_slice(items: &mut [T]) -> Self; - /// Safe sibling of [`from_bump_slice`] for `T: Copy` — the - /// "source must never be element-dropped again" precondition holds - /// vacuously (`Copy` ⇒ no `Drop`), so the bitwise move degenerates to a - /// plain copy and needs no `unsafe` at the call site. Takes `&[T]` - /// (read-only) since nothing is logically moved out. - /// - /// Covers the dominant js_parser pattern - /// `arena.alloc_slice_copy(&[a, b]) → unsafe { from_bump_slice(..) }` - /// (invariant: bump arena outlives the AST). Callers may pass the bump - /// slice directly, or skip the intermediate bump alloc entirely and pass - /// the stack array — both compile to one memcpy into the global heap. + /// Copies `items` into a fresh `A` allocation. fn from_arena_slice(items: &[T]) -> Self where T: Copy; - /// Safe sibling of [`from_bump_slice`]: consumes an `ArenaVec` (sole owner - /// of its elements + arena buffer), bitwise-moves every element into a - /// fresh global-allocator `Vec`, and leaks the now-logically-empty - /// arena buffer back to the bump (reclaimed on arena reset, which never - /// runs element destructors). Ownership of every `T` transfers exactly - /// once, so no double-drop and no allocator-identity confusion is - /// possible at the call site. - /// - /// Prefer this over `unsafe { from_bump_slice(v.into_bump_slice_mut()) }` - /// — it encodes the "source is leaked, never dropped again" contract in - /// the type system instead of a `// SAFETY:` comment. + /// Moves the elements of `v` into a fresh `A` allocation and frees `v`'s arena buffer. fn from_bump_vec(v: bun_alloc::ArenaVec<'_, T>) -> Self; /// Wrap a borrowed slice as a `Vec` that **must not be dropped or /// grown**. Same hazard as the original — callers wrap in `ManuallyDrop`. @@ -108,20 +75,17 @@ pub trait VecExt: Sized { /// # Safety /// Exposes `self[len..capacity]` as initialized. Every element must be /// overwritten before any read (including Drop). Prefer - /// [`unused_capacity_slice`] for `T` with validity invariants. + /// [`reserve_spare`] for `T` with validity invariants. unsafe fn expand_to_capacity(&mut self); /// # Safety /// Returns `&mut [T]` over `additional` uninitialized elements. Caller /// must fully initialize the slice before any read/drop. Prefer - /// [`unused_capacity_slice`] + `set_len` for non-POD `T`. - unsafe fn writable_slice(&mut self, additional: usize) -> &mut [T]; - /// # Safety - /// As [`writable_slice`] but uses `reserve_exact` so the allocation grows + /// [`reserve_spare`] + `set_len` for non-POD `T`. Uses `reserve_exact` so the allocation grows /// to *exactly* `len + additional`. Use when the buffer is the final /// single-shot blob (sourcemap finalize, etc.). unsafe fn writable_slice_exact(&mut self, additional: usize) -> &mut [T]; /// Reserves `additional` and returns the first `additional` slots of - /// spare capacity as `MaybeUninit`. Safe sibling of [`writable_slice`]: + /// spare capacity as `MaybeUninit`. Safe sibling of [`writable_slice_exact`]: /// caller writes some prefix then calls `set_len` (or [`uv_commit`] for /// `Vec`) to commit. Unlike `spare_capacity_mut()` the returned slice /// is exactly `additional` long, not `capacity - len`. @@ -129,7 +93,7 @@ pub trait VecExt: Sized { /// `reserve(additional)` then [`expand_to_capacity`], returning the /// freshly-exposed tail as a raw `(ptr, len)` pair — i.e. /// `(next_out, avail_out)` for C streaming APIs (zlib, brotli, zstd). - /// Unlike [`writable_slice`] this exposes the *full* over-allocated + /// Unlike [`writable_slice_exact`] this exposes the *full* over-allocated /// capacity (`cap - prev_len`), not exactly `additional`, so the FFI /// callee can use the allocator's slack. Pass `additional = 0` when the /// caller has already reserved. @@ -204,25 +168,10 @@ impl VecExt for Vec { Self::move_from_list(items.into_vec()) } #[inline] - unsafe fn from_bump_slice(items: &mut [T]) -> Self { - let mut v = Vec::with_capacity_in(items.len(), A::default()); - // SAFETY: caller contract — `items` is a leaked bump-arena slice - // (`into_bump_slice_mut`); bitwise-move elements into a fresh `A` - // allocation, leaving the arena bytes abandoned (they were already - // leaked into the bump and will never be element-dropped). - unsafe { - core::ptr::copy_nonoverlapping(items.as_ptr(), v.as_mut_ptr(), items.len()); - v.set_len(items.len()); - } - v - } - #[inline] fn from_arena_slice(items: &[T]) -> Self where T: Copy, { - // For `T: Copy` the `from_bump_slice` bitwise-move is just a memcpy and - // the source carries no destructor. let mut v = Vec::with_capacity_in(items.len(), A::default()); v.extend_from_slice(items); v @@ -356,13 +305,6 @@ impl VecExt for Vec { // before being observed. unsafe { self.set_len(self.capacity()) }; } - unsafe fn writable_slice(&mut self, additional: usize) -> &mut [T] { - self.reserve(additional); - let prev = self.len(); - // SAFETY: caller contract — slice is fully written before any read. - unsafe { self.set_len(prev + additional) }; - &mut self[prev..] - } #[inline] unsafe fn writable_slice_exact(&mut self, additional: usize) -> &mut [T] { self.reserve_exact(additional); diff --git a/src/js_parser/p.rs b/src/js_parser/p.rs index 7af9d48c2bda..b442872ed88a 100644 --- a/src/js_parser/p.rs +++ b/src/js_parser/p.rs @@ -9011,8 +9011,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O } if part.import_record_indices.is_empty() { - // Bump-arena slice; Vec::from_bump_slice marks origin - // Borrowed so Drop is a no-op (arena ownership). // The *old* value is a bitwise duplicate of // `parts[idx].import_record_indices` and may be Owned — // overwrite without running Drop, which would double-free the original. From 3aad198ee09cccbff628198fbeac4ddda0d7dc5e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:52 +0000 Subject: [PATCH 06/27] sema: the stand-ins that ignore their arguments are safe `extern "C" fn`s Eleven of the `#[no_mangle]` stand-ins for C and C++ symbols in `src/sema/standalone` (ten in `native_libraries.rs`, one in `native.rs`) were `unsafe extern "C" fn` although every parameter is unused: four return a constant and seven panic. No argument can make them misbehave, so they are safe, like `Bun__AOT__addressOfOffsetInCode` and the other stand-ins next to them that take no pointer. Nothing names them in Rust. The crates that import them declare the symbols themselves, so no caller changes, and neither does the compiled code. --- src/sema/standalone/native.rs | 2 +- src/sema/standalone/native_libraries.rs | 23 ++++++++++------------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/sema/standalone/native.rs b/src/sema/standalone/native.rs index 7d38b936e05e..9ef40c5ec242 100644 --- a/src/sema/standalone/native.rs +++ b/src/sema/standalone/native.rs @@ -1135,7 +1135,7 @@ extern "C" fn simdutf__base64_length_from_binary(len: usize, options: c_int) -> } /// Answers simdutf's INVALID_BASE64_CHARACTER: `bun_base64::decode` then uses its own decoder. #[unsafe(no_mangle)] -unsafe extern "C" fn simdutf__base64_decode_from_binary( +extern "C" fn simdutf__base64_decode_from_binary( _p: *const u8, _len: usize, _out: *mut u8, diff --git a/src/sema/standalone/native_libraries.rs b/src/sema/standalone/native_libraries.rs index 1985b5fa2c6f..217261f63fd1 100644 --- a/src/sema/standalone/native_libraries.rs +++ b/src/sema/standalone/native_libraries.rs @@ -283,19 +283,19 @@ extern "C" fn Bun__abortForRemovedFeature(feature: u16) -> ! { #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__AOT__offsetInCode(_address: *const c_void) -> i64 { +extern "C" fn Bun__AOT__offsetInCode(_address: *const c_void) -> i64 { -1 } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__AOT__frameAtReturnAddress(_return_address: *const c_void) -> i64 { +extern "C" fn Bun__AOT__frameAtReturnAddress(_return_address: *const c_void) -> i64 { -1 } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__AOT__describeAddresses( +extern "C" fn Bun__AOT__describeAddresses( _addresses: *const usize, _count: usize, _buffer: *mut u8, @@ -312,34 +312,31 @@ extern "C" fn Bun__AOT__addressOfOffsetInCode(offset: usize) -> usize { #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn BunString__fromBytes(_bytes: *const u8, _len: usize) -> bun_core::String { +extern "C" fn BunString__fromBytes(_bytes: *const u8, _len: usize) -> bun_core::String { panic!("this program has no WTF strings") } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn BunString__fromLatin1(_bytes: *const u8, _len: usize) -> bun_core::String { +extern "C" fn BunString__fromLatin1(_bytes: *const u8, _len: usize) -> bun_core::String { panic!("this program has no WTF strings") } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn BunString__fromUTF16(_units: *const u16, _len: usize) -> bun_core::String { +extern "C" fn BunString__fromUTF16(_units: *const u16, _len: usize) -> bun_core::String { panic!("this program has no WTF strings") } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn BunString__fromUTF16ToLatin1( - _units: *const u16, - _len: usize, -) -> bun_core::String { +extern "C" fn BunString__fromUTF16ToLatin1(_units: *const u16, _len: usize) -> bun_core::String { panic!("this program has no WTF strings") } #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__WTFStringImpl__destroy(_this: *const WTFStringImplStruct) { +extern "C" fn Bun__WTFStringImpl__destroy(_this: *const WTFStringImplStruct) { panic!("this program has no WTF strings") } @@ -363,7 +360,7 @@ extern "C" fn bun_remember_inherited_file_descriptors() {} #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__collateUTF8( +extern "C" fn Bun__collateUTF8( _is_numeric_and_base: bool, _a: *const u8, _a_len: usize, @@ -375,7 +372,7 @@ unsafe extern "C" fn Bun__collateUTF8( #[unsafe(no_mangle)] #[linkage = "weak"] -unsafe extern "C" fn Bun__collateUTF16( +extern "C" fn Bun__collateUTF16( _is_numeric_and_base: bool, _a: *const u16, _a_len: usize, From 0f38b7ea95d590e566ef9105a397a7cc75f5c983 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:53 +0000 Subject: [PATCH 07/27] alloc, opaque: remove dead code that carried `unsafe` `MaxHeapAllocator`: `init` sets its private `ptr` to `None` and no method allocates, so the `dealloc` in `Drop` never ran and the `unsafe impl Send` and `Sync` guarded nothing. Its only users were three locals and a per-chunk `scope()` guard in `write_output_files_to_disk`, which their own comments called placeholders. `MAX_ALIGN_T`, with its Windows and FreeBSD aarch64 arms, had no other use. That function loses three calls to the `Drop` impl and one store of `len = 0` per chunk. `unsafe impl Send`/`Sync for BSSList`: the list is only reached as a `*mut` out of the `AtomicPtr` static of `bss_singleton!`, which asks for neither. `unsafe impl Send for Zone`: `OnceLock<&'static Zone>` needs `Zone: Sync`, which stays. `unsafe trait FfiLayout` and its `unsafe impl` in `assert_ffi_layout!`: the trait was never a bound and its two constants were never read. The `const` assertions of the macro stay. The `[bun_opaque]` entry of `mordant-baseline.toml` counted the two constants and goes with them. `opaque_deref_mut_nn` had one caller, `opaque_deref_mut`, and is merged into it. Its `debug_assert` for null went: the pointer comes out of a `NonNull`. Apart from `MaxHeapAllocator::drop`, the release assembly of `bun_alloc` and `bun_core` is the same before and after. --- mordant-baseline.toml | 3 - src/bun_alloc/MaxHeapAllocator.rs | 90 ------------------- src/bun_alloc/heap_breakdown.rs | 3 - src/bun_alloc/lib.rs | 35 -------- src/bun_core/lib.rs | 2 +- .../linker_context/writeOutputFilesToDisk.rs | 13 --- src/opaque/lib.rs | 45 +--------- 7 files changed, 5 insertions(+), 186 deletions(-) delete mode 100644 src/bun_alloc/MaxHeapAllocator.rs diff --git a/mordant-baseline.toml b/mordant-baseline.toml index dcaec62b4347..ccd1f7a198d8 100644 --- a/mordant-baseline.toml +++ b/mordant-baseline.toml @@ -116,9 +116,6 @@ "bool_cluster:src/lint/estree/schema.rs" = 1 "bool_cluster:src/lint/linter/config/flat.rs" = 1 -[bun_opaque] -"unused_pub:src/opaque/lib.rs" = 2 - [bun_parsers] "generic_body_not_generic:src/parsers/xml.rs" = 1 "parallel_vecs:src/parsers/xml.rs" = 1 diff --git a/src/bun_alloc/MaxHeapAllocator.rs b/src/bun_alloc/MaxHeapAllocator.rs deleted file mode 100644 index 8421ac53cb8b..000000000000 --- a/src/bun_alloc/MaxHeapAllocator.rs +++ /dev/null @@ -1,90 +0,0 @@ -//! Single allocation only. - -use core::alloc::Layout; -use core::ptr::NonNull; - -use crate::Allocator; -use crate::MAX_ALIGN_T as MAX_ALIGN; - -/// Owns a single raw `MAX_ALIGN`-aligned buffer (a `Vec` would allocate -/// with align 1, violating the `alignment <= MAX_ALIGN` contract). -pub struct MaxHeapAllocator { - ptr: Option>, - capacity: usize, - len: usize, -} - -// SAFETY: `MaxHeapAllocator` owns its buffer exclusively; no interior shared -// state. Same Send/Sync story as `Vec`. -unsafe impl Send for MaxHeapAllocator {} -// SAFETY: `&MaxHeapAllocator` exposes no interior mutability — the raw `ptr` -// is only dereferenced via `&mut self` methods, so sharing `&Self` across -// threads cannot race on the buffer. Same `Sync` story as `Vec`. -unsafe impl Sync for MaxHeapAllocator {} - -impl MaxHeapAllocator { - pub(crate) fn reset(&mut self) { - self.len = 0; - } - - /// Borrow the allocator for a scope; `reset()` is called automatically when - /// the returned guard drops. - pub fn scope(&mut self) -> MaxHeapScope<'_> { - MaxHeapScope { inner: self } - } - - // The caller constructs `MaxHeapAllocator::init()` and obtains - // `&dyn Allocator` by borrowing the result. - pub fn init() -> Self { - Self { - ptr: None, - capacity: 0, - len: 0, - } - } -} - -/// RAII guard returned by [`MaxHeapAllocator::scope`]. Derefs to the underlying -/// allocator so callers can hand out `&mut MaxHeapAllocator` (or a derived -/// `&dyn Allocator`) for the duration of the scope, and resets it on drop. -pub struct MaxHeapScope<'a> { - inner: &'a mut MaxHeapAllocator, -} - -impl core::ops::Deref for MaxHeapScope<'_> { - type Target = MaxHeapAllocator; - fn deref(&self) -> &Self::Target { - self.inner - } -} - -impl core::ops::DerefMut for MaxHeapScope<'_> { - fn deref_mut(&mut self) -> &mut Self::Target { - self.inner - } -} - -impl Drop for MaxHeapScope<'_> { - fn drop(&mut self) { - self.inner.reset(); - } -} - -// `Allocator` is a marker trait; the vtable methods above are inherent (no -// dynamic dispatch needed for a single-allocation arena). -impl Allocator for MaxHeapAllocator {} - -impl Drop for MaxHeapAllocator { - fn drop(&mut self) { - if let Some(ptr) = self.ptr.take() { - // SAFETY: `ptr`/`capacity` describe this allocator's own buffer, - // allocated with `MAX_ALIGN` alignment. - unsafe { - std::alloc::dealloc( - ptr.as_ptr(), - Layout::from_size_align_unchecked(self.capacity, MAX_ALIGN), - ); - } - } - } -} diff --git a/src/bun_alloc/heap_breakdown.rs b/src/bun_alloc/heap_breakdown.rs index f4e56217ca81..19fc8153260c 100644 --- a/src/bun_alloc/heap_breakdown.rs +++ b/src/bun_alloc/heap_breakdown.rs @@ -25,9 +25,6 @@ bun_opaque::opaque_ffi! { // SAFETY: `malloc_zone_t` is internally synchronized by libmalloc; sharing // `&Zone` across threads is the documented usage. unsafe impl Sync for Zone {} -// SAFETY: `Zone` is an opaque libmalloc handle with no thread-affine state; the -// zone API is callable from any thread, so transferring the handle is sound. -unsafe impl Send for Zone {} impl Zone { /// # Safety diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index 79f708d5e1a0..4cdc6a75061c 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -33,30 +33,6 @@ impl Alignment { } } -// ── `max_align_t` alignment ──────────────────────────────────────────────── -// The `libc` crate does not expose `max_align_t` on every target Bun ships -// (missing on Windows MSVC and on FreeBSD aarch64), so those targets carry a -// local mirror of `max_align_t`. Remaining non-Windows targets keep -// `libc::max_align_t` (which carries `long double`, align 16 on x86_64/aarch64; -// the {f64,i64,*const ()} fallback would silently downgrade to 8). -#[cfg(windows)] -#[repr(C)] -struct MaxAlignT { - _f: f64, - _i: i64, - _p: *const (), -} -#[cfg(windows)] -pub(crate) const MAX_ALIGN_T: usize = core::mem::align_of::(); -// On AArch64 -// AAPCS64 `long double` is IEEE binary128, 16-byte aligned. The `libc` crate -// only defines `max_align_t` for FreeBSD on x86_64, so hardcode the ABI value -// for the aarch64 port. -#[cfg(all(target_os = "freebsd", target_arch = "aarch64"))] -pub(crate) const MAX_ALIGN_T: usize = 16; -#[cfg(not(any(windows, all(target_os = "freebsd", target_arch = "aarch64"))))] -pub(crate) const MAX_ALIGN_T: usize = core::mem::align_of::(); - pub struct AllocatorVTable { pub free: unsafe fn(*mut core::ffi::c_void, &mut [u8], Alignment, usize), } @@ -177,7 +153,6 @@ pub const USE_MIMALLOC: bool = cfg!(not(bun_asan)); // ── Allocator-vtable modules: per-module disposition (PORTING.md §Allocators) ── // // MimallocArena → prefer `bun_alloc::Arena` (= bumpalo::Bump) -// MaxHeapAllocator → debug-only cap (single-allocation arena) // heap_breakdown → macOS malloc_zone_* per-tag heaps (debug builds) // basic → `impl GlobalAlloc for Mimalloc` above is the canonical impl // @@ -186,8 +161,6 @@ pub const USE_MIMALLOC: bool = cfg!(not(bun_asan)); // `bun_runtime::allocators`; callers import from // there directly. // -#[path = "MaxHeapAllocator.rs"] -pub mod max_heap_allocator; pub mod stack_fallback; /// Raw alloc/free matching the `#[global_allocator]` (`mi_*` normally, libc under ASAN). @@ -255,7 +228,6 @@ pub mod default_alloc { } } -pub use max_heap_allocator::MaxHeapAllocator; pub use stack_fallback::ArenaPtr; #[path = "MimallocArena.rs"] @@ -1594,13 +1566,6 @@ pub struct BSSList { pub(crate) backing_buf: [MaybeUninit; COUNT], } -// SAFETY: `head` is a self-referential `NonNull` into `self.tail` or a heap block owned by -// `self`; all mutation goes through `self.mutex`. The raw pointer is the only `!Sync` field; -// the type is logically a mutex-guarded global singleton. -unsafe impl Send for BSSList {} -// SAFETY: see the `Send` impl directly above — all access is mutex-serialized. -unsafe impl Sync for BSSList {} - const BSS_LIST_CHUNK_SIZE: usize = 256; /// The per-store overflow-block size is `count / 4`; this shared constant must diff --git a/src/bun_core/lib.rs b/src/bun_core/lib.rs index 801c75d93fe3..48b48fa3547c 100644 --- a/src/bun_core/lib.rs +++ b/src/bun_core/lib.rs @@ -631,7 +631,7 @@ pub use bun_alloc::{ // "FFI shape invariant" tooling lives in one file. Re-exported here so callers // can write `bun_core::assert_ffi_layout!(...)` without naming `bun_opaque`. pub use Global::*; -pub use bun_opaque::{FfiLayout, assert_ffi_discr, assert_ffi_layout}; +pub use bun_opaque::{assert_ffi_discr, assert_ffi_layout}; pub use error::{Error, Error as CrateError, Result as CrateResult}; pub use ffi::{Zeroable, boxed_zeroed}; pub use result::coreutils_error_map; diff --git a/src/bundler/linker_context/writeOutputFilesToDisk.rs b/src/bundler/linker_context/writeOutputFilesToDisk.rs index 82403fa9e140..7b94aee7ea63 100644 --- a/src/bundler/linker_context/writeOutputFilesToDisk.rs +++ b/src/bundler/linker_context/writeOutputFilesToDisk.rs @@ -2,7 +2,6 @@ use crate::mal_prelude::*; use std::io::Write as _; use crate::Error; -use bun_alloc::MaxHeapAllocator; use bun_ast::Loc; use bun_core::fmt::quote; use bun_core::{String as BunString, strings}; @@ -65,13 +64,6 @@ pub(crate) fn write_output_files_to_disk( return Err(e.into()); } }; - // Optimization: when writing to disk, we can re-use the memory - // between iterations: MaxHeapAllocator retains the largest allocation. - // DynAlloc is currently `()` so the arena - // handles below are placeholders; allocation routes through global mimalloc. - let mut max_heap_allocator = MaxHeapAllocator::init(); - let mut _max_heap_allocator_source_map = MaxHeapAllocator::init(); - let mut _max_heap_allocator_inline_source_map = MaxHeapAllocator::init(); let mut pathbuf = bun_paths::path_buffer_pool::get(); // SAFETY: c points to LinkerContext which is the `linker` field of BundleV2. @@ -194,11 +186,6 @@ pub(crate) fn write_output_files_to_disk( } let _trace2 = bun_core::perf::trace("Bundler.writeChunkToDisk"); - // Reset the reusable - // buffer after each chunk. `MaxHeapAllocator::scope()` returns an RAII - // guard that resets on drop and derefs to the arena, so when - // `code_allocator` is wired up it can borrow through `_code_allocator`. - let _code_allocator = max_heap_allocator.scope(); let rel_parent = paths::resolve_path::dirname::(&chunk.final_rel_path); diff --git a/src/opaque/lib.rs b/src/opaque/lib.rs index e63b38750d22..66b60bccfb4a 100644 --- a/src/opaque/lib.rs +++ b/src/opaque/lib.rs @@ -112,29 +112,8 @@ macro_rules! opaque_ffi { // FFI layout assertions // ─────────────────────────────────────────────────────────────────────────── -/// Marker: `Self` is passed across `extern "C"` by value (or embedded in a -/// type that is) and its Rust layout has been **statically verified** against -/// the foreign side's `sizeof`/`alignof`. Implemented only via -/// [`assert_ffi_layout!`]; never `impl` by hand. -/// -/// Generic FFI helpers may bound on `T: FfiLayout` to refuse unaudited types -/// at the *call* site, not just the *decl* site. -/// -/// # Safety -/// Implementing this trait asserts that `size_of::() == C_SIZE` and -/// `align_of::() == C_ALIGN`, and that those constants match the C/C++ -/// declaration `Self` is paired with. The only sound way to discharge this -/// obligation is via [`assert_ffi_layout!`], which `const`-asserts both. -pub unsafe trait FfiLayout { - /// `sizeof(T)` on the C/C++ side. Equals `size_of::()` (asserted). - const C_SIZE: usize; - /// `alignof(T)` on the C/C++ side. Equals `align_of::()` (asserted). - const C_ALIGN: usize; -} - /// Compile-time-fail if `size_of::<$T>() != $size` or -/// `align_of::<$T>() != $align`. Also implements [`FfiLayout`] for `$T`, so -/// the assertion is the *only* way to acquire the marker. +/// `align_of::<$T>() != $align`. /// /// ```ignore /// // Mirrors C++: static_assert(sizeof(BunString) == 24 && alignof == 8) @@ -177,11 +156,6 @@ macro_rules! assert_ffi_layout { ); )+)? }; - // SAFETY: the const-asserts above are the proof obligation. - unsafe impl $crate::FfiLayout for $T { - const C_SIZE: usize = $size; - const C_ALIGN: usize = $align; - } }; // mirror-type form: assert against a bindgen'd C struct ($T:ty = $Mirror:ty $(; $($field:ident),+ $(,)?)?) => { @@ -300,17 +274,6 @@ pub unsafe fn opaque_deref_nn<'a, T>(p: *const T) -> &'a T { /// mutable borrow of zero bytes cannot overlap any other borrow). #[inline(always)] pub fn opaque_deref_mut<'a, T>(p: *mut T) -> &'a mut T { - let p = ::core::ptr::NonNull::new(p).unwrap_or_else(|| null_handle()); - // SAFETY: non-null established above. - unsafe { opaque_deref_mut_nn(p.as_ptr()) } -} - -/// Unchecked `*mut T → &mut T`. See [`opaque_deref_nn`] / [`opaque_deref_mut`]. -/// -/// # Safety -/// `p` must be non-null. -#[inline(always)] -pub unsafe fn opaque_deref_mut_nn<'a, T>(p: *mut T) -> &'a mut T { const { assert!( ::core::mem::size_of::() == 0, @@ -323,9 +286,9 @@ pub unsafe fn opaque_deref_mut_nn<'a, T>(p: *mut T) -> &'a mut T { "opaque_deref_mut: T must be align-1" ) }; - debug_assert!(!p.is_null(), "opaque_deref_mut_nn: null FFI handle"); - // SAFETY: see `opaque_deref_nn`; zero-byte `&mut` cannot alias. - unsafe { &mut *p } + let p = ::core::ptr::NonNull::new(p).unwrap_or_else(|| null_handle()); + // SAFETY: non-null established above. See `opaque_deref_nn`; zero-byte `&mut` cannot alias. + unsafe { &mut *p.as_ptr() } } /// `core`-only FFI slice/string primitives shared between `bun_core::ffi` and From dbddc132ff09d143599f94572729d1975d4febdd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:53 +0000 Subject: [PATCH 08/27] alloc: `WTFStringImplStruct::m_ref_count` is an `AtomicU32` The C++ field is `std::atomic m_refCount` (StringImpl.h:162). The Rust mirror declared it `Cell` and had a private accessor that turned the cell's address into an `&AtomicU32` with `AtomicU32::from_ptr` for `is_static`, `ref` and `deref`. The field has the atomic type itself, which has the size and alignment of `u32`, and the accessor is gone. The struct stays `!Freeze`, and `!Send` and `!Sync` through its other fields. Its doc comment said that `ref` and `deref` let C++ write the count; they do it in Rust. One line says who writes the two interior-mutable fields. `ref_count()` read the cell without an atomic. It does a relaxed load, as `StringImpl::refCount()` does (StringImpl.h:429), so it does not race with another thread's `ref` or `deref`. It has four callers: three `debug_assert`s and `Any::memory_cost`. The orderings of the other three are unchanged. The release assembly of `bun_alloc` is the same. In `bun_core` one function differs, `String::thread_isolated_copy`, which touches no count: it tests the two tags in the other order and uses one `add` more to address the 16 bytes it copies. The crates that use `bun_core::String` emit the same load, test and `lock add` for a `ref`, but the inliner chooses differently around them. In `bun_runtime`, `String::create_if_different` (52 instructions) was called at 16 places and had `String::clone` inlined. It is inlined at those places and calls `String::clone` (18 instructions). At 15 other places in `bun_runtime`, and at one each in `bun_jsc` and `bun_bundler`, `String::clone` is called where it was inlined. --- src/bun_alloc/lib.rs | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index 4cdc6a75061c..5829f2038deb 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -650,16 +650,11 @@ pub fn page_size() -> usize { /// Port of `WTFStringImplStruct` — must match WebKit's `WTF::StringImpl` layout. /// -/// `m_ref_count` / `m_hash_and_flags` are `Cell` (not bare `u32`) because -/// `r#ref`/`deref`/`ensure_hash` hand a `*const Self` derived from `&self` to -/// C++ FFI that **writes** those fields. Without `UnsafeCell` the struct is -/// `Freeze`, the `&self` borrow asserts the whole pointee is read-only, and -/// the FFI write is a Stacked-Borrows violation (LLVM may also CSE the -/// pre-/post-FFI `ref_count()` loads). `Cell` is `repr(transparent)` over -/// `UnsafeCell`, so the C ABI layout is unchanged. +/// `m_ref_count` and `m_hash_and_flags` are interior-mutable: Rust, C++ and other threads write them behind a `&Self`. #[repr(C)] pub struct WTFStringImplStruct { - pub(crate) m_ref_count: core::cell::Cell, + /// `std::atomic m_refCount` (StringImpl.h:162). + pub(crate) m_ref_count: AtomicU32, pub(crate) m_length: u32, pub m_ptr: WTFStringImplPtr, pub(crate) m_hash_and_flags: core::cell::Cell, @@ -709,25 +704,11 @@ impl WTFStringImplStruct { } #[inline] pub fn ref_count(&self) -> u32 { - self.m_ref_count.get() / Self::S_REF_COUNT_INCREMENT - } - /// Atomic view of `m_ref_count`. The C++ field is - /// `std::atomic m_refCount` (StringImpl.h:163); we model it as - /// `Cell` for the read-only accessors above but `ref`/`deref` must - /// issue real atomic RMWs to match `WTF::StringImpl::ref`/`deref` exactly. - /// `Cell` is `repr(transparent)` over `UnsafeCell` and - /// `AtomicU32` is `repr(C, align(4))` over `UnsafeCell`: same size, - /// same alignment (`m_ref_count` is the first field of a `#[repr(C)]` - /// struct so it is 4-aligned), so the in-place reborrow is sound. - #[inline(always)] - fn ref_count_atomic(&self) -> &AtomicU32 { - // SAFETY: layout-compatible reborrow of `UnsafeCell` as - // `AtomicU32`; see doc comment above. - unsafe { AtomicU32::from_ptr(self.m_ref_count.as_ptr()) } + self.m_ref_count.load(Ordering::Relaxed) / Self::S_REF_COUNT_INCREMENT } #[inline(always)] fn is_static(&self) -> bool { - let count = self.ref_count_atomic().load(Ordering::Relaxed); + let count = self.m_ref_count.load(Ordering::Relaxed); count & Self::S_REF_COUNT_FLAG_IS_STATIC_STRING != 0 } /// Inline port of `WTF::StringImpl::ref()` (StringImpl.h:1181). @@ -744,7 +725,7 @@ impl WTFStringImplStruct { return; } let old = self - .ref_count_atomic() + .m_ref_count .fetch_add(Self::S_REF_COUNT_INCREMENT, Ordering::Relaxed); debug_assert!(old > 0); // hasAtLeastOneRef — also true for static (flag bit set) debug_assert!( @@ -764,7 +745,7 @@ impl WTFStringImplStruct { return; } let old = self - .ref_count_atomic() + .m_ref_count .fetch_sub(Self::S_REF_COUNT_INCREMENT, Ordering::Relaxed); debug_assert!(old > 0); // hasAtLeastOneRef if old != Self::S_REF_COUNT_INCREMENT { @@ -835,7 +816,7 @@ impl WTFStringImplStruct { unsafe extern "C" { // `&WTFStringImplStruct` is ABI-identical to the C++ `StringImpl*` (thin // non-null pointer to a `#[repr(C)]` struct). C++-side mutation lands in - // `m_ref_count` / `m_hash_and_flags`, both `Cell`, so writes through + // `m_ref_count` / `m_hash_and_flags`, both interior-mutable, so writes through // a `&`-derived pointer are sound. The type encodes the only validity // precondition, so `safe fn` discharges the link-time proof. // `ref`/`deref` are inlined in Rust above; only the cold last-ref From ebf7030c20d0a974c40ab0dafe99dcab3d81998b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:54 +0000 Subject: [PATCH 09/27] alloc: `Mutex::lock` returns a guard that borrows the mutex `bun_alloc::Mutex::lock` transmuted the `std::sync::MutexGuard<'_, ()>` to `'static` and wrapped it in its own `MutexGuard`, so that `&mut self` methods could be called while the guard was alive. Only one place needed that: `BSSMapInner::get` calls `self.at_index(..)` under the lock. `get_or_put`, `mark_not_found`, `put` and `remove` only touch sibling fields, and the four callers that have a `*mut Self` dereference it for every access. `lock` returns `std::sync::MutexGuard<'_, ()>`. The body of `at_index` moves, with its `unsafe` block, into a private `slot_at` that takes the two fields it reads, and `at_index` and `get` call that. In `get` the compiler now checks that nothing borrows the mutex mutably while it is locked. Four comments said that the guard stores a raw pointer. It held the `std` guard, which is a reference. Those sentences are deleted. What they concluded was not true either, and is unchanged: `(*this).do_append(..)` and `(*this).append_overflow_uninit()` still form a `&mut Self` that spans `mutex` while the guard holds `&mutex`; the borrow checker does not follow borrows through a raw pointer. The locks are taken and released at the same places. The release assembly of `bun_alloc` is the same. In `bun_resolver`, where the map is instantiated, an out-of-line `slot_at` (28 instructions) stands where the out-of-line `at_index` of the `DirInfo` map (26) stood. --- src/bun_alloc/lib.rs | 60 +++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index 5829f2038deb..d0f25865d009 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -417,13 +417,6 @@ pub fn buf_print_len( } // ── RAII Mutex ──────────────────────────────────────────────────────────── -// The BSS containers below need to hold the lock across `&mut self` method calls, so -// the returned [`MutexGuard`] deliberately erases its borrow of `self` — it -// stores the `std::sync::MutexGuard` lifetime-extended to `'static` (lifetimes -// are erased at codegen, so this is a layout no-op). This is sound because -// every `Mutex` here lives inside a `'static` BSS singleton (see `instance()` -// below), so the pointee always outlives the guard. -// // LAYERING: `bun_alloc` is below `bun_threading` in the crate graph, so the // futex-backed `bun_threading::Mutex` is unavailable here; `std::sync` (itself // futex-backed since Rust 1.62) is the dependency-free stand-in. @@ -433,31 +426,13 @@ impl Mutex { Self(std::sync::Mutex::new(())) } #[inline] - pub(crate) fn lock(&self) -> MutexGuard { - let g = self - .0 + pub(crate) fn lock(&self) -> std::sync::MutexGuard<'_, ()> { + self.0 .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - // SAFETY: lifetime extension only — `std::sync::MutexGuard<'a, ()>` and - // `<'static, ()>` have identical layout. Every `bun_alloc::Mutex` lives - // in a `'static` BSS singleton, so the inner `&Mutex` the guard holds - // is in fact valid for `'static`. - let _guard = unsafe { - core::mem::transmute::, std::sync::MutexGuard<'static, ()>>( - g, - ) - }; - MutexGuard { _guard } + .unwrap_or_else(std::sync::PoisonError::into_inner) } } -/// Unlocks the paired [`Mutex`] on drop. See the type-level comment on -/// [`Mutex`] for why this erases the guard lifetime rather than borrowing. -#[must_use = "if unused the Mutex will immediately unlock"] -pub(crate) struct MutexGuard { - _guard: std::sync::MutexGuard<'static, ()>, -} - // Per PORTING.md type map: `OOM!T` / `error{OutOfMemory}!T` → `Result`. // This is the crate root, so define it here. Re-exported as `bun_core::OOM`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -1700,8 +1675,7 @@ impl BSSList { this: *mut Self, ) -> core::result::Result<*mut MaybeUninit, AllocError> { // SAFETY: `this` is live; `Mutex: Sync` so concurrent `&Mutex` formation - // is sound. `MutexGuard` stores a raw pointer (see its doc), so the - // `&mut *this` formed below does not alias a live guard borrow. + // is sound. let _guard = unsafe { (*this).mutex.lock() }; // SAFETY: the inner mutex is held, so this call has exclusive access // to `*this` (the receiver is raw precisely so nothing exclusive is @@ -1881,8 +1855,7 @@ impl BSSStringList core::result::Result<&'a mut [u8], AllocError> { // SAFETY: `this` is live; `Mutex: Sync` so concurrent `&Mutex` formation - // is sound. `MutexGuard` stores a raw pointer (see its doc), so the - // `&mut *this` formed below does not alias a live guard borrow. + // is sound. let _guard = unsafe { (*this).mutex.lock() }; // SAFETY: inner mutex held ⇒ this thread has exclusive access. let (ptr, len) = unsafe { (*this).do_append(value)? }; @@ -1960,8 +1933,7 @@ impl BSSStringList core::result::Result<&'a [u8], AllocError> { // SAFETY: `this` is live; `Mutex: Sync` so concurrent `&Mutex` formation - // is sound. `MutexGuard` stores a raw pointer (see its doc), so the - // `&mut *this` formed below does not alias a live guard borrow. + // is sound. let _guard = unsafe { (*this).mutex.lock() }; // SAFETY: inner mutex held ⇒ this thread has exclusive access. let (ptr, len) = unsafe { (*this).do_append(value)? }; @@ -2205,13 +2177,12 @@ impl pub fn get(&mut self, denormalized_key: &[u8]) -> Option<&mut ValueType> { let _key = Self::key_hash(denormalized_key); - // Hold the lock across `at_index` — + // Hold the lock across `slot_at` — // a concurrent `put()` could otherwise mutate `overflow_list`/`backing_buf` while - // we dereference `index`. `MutexGuard` holds a raw pointer (see [`Mutex`] docs), - // so it does not conflict with the `&mut self` borrow in `at_index`. + // we dereference `index`. let _guard = self.mutex.lock(); let index = self.index.get(&_key).copied()?; - self.at_index(index) + Self::slot_at(&mut self.overflow_list, &mut self.backing_buf, index) } pub fn mark_not_found(&mut self, result: Result) { @@ -2220,16 +2191,25 @@ impl } pub fn at_index(&mut self, index: IndexType) -> Option<&mut ValueType> { + Self::slot_at(&mut self.overflow_list, &mut self.backing_buf, index) + } + + /// `at_index` on the two fields it reads, for a caller that holds the guard of `mutex`. + fn slot_at<'a>( + overflow_list: &'a mut OverflowList, + backing_buf: &'a mut [MaybeUninit; COUNT], + index: IndexType, + ) -> Option<&'a mut ValueType> { if index.index() == NOT_FOUND.index() || index.index() == UNASSIGNED.index() { return None; } if index.is_overflow() { - Some(self.overflow_list.at_index_mut(index)) + Some(overflow_list.at_index_mut(index)) } else { // SAFETY: a non-sentinel, non-overflow index was assigned by `put`, which // initialized this slot via `.write()`. - Some(unsafe { self.backing_buf[index.index() as usize].assume_init_mut() }) + Some(unsafe { backing_buf[index.index() as usize].assume_init_mut() }) } } From eaccd7aa0d6f81ccd7be28b1bbc584180ac928ee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:54 +0000 Subject: [PATCH 10/27] alloc: use the reference `MaybeUninit::write` returns, and index a checked slot `OverflowListBlock::append` and `BSSMapInner::put` wrote a slot with `MaybeUninit::write`, dropped the `&mut T` it returns, and indexed the slot again for `unsafe { assume_init_mut() }`. They keep the returned reference. In `append` it borrows `items` while the sibling `used` is stored, in the same order as before. `BSSListOverflowBlock::append_uninit` returns `Err` when `index >= BSS_LIST_CHUNK_SIZE` and then did `as_mut_ptr().add(index)` in `unsafe`. `&raw mut self.data[index]` is the same address; the array has `BSS_LIST_CHUNK_SIZE` elements, so its bounds check repeats the test above it. The release assembly of `bun_alloc` is the same before and after. In `bun_resolver`, which instantiates all three, `OverflowList::append` spells one test `> 2047` instead of `>= 2048`, and the rest is the same. --- src/bun_alloc/lib.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index d0f25865d009..9161d1ebdf4b 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -1399,10 +1399,9 @@ impl OverflowListBlock { debug_assert!((self.used as usize) < COUNT); let index = self.used as usize; // Raw write — slot may be uninit; no drop glue runs. - self.items[index].write(value); + let slot = self.items[index].write(value); self.used = self.used.wrapping_add(1); - // SAFETY: just initialized on the line above. - unsafe { self.items[index].assume_init_mut() } + slot } } @@ -1564,8 +1563,7 @@ impl BSSListOverflowBlock { if index as usize >= BSS_LIST_CHUNK_SIZE { return Err(AllocError); } - // SAFETY: `index < BSS_LIST_CHUNK_SIZE` checked above. - Ok(unsafe { self.data.as_mut_ptr().add(index as usize) }) + Ok(&raw mut self.data[index as usize]) } } @@ -2245,9 +2243,7 @@ impl } else { let idx = result.index.index() as usize; // Raw write — fresh slots are uninit; no drop glue runs. - self.backing_buf[idx].write(value); - // SAFETY: just initialized on the line above. - unsafe { self.backing_buf[idx].assume_init_mut() } + self.backing_buf[idx].write(value) }; self.index.insert(result.hash, result.index); Ok(ret) From 61b1ec68cf0bc49a13193eae3a953693ba1a281f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:55 +0000 Subject: [PATCH 11/27] alloc: `Zone::init` takes `&'static CStr` Both `cfg` arms of `heap_breakdown::Zone::init` were `unsafe fn(name: *const c_char)` whose contract was "NUL-terminated and valid for the whole process". That is `&'static CStr`, so the function takes one and is safe. The macOS arm keeps its `unsafe` block for `malloc_set_zone_name` and `&*zone`. Its one caller, `get_zone!`, built the pointer from `concat!($name, "\0")` in an `unsafe` block. It passes the literal on, and the two users of `c_thunks_for_zone!` write `c"brotli"` and `c"zlib"`. Zones exist only in macOS debug builds without ASAN (`ENABLED`). There `init` receives a pointer and a length instead of a pointer, once per zone. Elsewhere the call is dead code: the release assembly of `bun_alloc`, `bun_zlib` and `bun_brotli` on Linux is the same before and after. The macOS arm passes `cargo check --target aarch64-apple-darwin`. --- src/brotli/lib.rs | 2 +- src/bun_alloc/heap_breakdown.rs | 18 +++++++----------- src/bun_alloc/lib.rs | 10 +--------- src/zlib/stream.rs | 2 +- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/brotli/lib.rs b/src/brotli/lib.rs index e6d668ff39d1..24e70609d71c 100644 --- a/src/brotli/lib.rs +++ b/src/brotli/lib.rs @@ -12,7 +12,7 @@ use c::BrotliDecoder; #[allow(non_snake_case)] pub mod BrotliAllocator { - bun_alloc::c_thunks_for_zone!("brotli"); + bun_alloc::c_thunks_for_zone!(c"brotli"); pub use malloc_size as alloc; } diff --git a/src/bun_alloc/heap_breakdown.rs b/src/bun_alloc/heap_breakdown.rs index 19fc8153260c..08250c628b2b 100644 --- a/src/bun_alloc/heap_breakdown.rs +++ b/src/bun_alloc/heap_breakdown.rs @@ -1,6 +1,6 @@ +use core::ffi::{CStr, c_void}; #[cfg(target_os = "macos")] -use core::ffi::c_uint; -use core::ffi::{c_char, c_void}; +use core::ffi::{c_char, c_uint}; #[cfg(target_os = "macos")] #[allow(non_camel_case_types)] @@ -27,24 +27,20 @@ bun_opaque::opaque_ffi! { unsafe impl Sync for Zone {} impl Zone { - /// # Safety - /// `name` must point to a NUL-terminated C string that remains valid for - /// the entire process lifetime — `malloc_set_zone_name` stores the pointer - /// (does not copy). + /// `'static`: `malloc_set_zone_name` may keep the pointer instead of copying. #[cfg(target_os = "macos")] - pub unsafe fn init(name: *const c_char) -> &'static Zone { + pub fn init(name: &'static CStr) -> &'static Zone { // SAFETY: malloc_create_zone is safe to call with (0, 0); returns a - // process-lifetime zone pointer. Caller guarantees `name` outlives the + // process-lifetime zone pointer. `name` is NUL-terminated and outlives the // process. unsafe { let zone = malloc_create_zone(0, 0); - malloc_set_zone_name(zone, name); + malloc_set_zone_name(zone, name.as_ptr()); &*zone } } #[cfg(not(target_os = "macos"))] - #[allow(clippy::missing_safety_doc)] - pub unsafe fn init(_name: *const c_char) -> &'static Zone { + pub fn init(_name: &'static CStr) -> &'static Zone { unreachable!("heap_breakdown is macOS-only; guard call sites on ENABLED") } diff --git a/src/bun_alloc/lib.rs b/src/bun_alloc/lib.rs index 9161d1ebdf4b..f0a939d2bbb7 100644 --- a/src/bun_alloc/lib.rs +++ b/src/bun_alloc/lib.rs @@ -1260,15 +1260,7 @@ macro_rules! get_zone { ($name:literal) => {{ static ZONE: ::std::sync::OnceLock<&'static $crate::heap_breakdown::Zone> = ::std::sync::OnceLock::new(); - *ZONE.get_or_init(|| { - // SAFETY: concat!($name, "\0") is a valid NUL-terminated string - // literal in static memory — valid for process lifetime. - unsafe { - $crate::heap_breakdown::Zone::init( - concat!($name, "\0").as_ptr().cast::<::core::ffi::c_char>(), - ) - } - }) + *ZONE.get_or_init(|| $crate::heap_breakdown::Zone::init($name)) }}; } diff --git a/src/zlib/stream.rs b/src/zlib/stream.rs index e95753574f67..c6ed4976f26e 100644 --- a/src/zlib/stream.rs +++ b/src/zlib/stream.rs @@ -10,7 +10,7 @@ use bun_zlib_sys::shared::{DataType, ReturnCode, uInt, uLong, zStream_struct}; #[allow(non_snake_case)] mod ZlibAllocator { - bun_alloc::c_thunks_for_zone!("zlib"); + bun_alloc::c_thunks_for_zone!(c"zlib"); pub(crate) use malloc_items as alloc; } From d0d863b8b7c51322cb55e0f9ca7f9be77c586dd7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:55 +0000 Subject: [PATCH 12/27] threading: a worker's `Thread` is only ever borrowed shared `Thread::run` took `&raw mut self_` and dereferenced it in `unsafe` to call `pop` and, twice, `drain_idle_events`, and `ThreadPool::register` stored `(*thread).next` in `unsafe`. `drain_idle_events` and every method of `Queue` and `Buffer` already take `&self`. `pop` took `&mut self` only to store `self.target`, and that `&mut` was not exclusive: while a worker is in `pop`, other workers read its `next`, `run_queue` and `run_buffer` through the pool's list. `next` and `target` are `Cell<*mut Thread>`, which keeps them at offsets 0 and 8. `next` is set before the compare-exchange that publishes the thread and only read afterwards; `target` is used by its own worker alone. Each field says so. `pop` takes `&self`, `run` calls the methods on its local, and `ThreadRegistration` holds `&'a Thread`. `register` had one caller, `ThreadRegistration::new`, and is written into it, so no safe fn leaves the address of a `&Thread` in the pool's list. `new` stays `unsafe`: a leaked guard would leave `CURRENT` and that list pointing at a dead stack frame, and `pool` has to count the calling thread in `spawned`, which `unregister` subtracts from. `unregister` takes `Option<&Thread>` instead of `*mut Thread`, which has the same ABI: `drop` passes `Some(self.thread)`, `warm` and `notify_slow` pass `None`, and the step through `NonNull` and `BackRef` is gone. The steal loop reads and writes the same fields in the same order with the same orderings: the release assembly of `Thread::run`, into which `pop` is inlined, is the same before and after. One function of `bun_threading` differs: `ThreadRegistration::drop` loses the null test of `unregister`'s `maybe_thread` (68 to 64 instructions). --- src/threading/ThreadPool.rs | 114 ++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 62 deletions(-) diff --git a/src/threading/ThreadPool.rs b/src/threading/ThreadPool.rs index e3138dbecf6f..ac8899f69b56 100644 --- a/src/threading/ThreadPool.rs +++ b/src/threading/ThreadPool.rs @@ -294,7 +294,7 @@ impl ThreadPool { if !other.run_queue.is_empty() || !other.run_buffer.is_empty() { return true; } - thread = other.next; + thread = other.next.get(); } false } @@ -358,7 +358,7 @@ impl ThreadPool { // `Thread::pop` walks the same links. let thread = bun_ptr::BackRef::from(thread); thread.push_idle_task(new_task()); - next = thread.next; + next = thread.next.get(); } self.wake_for_idle_events(); } @@ -905,10 +905,10 @@ impl ThreadPool { // Dropping JoinHandle detaches the thread. } Err(_) => { - // SAFETY: `&self` keeps the pool live; `null` thread makes + // SAFETY: `&self` keeps the pool live; `None` makes // `unregister` return right after undoing the `spawned` // increment CAS'd above (no per-thread wait past notify). - return unsafe { Self::unregister(self, ptr::null_mut()) }; + return unsafe { Self::unregister(self, None) }; } } sync = new_sync; @@ -987,10 +987,10 @@ impl ThreadPool { { exit_spawn_failed(&err); } - // SAFETY: `&self` keeps the pool live; `null` thread makes + // SAFETY: `&self` keeps the pool live; `None` makes // `unregister` return right after undoing the `spawned` // increment CAS'd above (no per-thread wait past notify). - return unsafe { Self::unregister(self, ptr::null_mut()) }; + return unsafe { Self::unregister(self, None) }; } } return; @@ -1103,30 +1103,12 @@ impl ThreadPool { } } - fn register(&self, thread: *mut Thread) { - // Push the thread onto the threads stack in a lock-free manner. - let mut threads = self.threads.load(Ordering::Relaxed); - loop { - // SAFETY: thread is the calling worker's own stack-local Thread. - unsafe { (*thread).next = threads }; - match self.threads.compare_exchange_weak( - threads, - thread, - Ordering::Release, - Ordering::Relaxed, - ) { - Ok(_) => break, - Err(cur) => threads = cur, - } - } - } - /// # Safety /// `pool` is the worker's owning pool. After `(*pool).join_event.notify()` /// the joiner may return and the pool may be **deallocated**, so this fn /// takes `*const Self` (no `&self` protector) and never touches `pool` /// past that point — the shutdown chain follows worker-stack `.next` links. - unsafe fn unregister(pool: *const Self, maybe_thread: *mut Thread) { + unsafe fn unregister(pool: *const Self, maybe_thread: Option<&Thread>) { // Un-spawn one thread, either due to a failed OS thread spawning or the thread is exiting. let one_spawned = { let mut s = Sync::zero(); @@ -1148,21 +1130,15 @@ impl ThreadPool { // ── `*pool` may be invalid past this point. ── // If this is a thread pool thread, wait for a shutdown signal by the thread pool join()er. - let Some(thread) = NonNull::new(maybe_thread) else { + let Some(thread) = maybe_thread else { return; }; - // `maybe_thread` is the calling worker's own stack-local `Thread` - // (set in `ThreadRegistration::new`); it lives on this OS thread's - // stack and outlives the entire `unregister` call. BackRef invariant - // — pointee outlives holder — covers the `join_event.wait()` and - // `.next` reads below. - let thread = bun_ptr::BackRef::from(thread); thread.join_event.wait(None); // After receiving the shutdown signal, shutdown the next thread in the pool. // We have to do that without touching the thread pool itself since its memory is invalidated by now. // So just follow our .next link. - let Some(next_thread) = NonNull::new(thread.next) else { + let Some(next_thread) = NonNull::new(thread.next.get()) else { return; }; // `next_thread` is a registered worker still blocked in @@ -1211,8 +1187,10 @@ enum WaitError { // which profiled ~43% hotter on the steal traversal. #[repr(C)] pub struct Thread { - next: *mut Thread, - target: *mut Thread, + /// Its worker sets it before the CAS that publishes the `Thread`; after that it is only read. + next: Cell<*mut Thread>, + /// Only its own worker reads or writes it, in `pop`. + target: Cell<*mut Thread>, join_event: Event, run_queue: node::Queue, idle_queue: node::Queue, @@ -1230,16 +1208,30 @@ thread_local! { /// /// `pool` is a [`BackRef`]: the pool's `join()` blocks on every registered /// worker, so it strictly outlives this guard. -struct ThreadRegistration { +struct ThreadRegistration<'a> { pool: bun_ptr::BackRef, - thread: *mut Thread, + thread: &'a Thread, } -impl ThreadRegistration { - /// SAFETY: `thread` must point to the caller's stack-local `Thread`. - unsafe fn new(pool: &ThreadPool, thread: *mut Thread) -> Self { - CURRENT.with(|c| c.set(thread)); - pool.register(thread); +impl<'a> ThreadRegistration<'a> { + /// SAFETY: `pool` counts the calling thread in `spawned`. The guard must be dropped, not + /// leaked: until then `CURRENT` and `pool` point to `thread`. + unsafe fn new(pool: &ThreadPool, thread: &'a Thread) -> Self { + CURRENT.with(|c| c.set(ptr::from_ref(thread).cast_mut())); + // Push the thread onto the threads stack in a lock-free manner. + let mut threads = pool.threads.load(Ordering::Relaxed); + loop { + thread.next.set(threads); + match pool.threads.compare_exchange_weak( + threads, + ptr::from_ref(thread).cast_mut(), + Ordering::Release, + Ordering::Relaxed, + ) { + Ok(_) => break, + Err(cur) => threads = cur, + } + } Self { pool: bun_ptr::BackRef::new(pool), thread, @@ -1247,12 +1239,13 @@ impl ThreadRegistration { } } -impl Drop for ThreadRegistration { +impl Drop for ThreadRegistration<'_> { fn drop(&mut self) { - // SAFETY: per `new()` contract. `unregister` takes `*const` (not the + // SAFETY: `pool` is the pool this worker is registered with and counted in + // (`new`'s contract). `unregister` takes `*const` (not the // `BackRef`) because the pool may be freed by the joiner before it // returns — see `unregister`'s doc. - unsafe { ThreadPool::unregister(self.pool.as_const_ptr(), self.thread) }; + unsafe { ThreadPool::unregister(self.pool.as_const_ptr(), Some(self.thread)) }; CURRENT.with(|c| c.set(ptr::null_mut())); } } @@ -1353,21 +1346,21 @@ impl Thread { } } - let mut self_ = Thread { - next: ptr::null_mut(), - target: ptr::null_mut(), + let self_ = Thread { + next: Cell::new(ptr::null_mut()), + target: Cell::new(ptr::null_mut()), join_event: Event::default(), run_queue: node::Queue::default(), idle_queue: node::Queue::default(), run_buffer: node::Buffer::default(), thread_pool, }; - let self_ptr: *mut Thread = &raw mut self_; // `BackRef` invariant: pool's `join()` waits for every worker, so the // pointee outlives this fn. Hoist a single shared ref for the hot loop. let pool: &ThreadPool = thread_pool.get(); - // SAFETY: self_ptr is our stack-local Thread. - let _registration = unsafe { ThreadRegistration::new(pool, self_ptr) }; + // SAFETY: `warm` and `notify_slow` count a thread in `spawned` before they start it. The + // guard is a local declared after `self_`, so it is dropped first. + let _registration = unsafe { ThreadRegistration::new(pool, &self_) }; let stats = stats_enabled(); let mut is_waking = false; @@ -1384,8 +1377,7 @@ impl Thread { // worker thread tears down its own `Worker`/`WorkerData` // (whose `ThreadLocalArena` is mimalloc thread-local and // must be freed here, not from the bundler thread). - // SAFETY: self_ptr is our own stack-local Thread. - unsafe { (*self_ptr).drain_idle_events() }; + self_.drain_idle_events(); return; } }; @@ -1395,8 +1387,7 @@ impl Thread { .fetch_add(now_ns().wrapping_sub(wait_start), Ordering::Relaxed); } - // SAFETY: self_ptr is our own stack-local Thread. - while let Some(result) = unsafe { (*self_ptr).pop(pool) } { + while let Some(result) = self_.pop(pool) { if result.pushed || is_waking { pool.notify(is_waking); } @@ -1415,8 +1406,7 @@ impl Thread { } Output::flush(); - // SAFETY: self_ptr is our own stack-local Thread. - unsafe { (*self_ptr).drain_idle_events() }; + self_.drain_idle_events(); } } @@ -1443,7 +1433,7 @@ impl Thread { /// already proved liveness once (`join()` waits on every registered /// worker), so the per-access raw-pointer derefs that the `*const` /// signature forced are gone. - pub(crate) fn pop(&mut self, thread_pool: &ThreadPool) -> Option { + pub(crate) fn pop(&self, thread_pool: &ThreadPool) -> Option { // Check our local buffer first if let Some(node) = self.run_buffer.pop() { return Some(node::Stole { @@ -1466,8 +1456,8 @@ impl Thread { let mut num_threads = thread_pool.sync.load(Ordering::Relaxed).spawned(); while num_threads > 0 { // Traverse the stack of registered threads on the thread pool - let target = if !self.target.is_null() { - self.target + let target = if !self.target.get().is_null() { + self.target.get() } else { let t = thread_pool.threads.load(Ordering::Acquire); if t.is_null() { @@ -1476,7 +1466,7 @@ impl Thread { t }; // SAFETY: target is a registered Thread in the lock-free stack. - self.target = unsafe { (*target).next }; + self.target.set(unsafe { (*target).next.get() }); // Try to steal from their queue first to avoid contention (the target steal's from queue last). // SAFETY: target is a registered Thread in the lock-free stack, alive until join(). @@ -1486,7 +1476,7 @@ impl Thread { // Skip stealing from the buffer if we're the target. // We still steal from our own queue above given it may have just been locked the first time we tried. - if target == std::ptr::from_mut::(self) { + if ptr::eq(target, self) { num_threads -= 1; continue; } From 039a1189b6aba4a53e3fcef0f1043d863a75df57 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:56 +0000 Subject: [PATCH 13/27] threading: `Bun__lock` takes `&ReleaseImpl` `Bun__lock` was `unsafe extern "C" fn(ptr: *mut ReleaseImpl)` around `(*ptr).lock()`. Only C calls it, at six places in `packages/bun-usockets/src`: - `fault_inject.c:44`, `:54`, `:63` and `:73` pass `&us_fault_lock`, a `static zig_mutex_t`. - `loop.c:376` and `:400` pass `&loop->data.mutex`, a field of the `us_loop_t` that `us_create_loop` gets from `us_xcalloc`. Both are non-null and zero-initialised, which is the unlocked state of the futex, `os_unfair_lock` and SRWLOCK implementations. `zig_mutex_t` is `uint32_t`, `os_unfair_lock` or `void *`, which have the alignment of the Rust types. Every caller goes on to use the data that the mutex guards, so the object is live for the whole call. All three implementations are interior-mutable, so a shared reference lets other threads lock at the same time. `Bun__unlock` keeps its raw pointer, for the reason `Mutex::unlock_raw` gives. `Bun__lock` compiles to the same instructions. It now equals `Mutex::lock` down to the parameter attributes, so in the Linux release build LLVM emits `Mutex::lock` as an alias of it. The Windows and macOS implementations pass `cargo check` for their targets. --- src/threading/Mutex.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/threading/Mutex.rs b/src/threading/Mutex.rs index 6e01eef2ba8e..83244f2c2e3d 100644 --- a/src/threading/Mutex.rs +++ b/src/threading/Mutex.rs @@ -426,9 +426,8 @@ impl FutexImpl { // These have to be a size known to C. #[unsafe(no_mangle)] -unsafe extern "C" fn Bun__lock(ptr: *mut ReleaseImpl) { - // SAFETY: C caller passes a valid, initialized ReleaseImpl pointer. - unsafe { (*ptr).lock() } +extern "C" fn Bun__lock(lock: &ReleaseImpl) { + lock.lock() } // These have to be a size known to C. From 4180b968c3e10d4e5b31af8e491e70e7162560d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:56 +0000 Subject: [PATCH 14/27] ptr: `DebugData::deinit` takes `&self` `DebugData` is the canary that `RefCount` and `ThreadSafeRefCount` carry in builds with `debug_assertions`. Its `deinit` took `&mut self` to clear `magic: u32`. `RefCount::deref`, `ThreadSafeRefCount::deref` and `ThreadSafeRefCount::release` have a `&` to the count in scope, so each of them projected the field from the raw pointer a second time, in `unsafe`, to get a `&mut` next to that `&`. `magic` is an `AtomicU32` that is read and written relaxed, `deinit` takes `&self`, and the three callers write `count.debug.deinit()`. Release builds contain none of this: the release assembly of `bun_ptr` is the same before and after. In debug builds the plain load and store of `magic` become relaxed atomic ones. --- src/ptr/ref_count.rs | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/src/ptr/ref_count.rs b/src/ptr/ref_count.rs index e76418055d56..af7fe2f59e5c 100644 --- a/src/ptr/ref_count.rs +++ b/src/ptr/ref_count.rs @@ -198,10 +198,7 @@ impl RefCount { count.raw_count.set(count.raw_count.get() - 1); if count.raw_count.get() == 0 { #[cfg(debug_assertions)] - { - // SAFETY: `count` is not used again; we need &mut for deinit - unsafe { (*T::get_ref_count(self_)).debug.deinit() }; - } + count.debug.deinit(); // SAFETY: raw_count == 0, sole owner unsafe { T::destructor(self_) }; } @@ -325,10 +322,7 @@ impl ThreadSafeRefCount { debug_assert!(old_count > 0); if old_count == 1 { #[cfg(debug_assertions)] - { - // SAFETY: we hold the last ref; exclusive access - unsafe { (*T::get_ref_count(self_)).debug.deinit() }; - } + count.debug.deinit(); // SAFETY: last ref dropped unsafe { T::destructor(self_) }; } @@ -359,10 +353,7 @@ impl ThreadSafeRefCount { debug_assert!(old_count > 0); if old_count == 1 { #[cfg(debug_assertions)] - { - // SAFETY: we hold the last ref; exclusive access - unsafe { (*T::get_ref_count(self_)).debug.deinit() }; - } + count.debug.deinit(); true } else { false @@ -660,25 +651,27 @@ const MAGIC_VALID: u32 = 0x2f84_e51d; /// pointer asserts instead of silently corrupting the count. #[cfg(debug_assertions)] struct DebugData { - magic: u32, + magic: AtomicU32, } #[cfg(debug_assertions)] impl DebugData { const fn empty() -> Self { - Self { magic: MAGIC_VALID } + Self { + magic: AtomicU32::new(MAGIC_VALID), + } } fn assert_valid(&self) { debug_assert!( - self.magic == MAGIC_VALID, + self.magic.load(Ordering::Relaxed) == MAGIC_VALID, "ref/deref on a destroyed refcount" ); } - fn deinit(&mut self) { + fn deinit(&self) { self.assert_valid(); - self.magic = 0; + self.magic.store(0, Ordering::Relaxed); } } From fcab46dd5e71c676fb4d87f9c02db554f1a577ca Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:57 +0000 Subject: [PATCH 15/27] sql: `PostgresSQLConnection::drop` zeroes `options_buf` with `secure_zero` `drop` cleared the buffer that holds the connection options, the password among them, with its own loop of one `write_volatile` per byte in `unsafe`. `bun_alloc::secure_zero` takes the `&mut [u8]` and is safe, and `bun_sql_jsc` already depends on `bun_alloc`. The compiled code changes: the byte loop becomes one `memset`, which a `black_box` on the pointer and a compiler fence keep rustc's LLVM backend from removing. `write_volatile` is guaranteed by the language and `black_box` is documented as best effort; this is the form `secure_zero`'s doc comment argues for, and the other Rust code that zeroes a secret it owns uses it (`Data::zdeinit`, `zero_sensitive`, `free_sensitive`, `free_sensitive_cstr`). --- src/sql_jsc/postgres/PostgresSQLConnection.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index 9e82a7760381..bd78917900c7 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -174,11 +174,8 @@ bun_event_loop::impl_timer_owner!(PostgresSQLConnection; impl Drop for PostgresSQLConnection { fn drop(&mut self) { self.disconnect(); - // `free_sensitive`: the connection options carry the password. - for b in self.options_buf.iter_mut() { - // SAFETY: plain byte write; volatile so it is not elided. - unsafe { core::ptr::write_volatile(b, 0) }; - } + // The connection options carry the password. + bun_alloc::secure_zero(&mut self.options_buf); } } From 12efe51d29133bcfacf934d36c39bff0940d2906 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:57 +0000 Subject: [PATCH 16/27] bundler: `zig__ModuleInfoDeserialized__deinit` takes the `Box` that it drops The exported function took `*mut ModuleInfoDeserialized`. It turned the argument into a `NonNull` with `unwrap_unchecked`, turned that back into a raw pointer, and passed it to `ModuleInfoDeserialized::deinit`, an `unsafe fn` with no other caller whose body was `drop(heap::take(this))`. The parameter is now `Box` and the body is `drop(info)`. That is what C++ passes: `ResolvedSource.module_info` is an `Option>`, and C++ gives back the pointer that it found in that field. A `Box` of a sized type is passed as that pointer, and `heap::take` is `Box::from_raw`, so the same box is dropped. As a `Box` the parameter is `noalias`, `nonnull` and aligned for LLVM. No `unsafe` is left in the function. `deinit`, the `NonNull` import and `use bun_core;` are gone. A `Box` is never null and is owned by the callee. The four C++ call sites hold to both. `Bun__analyzeTranspiledModule` (BunAnalyzeTranspiledModule.cpp) has returned early if `m_moduleInfo` is null, and `releaseModuleInfo` (ZigGlobalObject.cpp) gets a provider only if it is set; both set the field to null after the call. `~SourceProvider` (ZigSourceProvider.cpp) and `~ErrorableResolvedSource` (headers-handwritten.h) test the pointer in an `if`, and the object that held it is gone afterwards. `m_moduleInfo` gets its value only from `std::exchange(resolvedSource.module_info, nullptr)`, so the two objects never hold the same pointer. --- src/bundler/analyze_transpiled_module.rs | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/bundler/analyze_transpiled_module.rs b/src/bundler/analyze_transpiled_module.rs index 8f90eec3b099..fbb851c65fd3 100644 --- a/src/bundler/analyze_transpiled_module.rs +++ b/src/bundler/analyze_transpiled_module.rs @@ -1,7 +1,3 @@ -use core::ptr::NonNull; - -use bun_core; - // ────────────────────────────────────────────────────────────────────────── // Re-exports from the printer crate // @@ -267,15 +263,6 @@ impl ModuleInfoDeserialized { self.table.get(id) } - /// Consumes the heap allocation containing `self`. - /// - /// # Safety - /// `this` must have been produced by one of the constructors below. - pub(crate) unsafe fn deinit(this: *mut ModuleInfoDeserialized) { - // SAFETY: caller contract — see fn doc above. - drop(unsafe { bun_core::heap::take(this) }); - } - /// The self-contained form (a module's own string table followed by its /// body, as the runtime transpiler cache stores it). The bytes are copied /// once; the record views the copy. @@ -541,11 +528,8 @@ impl ModuleInfoExt for ModuleInfo { // zig__renderDiff, zig__ModuleInfoDeserialized__toJSModuleRecord, and the // JSModuleRecord/IdentifierArray opaques: see bun_bundler_jsc::analyze_jsc +/// C++ gives back the `Box` it took from `ResolvedSource.module_info`, never null. #[unsafe(no_mangle)] -extern "C" fn zig__ModuleInfoDeserialized__deinit(info: *mut ModuleInfoDeserialized) { - // SAFETY: C++ caller passes a non-null pointer obtained from `create` or - // `ModuleInfoExt::into_deserialized`. - let info = unsafe { NonNull::new(info).unwrap_unchecked() }; - // SAFETY: `info` is a valid, exclusively-owned pointer; `deinit` is its only destructor. - unsafe { ModuleInfoDeserialized::deinit(info.as_ptr()) } +extern "C" fn zig__ModuleInfoDeserialized__deinit(info: Box) { + drop(info); } From 63f26c1ee7d7273a509645fe9f377a62d9933c36 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:58 +0000 Subject: [PATCH 17/27] transpiler: `print_with_source_map` takes `Option<&mut ModuleInfo>` The two callers that collect a module record own it in a local `Option>`. They turned `as_deref_mut()` into an `Option<*mut ModuleInfo>`, which went through four functions of `Transpiler`, all safe, one of them public, until `print_ast_esm_ascii` turned it back into the `Option<&mut ModuleInfo>` that `js_printer::Options` holds. The four functions now take `Option<&mut ModuleInfo>` and the two callers pass `module_info.as_deref_mut()`. They touch the box again only after the call has returned. The third caller, in `AsyncModule`, passes `None` as before. The argument is one pointer with null for `None`, where it was a tag and a pointer. --- src/bundler/transpiler.rs | 13 ++++--------- src/jsc/RuntimeTranspilerStore.rs | 8 +------- src/runtime/jsc_hooks.rs | 9 +-------- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index 16480d7d5260..53c17cf06e34 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -2271,7 +2271,7 @@ impl<'a> Transpiler<'a> { format: js_printer::Format, source_map_context: Option>, runtime_transpiler_cache: Option>, - module_info: Option<*mut analyze_transpiled_module::ModuleInfo>, + module_info: Option<&mut analyze_transpiled_module::ModuleInfo>, ) -> crate::Result { // Routed through the T0 ftrace subset like the // other bundler spans (`Bundler.computeChunks` etc.) — @@ -2422,7 +2422,7 @@ impl<'a> Transpiler<'a> { source_map_context: Option>, exports_kind: bun_ast::ExportsKind, runtime_transpiler_cache: Option>, - module_info: Option<*mut analyze_transpiled_module::ModuleInfo>, + module_info: Option<&mut analyze_transpiled_module::ModuleInfo>, ) -> crate::Result { self.print_ast_esm_ascii::( print_arena, @@ -2450,13 +2450,8 @@ impl<'a> Transpiler<'a> { source_map_context: Option>, exports_kind: bun_ast::ExportsKind, runtime_transpiler_cache: Option, - module_info: Option<*mut analyze_transpiled_module::ModuleInfo>, + module_info: Option<&mut analyze_transpiled_module::ModuleInfo>, ) -> crate::Result { - // Both set on this (EsmAscii) arm only. - // SAFETY: `module_info` is `ModuleInfo::create`'s `heap::alloc` (or - // null); it is exclusively owned by this print call until T6 reclaims - // it after `print_with_source_map` returns. - let module_info = module_info.map(|p| unsafe { &mut *p }); let opts = js_printer::Options { bundling: false, require_ref: Some(ast.require_ref), @@ -2542,7 +2537,7 @@ impl<'a> Transpiler<'a> { writer: &mut js_printer::BufferPrinter, format: js_printer::Format, handler: js_printer::SourceMapHandler<'_>, - module_info: Option<*mut analyze_transpiled_module::ModuleInfo>, + module_info: Option<&mut analyze_transpiled_module::ModuleInfo>, ) -> crate::Result { // env_var feature_flag getters return `Option` // (Some(default) when unset). diff --git a/src/jsc/RuntimeTranspilerStore.rs b/src/jsc/RuntimeTranspilerStore.rs index ce4b0564fd2e..64d695aa7fae 100644 --- a/src/jsc/RuntimeTranspilerStore.rs +++ b/src/jsc/RuntimeTranspilerStore.rs @@ -1042,12 +1042,6 @@ impl TranspilerJob { if let Some(mi) = module_info.as_deref_mut() { mi.flags.has_tla = !parse_result.ast.top_level_await_keyword.is_empty(); } - // Note: derive `*mut` from a `&mut` borrow (not `&x as *const _ as - // *mut _`, which is Stacked-Borrows UB). The `&mut` borrow ends when the - // closure returns; the raw pointer stays valid until `module_info` is - // moved/touched again (after `print_with_source_map`). - let module_info_ptr: Option<*mut analyze_transpiled_module::ModuleInfo> = - module_info.as_deref_mut().map(std::ptr::from_mut); let print_result = { // SAFETY: see `vm` note above — `from_raw` stores `vm` as a raw @@ -1063,7 +1057,7 @@ impl TranspilerJob { source_code_printer, js_printer::Format::EsmAscii, mapper.get(), - module_info_ptr, + module_info.as_deref_mut(), ) }; if let Err(err) = print_result { diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index c9c89840b594..0a3c8db5ba41 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -3128,13 +3128,6 @@ fn transpile_source_code_of_any_provenance( if let Some(mi) = module_info.as_deref_mut() { mi.flags.has_tla = !parse_result.ast.top_level_await_keyword.is_empty(); } - // Derive the `*mut` from a `&mut` borrow (not `&x as *const _ - // as *mut _`, which is Stacked-Borrows UB). The borrow ends - // here; the raw pointer stays valid until `module_info` is - // moved/touched again (after `print_with_source_map`). - let module_info_ptr: Option< - *mut bun_bundler::analyze_transpiled_module::ModuleInfo, - > = module_info.as_deref_mut().map(core::ptr::from_mut); // ── js_printer::print ─────────────────────────────────────── // SAFETY: `extra.source_code_printer` is non-null per `TranspileExtra` @@ -3188,7 +3181,7 @@ fn transpile_source_code_of_any_provenance( &mut *extra.source_code_printer, bun_js_printer::Format::EsmAscii, mapper.get(), - module_info_ptr, + module_info.as_deref_mut(), ) }; // The printer never took ownership of `module_info`; From b580702de8a2f9984729d0eb55e0ea769b3bb0ae Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:58 +0000 Subject: [PATCH 18/27] bundler: five `unsafe` blocks that plain borrows replace `StaticRouteVisitor` held `&'a LinkerContext<'a>`. `LinkerContext` is invariant in its lifetime, so no borrow of the `&mut LinkerContext<'a>` that `generate_chunks_in_parallel` gets has that type, and the borrow was detached. The struct now has two lifetimes, `&'a LinkerContext<'ctx>`, and holds `&*c`. Up to the visitor's last use `c` is only used shared; `write_output_files_to_disk(c, ..)` is in the other arm of the `if`. `is_done` called `(*this).graph.drain_deferred_tasks(&mut *this)`: a `&mut Graph` and a `&mut BundleV2` that contains it, both live. `drain_deferred_tasks` is now a method of `BundleV2` that names `self.graph.*`. It stays in Graph.rs, next to the counters it moves. `scan_css_imports` was a safe function that took `*const [CssCol]` and dereferenced it. It takes `&[CssCol]`. `finish_from_bake_dev_server` has a slice and passes it. `scan_imports_and_exports` has column pointers and uses `col_ref!`, as it does for the arguments next to this one. `find_imported_files_in_css_order` cast a slice to `*const [LayerName]` and dereferenced it, as a cast "between shadow types". `crate::bun_css::LayerName` re-exports `::bun_css::css_parser::LayerName`, so there is one type and the `match` yields `&[LayerName]`. For the same reason the `.cast::>()` on each of the two pointers that the function hands to `Layers::borrow` cast to the type it already had. Both are gone, with the comments there, on `Layers::borrow` and in `prepareCssAstsForChunk.rs` that spoke of two types. `Layers::borrow`, a safe function whose result `inner()` dereferences, took a `NonNull` for those casts. It takes `&Vec`, and `BackRef::new` stores the same pointer. `run_resolver` kept the slot from `get_or_put` as `*mut u32` in both arms. With `self.graph.build_graphs[target]` written out, which is the body of `path_to_source_index_map`, the arm for an existing entry reads `*entry.value_ptr` next to `self.graph.ast`. The other arm calls `enqueue_parse_task(&mut self)` before it fills the slot and keeps the pointer. The operations and their order stay the same at all five sites. --- src/bundler/Chunk.rs | 8 +--- src/bundler/DeferredBatchTask.rs | 2 +- src/bundler/Graph.rs | 16 ++++--- src/bundler/LinkerContext.rs | 5 +-- src/bundler/bundle_v2.rs | 39 +++++----------- .../linker_context/StaticRouteVisitor.rs | 6 +-- .../findImportedFilesInCSSOrder.rs | 45 +++++-------------- .../generateChunksInParallel.rs | 7 +-- .../linker_context/prepareCssAstsForChunk.rs | 7 --- .../linker_context/scanImportsAndExports.rs | 2 +- 10 files changed, 40 insertions(+), 97 deletions(-) diff --git a/src/bundler/Chunk.rs b/src/bundler/Chunk.rs index a5bdd1b0db72..3e00f843f9fa 100644 --- a/src/bundler/Chunk.rs +++ b/src/bundler/Chunk.rs @@ -1417,16 +1417,12 @@ impl Layers { /// Cow::Borrowed constructor. /// - /// Takes `NonNull` (not `&Vec`) because the sole caller in - /// `findImportedFilesInCSSOrder.rs` type-puns the lifetime-erased shadow - /// `crate::bun_css::LayerName` to the real `::bun_css::LayerName` via a - /// raw-pointer cast — that nominal-type erasure cannot go through `&`. /// The pointee is arena-owned storage that outlives the chunk pipeline /// (see the lifetime note on `Chunk`); `BackRef` encapsulates that /// invariant so `inner()`/`to_owned()` deref sites are safe. #[inline] - pub(crate) fn borrow(p: core::ptr::NonNull>) -> Self { - Layers::Borrowed(bun_ptr::BackRef::from(p)) + pub(crate) fn borrow(p: &Vec) -> Self { + Layers::Borrowed(bun_ptr::BackRef::new(p)) } /// If borrowed, deep-clone into an owned diff --git a/src/bundler/DeferredBatchTask.rs b/src/bundler/DeferredBatchTask.rs index 3f50fa4ad8f2..1876d24b26d2 100644 --- a/src/bundler/DeferredBatchTask.rs +++ b/src/bundler/DeferredBatchTask.rs @@ -45,7 +45,7 @@ impl DeferredBatchTask { } } - /// Bundle thread. The caller has counted the hop as a pending item (`Graph::drain_deferred_tasks`). + /// Bundle thread. The caller has counted the hop as a pending item (`BundleV2::drain_deferred_tasks`). pub(crate) fn schedule(&mut self) { let task = ConcurrentTask::create(Task::init(std::ptr::from_mut::(self))); self.get_bundle_v2().enqueue_on_js_loop_for_plugins(task); diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index 4a944ff156f2..1a6356b77ed1 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -220,22 +220,24 @@ impl<'a> Graph<'a> { ) -> &mut PathToSourceIndexMap { &mut self.build_graphs[target] } +} +impl BundleV2<'_> { /// Schedule a task to be run on the JS thread which resolves the promise of /// each `.defer()` called in an onLoad plugin. /// /// Returns true if there were more tasks queued. - pub(crate) fn drain_deferred_tasks(&mut self, transpiler: &mut BundleV2) -> bool { - transpiler.thread_lock.assert_locked(); + pub(crate) fn drain_deferred_tasks(&mut self) -> bool { + self.thread_lock.assert_locked(); - if self.deferred_pending > 0 { + if self.graph.deferred_pending > 0 { // Their units are back in `pending_items` (a new epoch: those loads' own units are no longer // parked), plus one for the hop itself: the pass is not done until it is back (see // `DeferredBatchTask`). - self.pending_items += self.deferred_pending + 1; - self.deferred_pending = 0; - self.defer_epoch += 1; - transpiler.drain_defer_task.schedule(); + self.graph.pending_items += self.graph.deferred_pending + 1; + self.graph.deferred_pending = 0; + self.graph.defer_epoch += 1; + self.drain_defer_task.schedule(); return true; } diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index 60649fdd6f50..17ad22ac9945 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -3277,14 +3277,11 @@ impl<'a> LinkerContext<'a> { pub(crate) fn scan_css_imports( file_source_index: u32, file_import_records: &[ImportRecord], - css_asts: *const [crate::bundled_ast::CssCol], + css_asts: &[crate::bundled_ast::CssCol], sources: &[Source], loaders: &[Loader], log: &mut Log, ) -> ScanCssImportsResult { - // SAFETY: `css_asts` points at the `graph.ast.items_css()` column for - // the duration of `scan_imports_and_exports`; we only test `is_none()`. - let css_asts = unsafe { &*css_asts }; for record in file_import_records.iter() { if record.source_index.is_valid() { // Other file is not CSS diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 706e739ccb98..377ecb7614c4 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -2453,13 +2453,7 @@ pub mod bv2_impl { } if self.graph.pending_items == 0 { - let this: *mut Self = self; - // reshaped for borrowck — `&self.graph` and - // `self` go to the same call. Take a raw ptr so the two `&mut` don't - // overlap from rustc's view. - // SAFETY: `drain_deferred_tasks` only touches `self.graph.deferred_*` - // fields and the `BundleV2` callback surface; no aliasing UB. - if unsafe { (*this).graph.drain_deferred_tasks(&mut *this) } { + if self.drain_deferred_tasks() { return false; } return true; @@ -2672,21 +2666,12 @@ pub mod bv2_impl { ) { let file_map_result = _file_map_result; let mut path_primary = file_map_result.path_pair.primary; - // reshaped for borrowck — `get_or_put` borrows `*self` mutably via - // `self.graph`; capture the slot as `*mut u32` so subsequent `self.*` calls - // type-check. SAFETY: `path_to_source_index_map(target)` is not mutated again - // until after the last `*value_ptr` access below. - let (found_existing, value_ptr): (bool, *mut u32) = { - let entry = self - .path_to_source_index_map(target) - .get_or_put(path_primary.text) - .expect("oom"); - ( - entry.found_existing, - std::ptr::from_mut::(entry.value_ptr), - ) - }; - if !found_existing { + let entry = self.graph.build_graphs[target] + .get_or_put(path_primary.text) + .expect("oom"); + if !entry.found_existing { + // `enqueue_parse_task` below borrows all of `self`. + let value_ptr: *mut u32 = entry.value_ptr; let loader: Loader = 'brk: { let record: &mut ImportRecord = &mut self.graph.ast.items_import_records_mut() @@ -2717,7 +2702,7 @@ pub mod bv2_impl { import_record.original_target, ) .expect("oom"); - // SAFETY: see `value_ptr` note above. + // SAFETY: nothing has touched `build_graphs[target]` since `get_or_put`. unsafe { *value_ptr = idx }; let record: &mut ImportRecord = &mut self.graph.ast.items_import_records_mut() @@ -2731,8 +2716,7 @@ pub mod bv2_impl { [import_record.importer_source_index as usize] .as_mut_slice() [import_record.import_record_index as usize]; - // SAFETY: see `value_ptr` note above. - record.source_index = Index::init(unsafe { *value_ptr }); + record.source_index = Index::init(*entry.value_ptr); } return; } @@ -5870,10 +5854,7 @@ pub mod bv2_impl { if LinkerContext::scan_css_imports( u32::try_from(index).expect("int cast"), import_records.as_slice(), - // `scan_css_imports` takes the column as a raw - // `*const` slice (the scanImportsAndExports caller holds raw - // SoA pointers); it only reads via `is_none()`. - std::ptr::from_ref(css_asts), + css_asts, sources, loaders, &mut log, diff --git a/src/bundler/linker_context/StaticRouteVisitor.rs b/src/bundler/linker_context/StaticRouteVisitor.rs index 68b136f1b6fd..547df6ebde1f 100644 --- a/src/bundler/linker_context/StaticRouteVisitor.rs +++ b/src/bundler/linker_context/StaticRouteVisitor.rs @@ -10,8 +10,8 @@ use bun_core::env_var; use crate::import_record; use crate::{Index, LinkerContext, UseDirective}; -pub(crate) struct StaticRouteVisitor<'a> { - pub(crate) c: &'a LinkerContext<'a>, +pub(crate) struct StaticRouteVisitor<'a, 'ctx> { + pub(crate) c: &'a LinkerContext<'ctx>, pub(crate) cache: ArrayHashMap, pub(crate) visited: AutoBitSet, pub(crate) stack: Vec, @@ -23,7 +23,7 @@ pub(crate) struct Frame { next_record: usize, } -impl<'a> StaticRouteVisitor<'a> { +impl<'a> StaticRouteVisitor<'a, '_> { /// This the quickest, simplest, dumbest way I can think of doing this. /// Investigate performance. It can have false negatives (it doesn't properly /// handle cycles), but that's okay as it's just used an optimization diff --git a/src/bundler/linker_context/findImportedFilesInCSSOrder.rs b/src/bundler/linker_context/findImportedFilesInCSSOrder.rs index 9947b0777f0e..697188ef6676 100644 --- a/src/bundler/linker_context/findImportedFilesInCSSOrder.rs +++ b/src/bundler/linker_context/findImportedFilesInCSSOrder.rs @@ -214,11 +214,8 @@ pub(crate) fn find_imported_files_in_css_order<'a>( // Any pre-import layers come first. if !repr.layers_pre_import.is_empty() { - // See the `LayerName` note where the duplicate pass borrows `layer_names`. - let layers = - core::ptr::NonNull::from(&repr.layers_pre_import).cast::>(); self.push_entry( - CssImportOrderKind::Layers(Layers::borrow(layers)), + CssImportOrderKind::Layers(Layers::borrow(&repr.layers_pre_import)), wrapping_conditions, ); } @@ -473,17 +470,10 @@ pub(crate) fn find_imported_files_in_css_order<'a>( // This import is redundant, but it might have @layer rules. // So we should keep the @layer rules so that the cascade ordering of layers // is preserved - // - // `crate::bun_css::LayerName` (lifetime-erased - // shadow) and `::bun_css::LayerName` are distinct nominal - // types; cast through `NonNull` to satisfy - // `Layers::borrow`. - let layer_names_ptr = core::ptr::NonNull::from( - &css_asts[idx.get() as usize].as_deref().unwrap().layer_names, - ) - .cast::>(); + let layer_names = + &css_asts[idx.get() as usize].as_deref().unwrap().layer_names; order.mut_(i as usize).kind = - CssImportOrderKind::Layers(Layers::borrow(layer_names_ptr)); + CssImportOrderKind::Layers(Layers::borrow(layer_names)); continue 'next_backward; } } @@ -524,10 +514,7 @@ pub(crate) fn find_imported_files_in_css_order<'a>( { struct DuplicateEntry { // Lifetime-erased slice header — borrows either - // `css_asts[..].layer_names` (real `::bun_css::LayerName`) or - // `Layers::inner()` (shadow `LayerName`). Both nominal types should - // be reconciled; until then we compare via - // `LayerName::eql` on the shadow type and cast at the boundary. + // `css_asts[..].layer_names` or `Layers::inner()`. // `RawSlice` (vs raw `*const [_]`) so reads go through safe // `.slice()` under the back-reference invariant: the borrowed // storage (`css_asts` arena / `Layers` Vec) outlives this loop. @@ -615,25 +602,15 @@ pub(crate) fn find_imported_files_in_css_order<'a>( // that this tests all import order entries (not just layer ones) because // sometimes non-layer ones can make following layer ones redundant. // layers_post_import - let layers_key: *const [LayerName] = match &entry.kind { - CssImportOrderKind::SourceIndex(idx) => { - // See the LayerName nominal-type note above. - std::ptr::from_ref::<[_]>( - css_asts[idx.get() as usize] - .as_deref() - .unwrap() - .layer_names - .slice_const(), - ) as *const [LayerName] - } + let layers_key: &[LayerName] = match &entry.kind { + CssImportOrderKind::SourceIndex(idx) => css_asts[idx.get() as usize] + .as_deref() + .unwrap() + .layer_names + .slice_const(), CssImportOrderKind::Layers(layers) => layers.inner().slice_const(), CssImportOrderKind::ExternalPath(_) => &[][..], }; - // SAFETY: every match arm yields a pointer to a live slice (`css_asts` - // arena, `entry.kind`'s `Layers`, or a static empty); the source-index - // arm is a `*const [_]`-level cast between layout-identical `LayerName` - // shadows (see the note above). Valid for this loop iteration. - let layers_key: &[LayerName] = unsafe { &*layers_key }; let mut index: usize = 0; while index < layer_duplicates.len() as usize { let dup_layers: &[LayerName] = layer_duplicates.at(index).layers.slice(); diff --git a/src/bundler/linker_context/generateChunksInParallel.rs b/src/bundler/linker_context/generateChunksInParallel.rs index 87be060506b5..af530685b339 100644 --- a/src/bundler/linker_context/generateChunksInParallel.rs +++ b/src/bundler/linker_context/generateChunksInParallel.rs @@ -705,10 +705,7 @@ pub(crate) fn generate_chunks_in_parallel<'a, const IS_DEV_SERVER: bool>( let bundler: &mut BundleV2 = unsafe { &mut *LinkerContext::bundle_v2_ptr(std::ptr::from_mut::(c)) }; let mut static_route_visitor = StaticRouteVisitor { - // SAFETY: launder via raw ptr so this long-lived - // shared borrow doesn't conflict with `c.log_disjoint()` inside - // the chunk loop below. `c` outlives `static_route_visitor`. - c: unsafe { bun_ptr::detach_lifetime_ref::(c) }, + c: &*c, cache: bun_collections::ArrayHashMap::default(), visited: AutoBitSet::init_empty(c.graph.files.len()).expect("oom"), stack: Vec::new(), @@ -1180,7 +1177,7 @@ pub(crate) fn generate_chunks_in_parallel<'a, const IS_DEV_SERVER: bool>( // an error // logger OOM-only // Split-borrow — `static_route_visitor.c` holds a - // detached `&LinkerContext`; `log_disjoint` returns the + // `&LinkerContext`; `log_disjoint` returns the // disjoint `Transpiler.log` backref so no `&mut c` is // materialized. let _ = c.log_disjoint().add_error_fmt( diff --git a/src/bundler/linker_context/prepareCssAstsForChunk.rs b/src/bundler/linker_context/prepareCssAstsForChunk.rs index d62231d40c71..59b53c7d4321 100644 --- a/src/bundler/linker_context/prepareCssAstsForChunk.rs +++ b/src/bundler/linker_context/prepareCssAstsForChunk.rs @@ -108,13 +108,6 @@ fn prepare_css_asts_for_chunk_impl(c: &LinkerContext, chunk: &mut Chunk, bump: & let inner = layers.inner(); let len = inner.len(); let rules = if len > 0 { - // The `Chunk::Layers` payload is the lifetime-erased shadow - // `bun_css::LayerName { v: Vec> }`, - // not the real `css_parser::LayerName { v: SmallList<&'static [u8],1> }`, - // so the layouts differ. Rebuild the real list element-by-element; - // segments are arena-owned (`'bump`-laundered to `'static`) so the - // `&[u8]` reborrows below are valid for the chunk lifetime. - // // Both `SmallList` levels go into the arena-backed rule list // that `CssChunk::Drop` `set_len(0)`s without running element // destructors, so any global heap spill would leak. Build them diff --git a/src/bundler/linker_context/scanImportsAndExports.rs b/src/bundler/linker_context/scanImportsAndExports.rs index 73dee5b22d28..e1790d6cce2f 100644 --- a/src/bundler/linker_context/scanImportsAndExports.rs +++ b/src/bundler/linker_context/scanImportsAndExports.rs @@ -149,7 +149,7 @@ pub(crate) fn scan_imports_and_exports( let _ = LinkerContext::scan_css_imports( id as u32, col_ref!(import_records_list)[id].as_slice(), - css_asts, + col_ref!(css_asts), col_ref!(input_files), col_ref!(loaders), // `log_disjoint`: split-borrow with the SoA column refs above. From ae5fd67deb6a6c23a3e2d750b7933ec92d8ebac0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:59 +0000 Subject: [PATCH 19/27] bundler: `BundleThread::enqueue` takes `&self`; `generation` is a local of the thread `generation` was a field that only `thread_main` read and wrote, through the raw `instance` pointer. It was also the only field written after the thread has set `ready_event`, and so the reason why no thread could hold a `&BundleThread`. It is now a local of `thread_main`. After `waker` is written, every thread only reads the fields or uses the atomics in `ready_event` and `queue`, so `thread_main` takes one `&*instance` there, in place of a dereference each for `ready_event.set()`, `queue.pop()`, `waker.wait()` and, on Windows, `waker.uv_loop()`. `enqueue` was an `unsafe fn(*mut Self, *mut C)` that rebuilt a `NonNull` with `new_unchecked`. Its one caller has a `NonNull` and passed `.as_ptr()`. It is now `unsafe fn enqueue(&self, NonNull)`. The types say that the instance is live and that the task is not null, and the body has no `unsafe` block: `UnboundedQueue::push` and `Waker::wake` are safe `&self` functions on every platform. It stays `unsafe` for the one obligation that no type carries: `completion` must be a live task node that stays valid until the bundle thread has run or freed it, because `thread_main` dereferences what it pops. The caller dereferences the singleton pointer and calls `enqueue` in one block, where it called the old function. The counter moves from the heap allocation to the thread's stack or a register. The calls and their order stay the same. --- src/bundler/BundleThread.rs | 72 ++++++++++++------------------------- 1 file changed, 23 insertions(+), 49 deletions(-) diff --git a/src/bundler/BundleThread.rs b/src/bundler/BundleThread.rs index bdd0c35155a0..e82add8f5e32 100644 --- a/src/bundler/BundleThread.rs +++ b/src/bundler/BundleThread.rs @@ -51,7 +51,6 @@ pub(crate) struct BundleThread { // `bun.UnboundedQueue(CompletionStruct, .next)` — intrusive over `C.next`; // the field offset is encoded via the `Node` supertrait on `CompletionStruct`. pub(crate) queue: UnboundedQueue, - pub(crate) generation: bun_core::Generation, } /// Trait capturing the interface a completion task must satisfy. @@ -131,7 +130,6 @@ impl BundleThread { waker: Async::Waker::placeholder(), waker_error: None, queue: UnboundedQueue::new(), - generation: 0, ready_event: ResetEvent::default(), } } @@ -139,15 +137,15 @@ impl BundleThread { /// # Safety /// `instance` must be valid for `'static` (the spawned thread runs forever and /// accesses it). On `Ok`, the bundle thread concurrently accesses - /// `*instance`; callers must only touch it via the raw-pointer methods on this + /// `*instance`; callers must only touch it via the `&self` methods on this /// impl (e.g. `enqueue`) and never materialize a `&mut Self`. On `Err`, no /// thread references `*instance` and the caller owns it again. pub(crate) unsafe fn spawn(instance: *mut Self) -> std::io::Result<()> { // `std::thread::Builder` (not `std::thread::spawn`) so the spawn error // is surfaced to the caller. struct SendPtr(*mut T); - // SAFETY: the pointer is only dereferenced on the bundle thread via raw - // projections; `BundleThread` itself is never moved across threads. + // SAFETY: the pointer is only dereferenced on the bundle thread; + // `BundleThread` itself is never moved across threads. unsafe impl Send for SendPtr {} let ptr = SendPtr(instance); let thread = std::thread::Builder::new() @@ -155,8 +153,7 @@ impl BundleThread { .spawn(move || { let ptr = ptr; // SAFETY: caller guarantees `instance` is valid for 'static; `thread_main` - // accesses fields only via raw-ptr projection (never `&Self`/`&mut Self`) - // and is the sole writer of `waker`/`generation`, so concurrent `enqueue()` + // writes `waker`/`waker_error` only before `ready_event.set()`, so `enqueue()` // from other threads is sound. unsafe { Self::thread_main(ptr.0) } })?; @@ -179,22 +176,10 @@ impl BundleThread { } /// # Safety - /// `instance` must point to a live `BundleThread` whose bundle thread has been - /// spawned (so `waker` is initialized). Called concurrently with `thread_main`. - pub(crate) unsafe fn enqueue(instance: *mut Self, completion: *mut C) { - // SAFETY: `completion` is a live, caller-owned task node (non-null). - let completion = unsafe { core::ptr::NonNull::new_unchecked(completion) }; - // SAFETY: field projections via raw ptr — `thread_main` on the bundle thread - // accesses the same struct concurrently, so we never materialize `&mut Self`. - // `UnboundedQueue::push` takes `&self` (lock-free MPSC). `Waker::wake` takes - // `&self` on all platforms and only reads a Copy field (eventfd, mach port, - // `WindowsLoop` pointer) to pass to a wake call that is safe from any thread - // (eventfd write, mach_msg send, uv_async_send), so the `&Waker` autoref is - // sound alongside `wait(&self)` in `thread_main` and other `enqueue` callers. - unsafe { - (*instance).queue.push(completion); - (*instance).waker.wake(); - } + /// `completion` must be a live, caller-owned task node, valid until the bundle thread has run or freed it. + pub(crate) unsafe fn enqueue(&self, completion: NonNull) { + self.queue.push(completion); + self.waker.wake(); } unsafe fn thread_main(instance: *mut Self) { @@ -217,10 +202,12 @@ impl BundleThread { // SAFETY: `waker` is written exactly once here, before `ready_event.set()` // releases any thread that could call `enqueue` (which reads `waker`). unsafe { core::ptr::addr_of_mut!((*instance).waker).write(waker) }; + // SAFETY: valid for 'static (`spawn`'s contract). That was the last plain write: from + // here on every thread reads the fields or uses the atomics in `ready_event` / `queue`. + let this = unsafe { &*instance }; // Unblock the calling thread so it can continue. - // SAFETY: raw-ptr field projection; spawning thread is blocked in `ready_event.wait()`. - unsafe { (*instance).ready_event.set() }; + this.ready_event.set(); // The libuv Timer lives on stack for the lifetime of this never-returning fn. // It MUST be declared at function scope (not inside the `#[cfg(windows)] { ... }` @@ -231,20 +218,15 @@ impl BundleThread { let mut timer: bun_sys::windows::libuv::Timer = bun_core::ffi::zeroed(); #[cfg(windows)] { - // SAFETY: raw place read of `waker.loop_.uv_loop` (Copy ptr); field is - // write-once in `Waker::init()` above and never mutated by `wake()`, so a - // concurrent `enqueue()` (possible now that `ready_event.set()` has fired) - // does not conflict. No `&Waker`/`&mut Waker` is materialized here. - timer.init(unsafe { (*instance).waker.uv_loop() }); + timer.init(this.waker.uv_loop()); timer.start(u64::MAX, u64::MAX, Some(timer_callback)); } let mut has_bundled = false; + let mut generation: bun_core::Generation = 0; loop { loop { - // SAFETY: `UnboundedQueue::pop` takes `&self`; concurrent `push` from - // `enqueue` is the lock-free queue's intended use. - let completion = unsafe { (*instance).queue.pop() }; + let completion = this.queue.pop(); if completion.is_null() { break; } @@ -257,8 +239,6 @@ impl BundleThread { } // SAFETY: as above; started ⇒ the owner waits for us. let completion = unsafe { &mut *completion }; - // SAFETY: `generation` is only read/written on this (bundle) thread. - let generation = unsafe { (*instance).generation }; // `panic = "abort"` → a Rust panic on this thread enters the // crash-handler hook and aborts the whole process. // No `catch_unwind` — there is nothing to catch. @@ -271,11 +251,7 @@ impl BundleThread { } has_bundled = true; } - // SAFETY: `generation` is only read/written on this (bundle) thread. - unsafe { - let g = core::ptr::addr_of_mut!((*instance).generation); - *g = (*g).saturating_add(1); - } + generation = generation.saturating_add(1); if has_bundled { crate::bundle_v2::dispatch::__bun_jsc_destroy_bytecode_cache_vm(); @@ -283,8 +259,7 @@ impl BundleThread { has_bundled = false; } - // SAFETY: `Waker::wait` takes `&self`; concurrent `wake()` from `enqueue` is by design. - unsafe { (*instance).waker.wait() }; + this.waker.wait(); } } @@ -379,9 +354,7 @@ pub mod singleton { /// `Send` newtype around the leaked `BundleThread` allocation so it can be /// stored in a `Guarded`. Type-erased because Rust forbids generic - /// statics; see module comment. Stored as a raw pointer (not `&'static`) - /// because the bundle thread mutates `*self` concurrently — callers must - /// only ever project fields via raw-pointer access. + /// statics; see module comment. struct Instance(NonNull<()>); // SAFETY: the allocation is a leaked `Box>` valid for // `'static`; cross-thread access is mediated entirely through @@ -394,7 +367,7 @@ pub mod singleton { /// use. If the thread fails to start, the next build tries again. /// The bundle thread runs `thread_main` against this allocation for the /// process lifetime, so callers MUST NOT materialize `&mut BundleThread` from it. - /// Use `BundleThread::enqueue(get(), ...)` instead. + /// Use `&BundleThread` (`enqueue`) instead. /// /// # Safety /// All calls (across the process) must use the same `C`; the static is @@ -428,9 +401,10 @@ pub mod singleton { }); let err = match get::() { Ok(instance) => { - // SAFETY: `get()` returns the leaked 'static singleton whose bundle thread is - // running; `BundleThread::enqueue` only performs raw-ptr field projections. - unsafe { BundleThread::enqueue(instance, completion.as_ptr()) }; + // SAFETY: `get()` returned the leaked 'static singleton; since `spawn` returned, no thread writes it + // except through its atomics. `completion` is live and brings its creation reference, which is + // dropped only after the bundle thread has run the task or by `free_released_unstarted`. + unsafe { (*instance).enqueue(completion) }; return; } Err(err) => err, From 8fe881872b9666e56fa64ffd1efa9f1b8528aadc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:59 +0000 Subject: [PATCH 20/27] bundler: `enqueue_entry_points_dev_server` borrows the transpiler where it uses it The loop picked the client or the server transpiler at its top and kept it as `*mut Transpiler`, because a borrow would have held `self` across the plugin dispatch that follows. The pointer was dereferenced three times further down: for `resolve_entry_point`, and twice in its error arm for `log`. The transpiler is now picked after the plugin dispatch, right before `resolve_entry_point`, as a `&mut`. What that call returns borrows only `abs_path` and `dot_slash`, so `self` is free again in both arms. The error arm first copies `transpiler.log` and uses the copy; the dereference for `reset()` stays. The pick now runs only for entry points that no plugin matched. It has no side effect here: the one caller is the dev server, which sets `linker.dev_server` and `client_transpiler` before, so `transpiler_for_target` only selects a pointer. --- src/bundler/bundle_v2.rs | 62 ++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 377ecb7614c4..8f9a4d828a39 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -3545,16 +3545,6 @@ pub mod bv2_impl { debug_assert_eq!(files.set.keys().len(), files.set.values().len()); for (abs_path, flags) in files.set.keys().iter().zip(files.set.values().iter()) { - // Ensure we have the proper conditions set for client-side entrypoints. - // SAFETY: hold the transpiler as a `*mut` across the loop body - // so it doesn't keep `self` borrowed through the plugin - // dispatch / dev_server calls below; the pointee lives for `'a`. - let transpiler: *mut Transpiler<'a> = - if flags.client() && !flags.server() && !flags.ssr() { - std::ptr::from_mut(self.transpiler_for_target(Target::Browser)) - } else { - &raw mut *self.transpiler - }; let server_target = self.transpiler.options.target; let client_loader = flags.html().then_some(Loader::Html); @@ -3600,30 +3590,34 @@ pub mod bv2_impl { // Fall back to normal resolution if no plugins matched let dot_slash = OnceCell::new(); - // SAFETY: `transpiler` points at one of self's transpilers, live for `'a`. - let resolved = - match unsafe { &mut *transpiler }.resolve_entry_point(abs_path, &dot_slash) { - Ok(r) => r, - Err(err) => { - let dev = self.dev_server.expect("unreachable"); - dev.handle_parse_task_failure( - err, - if flags.client() { - bake::Graph::Client - } else { - bake::Graph::Server - }, - abs_path, - // SAFETY: `transpiler` points at one of self's transpilers, live for `'a`. - unsafe { (*transpiler).log }.cast_const(), - std::ptr::from_mut(self), - ) - .expect("oom"); - // SAFETY: `transpiler.log` is the `'a`-owned log; sole writer here. - unsafe { (*(*transpiler).log).reset() }; - continue; - } - }; + // Ensure we have the proper conditions set for client-side entrypoints. + let transpiler = if flags.client() && !flags.server() && !flags.ssr() { + self.transpiler_for_target(Target::Browser) + } else { + &mut *self.transpiler + }; + let resolved = match transpiler.resolve_entry_point(abs_path, &dot_slash) { + Ok(r) => r, + Err(err) => { + let log = transpiler.log; + let dev = self.dev_server.expect("unreachable"); + dev.handle_parse_task_failure( + err, + if flags.client() { + bake::Graph::Client + } else { + bake::Graph::Server + }, + abs_path, + log.cast_const(), + std::ptr::from_mut(self), + ) + .expect("oom"); + // SAFETY: `transpiler.log` is the `'a`-owned log; sole writer here. + unsafe { (*log).reset() }; + continue; + } + }; if flags.client() { 'brk: { From b1147c9efc5de4fc2a5ba86a0506b1c6d2aa7752 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:50:59 +0000 Subject: [PATCH 21/27] bundler: `process_html_import_files` borrows four fields of the parse graph The function went through the raw `parse_graph` pointer five times, four of them in every iteration. One of the five, `(*parse_graph).path_to_source_index_map(..)`, made a `&mut Graph` over the whole graph only to call `get(&self)` on one map, while the source map workers that `link` has just scheduled read the same graph. A sixth block made a `&mut Symbol` with `symbol_mut` to set `link`, which is a `Cell`. One block now borrows the four fields that are used: `html_imports.html_source_indices` mutably, and `html_imports.server_source_indices`, `input_files` and `build_graphs` shared. The workers hold `&Graph` and `&BundleV2` at that time, and of the graph they read only `input_files` and `pool`. The symbol comes from `LinkerGraph::symbol`, which is `symbols.get_const(..).expect(..)`. `get_const` has the same two guards as `get` and no bounds check either. `symbol` was compiled only with `debug_assertions`, because its one user was, and loses that `cfg`. The loop iterates over `server_source_indices` instead of indexing it, which drops one bounds check per iteration, and the `source` column and the browser map are looked up once instead of per iteration. --- src/bundler/LinkerContext.rs | 60 ++++++++++++------------------------ src/bundler/LinkerGraph.rs | 1 - 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index 17ad22ac9945..bfac37122ee5 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -725,46 +725,30 @@ impl<'a> LinkerContext<'a> { } fn process_html_import_files(&mut self) { - // SAFETY: `parse_graph` is a backref to `BundleV2.graph`, a sibling - // field of `BundleV2.linker` (= `*self`). The two are disjoint, and no - // other `&`/`&mut` to `BundleV2.graph` is live for this scope — - // `self.graph` below is `LinkerGraph`, a distinct allocation. - // Note: go through raw pointers and reborrow per use to avoid holding - // overlapping `&`/`&mut` into `parse_graph.html_imports` and - // `parse_graph.input_files`. - let parse_graph: *mut Graph<'a> = self.parse_graph; - // SAFETY: see above; sole accessor of `html_imports` for this scope. - let server_len = unsafe { (*parse_graph).html_imports.server_source_indices.len() }; - if server_len > 0 { + // SAFETY: `parse_graph` points to `BundleV2.graph`, a sibling of `BundleV2.linker` (= `*self`), so these four + // fields are disjoint from `*self` and from each other. The source map workers that may be running hold + // `&Graph` and `&BundleV2`; of the graph they read only `input_files` and `pool`. + let (html_source_indices, server_source_indices, input_files, build_graphs) = unsafe { + let parse_graph: *mut Graph<'a> = self.parse_graph; + ( + &mut (*parse_graph).html_imports.html_source_indices, + &(*parse_graph).html_imports.server_source_indices, + &(*parse_graph).input_files, + &(*parse_graph).build_graphs, + ) + }; + if !server_source_indices.is_empty() { let actual_ref = self.graph.runtime_function(b"__jsonParse"); + let sources = input_files.items_source(); + let browser_paths = &build_graphs[Target::Browser]; - for i in 0..server_len as usize { - // SAFETY: `server_source_indices` is a stable Vec; index - // bounded by `server_len`. - let html_import: u32 = - unsafe { (*parse_graph).html_imports.server_source_indices.slice()[i] }; - // SAFETY: `input_files` SoA is append-only; read-only here. - let path_text = unsafe { - &(*parse_graph).input_files.items_source()[html_import as usize] - .path - .text - }; - // SAFETY: sole `&mut` into the per-target map for this lookup. - let source_index: u32 = unsafe { - (*parse_graph).path_to_source_index_map(Target::Browser) - } - .get(path_text) - .unwrap_or_else(|| { + for &html_import in server_source_indices.iter() { + let path_text = sources[html_import as usize].path.text; + let source_index: u32 = browser_paths.get(path_text).unwrap_or_else(|| { panic!("Assertion failed: HTML import file not found in pathToSourceIndexMap"); }); - // SAFETY: sole `&mut` into `html_source_indices` for this push. - unsafe { - (*parse_graph) - .html_imports - .html_source_indices - .push(source_index) - }; + html_source_indices.push(source_index); // S.LazyExport is a call to __jsonParse. Each accessor returns // `Option`; `.unwrap()` panics on shape mismatch. @@ -782,11 +766,7 @@ impl<'a> LinkerContext<'a> { .ref_; // Make the __jsonParse in that file point to the __jsonParse in the runtime chunk. - // SAFETY: `original_ref`'s symbol slot is disjoint from any live borrow here - // (only `actual_ref` is held, a `Copy` value). - unsafe { self.graph.symbol_mut(original_ref) } - .link - .set(actual_ref); + self.graph.symbol(original_ref).link.set(actual_ref); // When --splitting is enabled, we have to make sure we import the __jsonParse function. self.graph diff --git a/src/bundler/LinkerGraph.rs b/src/bundler/LinkerGraph.rs index 2204c2f60b6a..c425ba8ff16b 100644 --- a/src/bundler/LinkerGraph.rs +++ b/src/bundler/LinkerGraph.rs @@ -483,7 +483,6 @@ impl<'a> LinkerGraph<'a> { /// produced by the symbol table itself). Thin wrapper over /// [`symbol::Map::get_const`]; callers previously open-coded /// `unsafe { &*graph.symbols.get(r).expect(..) }`. - #[cfg(debug_assertions)] #[inline] pub(crate) fn symbol(&self, ref_: Ref) -> &Symbol { self.symbols From e85108df2a4a1e760749b829ddcde0bf6bac359d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:00 +0000 Subject: [PATCH 22/27] transpiler: `Linker` takes what it reads as parameters instead of pointing into its `Transpiler` `Transpiler` holds its `Linker` by value. The `Linker` held three raw pointers back into the same `Transpiler`: `options`, `resolve_queue` and `resolve_results`. They were null after `Transpiler::init`, were set by `configure_linker*`, and had to be set again by `wire_after_move` after each move. Three accessors dereferenced them. `resolve_queue` and `resolve_results` served only `enqueue_resolve_result`, which has one caller, `Transpiler::enqueue_entry_points`. It is now an associated function that gets `&mut self.resolve_results` and `&mut self.resolve_queue`. `options` served only two scalar reads at the top of `link`, which now takes `options: &BundleOptions<'_>`. Its two callers in `Transpiler` pass `&self.options`. The two callers outside the crate, in `AsyncModule` and `jsc_hooks`, pass `&(*jsc_vm).transpiler.options` in the `unsafe` block they already have; that is the field the pointer pointed to, because the VM calls `configure_linker*` on the transpiler in place. `Linker` is left with `log`, and `Linker::init` takes only that. The three accessors and `reseat_self_refs` are gone. `link` gets the address in an argument register instead of loading it from `self`, `Transpiler` is 24 bytes smaller, and the debug assertions that the pointers are not null are gone with the pointers. --- src/bundler/ThreadPool.rs | 4 +- src/bundler/bundle_v2.rs | 2 +- src/bundler/linker.rs | 120 +++------------------------------- src/bundler/transpiler.rs | 55 +++++----------- src/install/PackageManager.rs | 2 +- src/jsc/AsyncModule.rs | 3 +- src/runtime/jsc_hooks.rs | 9 +-- 7 files changed, 35 insertions(+), 160 deletions(-) diff --git a/src/bundler/ThreadPool.rs b/src/bundler/ThreadPool.rs index b1240fb3299d..6bf48bcceccd 100644 --- a/src/bundler/ThreadPool.rs +++ b/src/bundler/ThreadPool.rs @@ -4,7 +4,7 @@ //! //! `Worker::create` / `initialize_transpiler` build the per-worker //! `Transpiler` via `Transpiler::for_worker` (per-field deep clone — no -//! bitwise struct copy); the self-referential `linker` backrefs are wired by +//! bitwise struct copy); the self-referential `macro_context` is wired by //! `Transpiler::wire_after_move` once the value is at its final address. use core::mem::ManuallyDrop; @@ -650,7 +650,7 @@ impl Worker { transpiler: Self::initialize_transpiler(log, ctx.transpiler(), arena_ref), other_transpiler: None, }); - // Wire self-referential `linker`/`macro_context` now that `transpiler` + // Wire the self-referential `macro_context` now that `transpiler` // is at its final address inside `WorkerData`. data.transpiler.wire_after_move(); diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 8f9a4d828a39..f78d46273a97 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1970,7 +1970,7 @@ pub mod bv2_impl { // the overwritten value is correct), then boxes it on the global heap // (NOT the bump arena — the clone holds `Box`/`Vec`/`MimallocArena` // fields that need `Drop` to run) and wires the self-referential - // `linker`/`macro_context`. The box is parked on + // `macro_context`. The box is parked on // `self.owned_client_transpiler` so `deinit_without_freeing_arena` // releases it. diff --git a/src/bundler/linker.rs b/src/bundler/linker.rs index 477f938087c9..ef82fd4e842f 100644 --- a/src/bundler/linker.rs +++ b/src/bundler/linker.rs @@ -21,15 +21,7 @@ use crate::transpiler::{ParseResult, ResolveQueue, ResolveResults}; pub struct Linker { // arena field dropped — global mimalloc (callers pass `bun.default_allocator`) - // `Transpiler` owns these values directly and also owns `linker: - // crate::Linker` by value, so storing references here would alias - // `&mut self` on every `transpiler.linker.link(...)` call. Use raw - // pointers and dereference at use-site; same - // contract as `transpiler::set_log`'s `linker.log = log as *mut _`. - pub(crate) options: *mut BundleOptions<'static>, pub log: *mut Log, - pub(crate) resolve_queue: *mut ResolveQueue, - pub(crate) resolve_results: *mut ResolveResults, } const RUNTIME_SOURCE_PATH: &[u8] = b"bun:wrap"; @@ -115,34 +107,6 @@ pub(crate) fn dupe(src: &[u8]) -> &'static [u8] { unsafe { ImportPathsList::append(relative_paths_list_ptr(), &src).expect("OOM") } } impl Linker { - // ── raw-pointer field accessors ────────────────────────────────────── - // The pointer fields are self-referential backrefs into the owning - // `Transpiler` (sibling fields), wired in `configure_linker*`. They are - // briefly null between `Transpiler::init` and `configure_linker`, but the - // contract is that no `link()`/`enqueue_*` call - // happens before `configure_linker` runs. Centralize the deref + invariant - // here so call sites are safe-Rust. - - /// Shared borrow of the owning `Transpiler.options`. - /// - /// SAFETY: `self.options` points at the sibling `Transpiler.options` field - /// (set via `addr_of_mut!` in `configure_linker*`). The `Transpiler` - /// outlives all `Linker` method calls, and `options` is not mutated for - /// the duration of any borrow returned here (callers only read scalar - /// config like `target` / `preserve_extensions`). Never null once - /// `configure_linker` has run. - #[inline] - pub(crate) fn options(&self) -> &BundleOptions<'static> { - debug_assert!( - !self.options.is_null(), - "Linker.options used before configure_linker" - ); - // SAFETY: non-null sibling backref into the owning `Transpiler` set in - // `configure_linker*`; the `Transpiler` outlives every `Linker` call and - // `options` is not mutated while this borrow is live. - unsafe { &*self.options } - } - /// Exclusive borrow of the owning `Transpiler.log`. /// /// SAFETY: `self.log` is the `*mut Log` copied from `Transpiler.log` in @@ -159,74 +123,11 @@ impl Linker { unsafe { &mut *self.log } } - /// Exclusive borrow of the owning `Transpiler.resolve_results`. - /// - /// SAFETY: sibling-field backref wired via `addr_of_mut!` in - /// `configure_linker*`. The only caller (`enqueue_resolve_result`) is - /// reached via `Transpiler::enqueue_entry_points`, which holds no other - /// borrow of `self.resolve_results` across the call. Never null after - /// `configure_linker`. - #[inline] - pub(crate) fn resolve_results_mut(&mut self) -> &mut ResolveResults { - debug_assert!( - !self.resolve_results.is_null(), - "Linker.resolve_results used before configure_linker" - ); - // SAFETY: non-null sibling backref wired in `configure_linker*`; the sole - // caller holds no other borrow of `Transpiler.resolve_results` across the - // call, so the `&mut` is exclusive. - unsafe { &mut *self.resolve_results } - } - - /// Exclusive borrow of the owning `Transpiler.resolve_queue`. - /// - /// SAFETY: sibling-field backref wired via `addr_of_mut!` in - /// `configure_linker*`. Disjoint from `resolve_results`; the only caller - /// holds no other borrow of `self.resolve_queue` across the call. Never - /// null after `configure_linker`. - #[inline] - pub(crate) fn resolve_queue_mut(&mut self) -> &mut ResolveQueue { - debug_assert!( - !self.resolve_queue.is_null(), - "Linker.resolve_queue used before configure_linker" - ); - // SAFETY: non-null sibling backref wired in `configure_linker*`; disjoint - // from `resolve_results` and the sole caller holds no other borrow of - // `Transpiler.resolve_queue`, so the `&mut` is exclusive. - unsafe { &mut *self.resolve_queue } - } - - pub(crate) fn init( - log: *mut Log, - resolve_queue: *mut ResolveQueue, - options: *mut BundleOptions<'static>, - resolve_results: *mut ResolveResults, - ) -> Self { + pub(crate) fn init(log: *mut Log) -> Self { // The `LazyLock` accessor initializes `relative_paths_list` lazily on // first `intern_path()` / `relative_paths_list()` call, so no eager // poke is needed (it would be startup overhead for non-bundling code paths). - Self { - options, - log, - resolve_queue, - resolve_results, - } - } - - /// Re-seat the self-referential back-pointers after the owning - /// `Transpiler` has been moved to its final address. Use instead of `init` - /// from `Transpiler::wire_after_move`. - pub(crate) fn reseat_self_refs( - &mut self, - log: *mut Log, - resolve_queue: *mut ResolveQueue, - options: *mut BundleOptions<'static>, - resolve_results: *mut ResolveResults, - ) { - self.log = log; - self.resolve_queue = resolve_queue; - self.options = options; - self.resolve_results = resolve_results; + Self { log } } // ── getModKey / getHashedFilename ──────────────────────────────────── @@ -282,17 +183,13 @@ impl Linker { /// crate doesn't enable `adt_const_params`. pub fn link( &mut self, + options: &BundleOptions<'_>, file_path: &Fs::Path<'_>, result: &mut ParseResult, origin: &URL<'_>, import_path_format: ImportPathFormat, ) -> crate::Result<()> { - // Copy out the two scalar config values we read so the `&self` borrow - // from `options()` doesn't overlap the later `&mut self` call (`log_mut`). - let (target, rewrite_jest_for_tests) = { - let opts = self.options(); - (opts.target, opts.rewrite_jest_for_tests) - }; + let (target, rewrite_jest_for_tests) = (options.target, options.rewrite_jest_for_tests); let source_dir = file_path.source_dir(); @@ -487,16 +384,17 @@ impl Linker { } pub(crate) fn enqueue_resolve_result( - &mut self, + resolve_results: &mut ResolveResults, + resolve_queue: &mut ResolveQueue, resolve_result: resolver::Result<'static>, ) -> crate::Result { let hash_key = Self::resolve_result_hash_key(&resolve_result); // `found_existing` is whether the key was already present. - let found_existing = self.resolve_results_mut().contains_key(&hash_key); + let found_existing = resolve_results.contains_key(&hash_key); if !found_existing { - self.resolve_results_mut().insert(hash_key, ()); - self.resolve_queue_mut().push_back(resolve_result); + resolve_results.insert(hash_key, ()); + resolve_queue.push_back(resolve_result); } Ok(!found_existing) diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index 53c17cf06e34..b9901e1c76eb 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -52,7 +52,7 @@ pub struct Transpiler<'a> { pub(crate) elapsed: u64, // `ModuleLoader::transpile_source_code` (jsc_hooks.rs) calls - // `transpiler.linker.link()`. Back-pointers wired + // `transpiler.linker.link()`. `linker.log` is set // by `configure_linker` below; `set_log` keeps `linker.log` in sync. pub linker: crate::linker::Linker, // Raw ptr — the global `DotEnv::Loader` singleton. @@ -227,14 +227,7 @@ impl<'a> Transpiler<'a> { resolve_results: Box::new(ResolveResults::default()), resolve_queue: ResolveQueue::default(), elapsed: 0, - // Self-referential — wired by `wire_after_move`. Null back-pointers - // for now (matches `Transpiler::init`; never derefed before then). - linker: crate::linker::Linker::init( - log, - core::ptr::null_mut(), - core::ptr::null_mut(), - core::ptr::null_mut(), - ), + linker: crate::linker::Linker::init(log), env: from.env, // `MacroContext::init(transpiler)` takes the // transpiler's *address*; deferred to `wire_after_move`. @@ -242,7 +235,7 @@ impl<'a> Transpiler<'a> { } } - /// Wire the self-referential `linker` back-pointers and `macro_context` + /// Wire the aliases of `log` and the self-referential `macro_context` /// after this `Transpiler` has reached its final address (post-move into /// `WorkerData` / arena slot). pub(crate) fn wire_after_move(&mut self) { @@ -252,12 +245,7 @@ impl<'a> Transpiler<'a> { let log = self.log; self.options.log = log; self.resolver.log = core::ptr::NonNull::new(log).expect("wire_after_move: log is non-null"); - self.linker.reseat_self_refs( - log, - core::ptr::addr_of_mut!(self.resolve_queue), - core::ptr::addr_of_mut!(self.options).cast(), - core::ptr::addr_of_mut!(*self.resolve_results), - ); + self.linker.log = log; self.macro_context = Some(js_ast::Macro::MacroContext::init(self)); } @@ -571,16 +559,10 @@ fn merge_tsconfig_jsx_into(tsconfig: &TSConfigJSON, out: &mut crate::options_imp } impl<'a> Transpiler<'a> { - /// Initialize `self.linker` with back-pointers into this `Transpiler`, + /// Initialize `self.linker` with the log of this `Transpiler`, /// optionally auto-configuring JSX from the nearest `tsconfig.json`. pub fn configure_linker_with_auto_jsx(&mut self, auto_jsx: bool) { - // Raw back-pointers into `self`; the linker never outlives this `Transpiler`. - self.linker = crate::linker::Linker::init( - self.log, - core::ptr::addr_of_mut!(self.resolve_queue), - core::ptr::addr_of_mut!(self.options).cast(), - core::ptr::addr_of_mut!(*self.resolve_results), - ); + self.linker = crate::linker::Linker::init(self.log); if auto_jsx { // Most of the time, this will already be cached @@ -1197,9 +1179,7 @@ impl<'a> Transpiler<'a> { // Construct directly into the caller-owned storage instead of building a // stack temporary and returning it. All fallible work is done; every - // field below is written exactly once. `Linker::init` gets null - // back-pointers; `configure_linker_with_auto_jsx` overwrites the - // self-referential pointers before any deref. + // field below is written exactly once. let p = dst.as_mut_ptr(); // SAFETY: `dst` is an exclusively-borrowed, currently-uninitialised // `MaybeUninit`; each `write` initialises a distinct field @@ -1222,12 +1202,7 @@ impl<'a> Transpiler<'a> { core::ptr::addr_of_mut!((*p).resolve_queue).write(ResolveQueue::default()); core::ptr::addr_of_mut!((*p).elapsed).write(0); // .thread_pool = pool, - core::ptr::addr_of_mut!((*p).linker).write(crate::linker::Linker::init( - log, - core::ptr::null_mut(), - core::ptr::null_mut(), - core::ptr::null_mut(), - )); + core::ptr::addr_of_mut!((*p).linker).write(crate::linker::Linker::init(log)); core::ptr::addr_of_mut!((*p).env).write(env_loader); core::ptr::addr_of_mut!((*p).macro_context).write(None); } @@ -2602,7 +2577,7 @@ impl<'a> Transpiler<'a> { } /// Returns only the count and lets - /// `linker.enqueue_resolve_result` push directly onto `resolve_queue`. + /// `Linker::enqueue_resolve_result` push directly onto `resolve_queue`. fn enqueue_entry_points(&mut self) -> usize { let mut entry_point_i: usize = 0; @@ -2640,10 +2615,12 @@ impl<'a> Transpiler<'a> { continue; } - if self - .linker - .enqueue_resolve_result(result) - .expect("unreachable") + if crate::linker::Linker::enqueue_resolve_result( + &mut self.resolve_results, + &mut self.resolve_queue, + result, + ) + .expect("unreachable") { entry_point_i += 1; } @@ -2923,6 +2900,7 @@ impl<'a> Transpiler<'a> { let origin = self.options.origin.url(); if !self.options.target.is_bun() { self.linker.link::( + &self.options, &file_path, &mut result, &origin, @@ -2930,6 +2908,7 @@ impl<'a> Transpiler<'a> { )?; } else { self.linker.link::( + &self.options, &file_path, &mut result, &origin, diff --git a/src/install/PackageManager.rs b/src/install/PackageManager.rs index 0b6b9964575d..e636d7e80f8f 100644 --- a/src/install/PackageManager.rs +++ b/src/install/PackageManager.rs @@ -1110,7 +1110,7 @@ impl PackageManager { ) -> Result<&mut transpiler::Transpiler<'static>, Error> { // Compute once and return the cached value on // subsequent calls. `Transpiler` is non-`Copy` (and self-referential via - // `linker.options = &options`), so cache by pointer in a process-static. + // `macro_context`), so cache by pointer in a process-static. // SAFETY: `PackageManager` is a leaked singleton; main-thread-only call site. let mut ptr = CONFIGURE_ENV_FOR_SCRIPTS_ONCE.load(core::sync::atomic::Ordering::Acquire); if ptr.is_null() { diff --git a/src/jsc/AsyncModule.rs b/src/jsc/AsyncModule.rs index 97d2313318ee..0b9d76376fa2 100644 --- a/src/jsc/AsyncModule.rs +++ b/src/jsc/AsyncModule.rs @@ -1053,9 +1053,10 @@ impl AsyncModule { // We _must_ link because: // - node_modules bundle won't be properly - // SAFETY: per-thread VM; `linker` is a value field of `transpiler`. + // SAFETY: per-thread VM; `linker` and `options` are distinct value fields of `transpiler`. unsafe { (*jsc_vm).transpiler.linker.link::( + &(*jsc_vm).transpiler.options, &path, &mut parse_result, &(*jsc_vm).origin, diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index 0a3c8db5ba41..f9dcda770cac 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -462,11 +462,7 @@ unsafe fn init_runtime_state( // field is zero-init'd uninhabited memory (never dropped). unsafe { ptr::write(ptr::addr_of_mut!((*vm).transpiler), transpiler) }; // Post-`Transpiler.init` wiring that must run BEFORE - // `JSGlobalObject.create`. `configure_linker` MUST run - // after the `ptr::write` above so the self-referential - // `addr_of_mut!(self.options)` etc. captured by `Linker::init` - // point at the final `(*vm).transpiler` storage, not the moved- - // from stack temporary. + // `JSGlobalObject.create`. // SAFETY: `vm` unique on this thread; `transpiler` just written. unsafe { let t = &mut (*vm).transpiler; @@ -3038,9 +3034,10 @@ fn transpile_source_code_of_any_provenance( // Link import records. // SAFETY: per fn contract — `jsc_vm` is the live per-thread VM; - // `origin` is read-only and `linker` is JS-thread-exclusive. + // `origin` and `options` are only read and `linker` is JS-thread-exclusive. unsafe { (*jsc_vm).transpiler.linker.link::( + &(*jsc_vm).transpiler.options, path, &mut parse_result, &(*jsc_vm).origin, From 11e23381ecbc4794d2915c7118b1331c36d146e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:00 +0000 Subject: [PATCH 23/27] transpiler: `Transpiler::init` builds its value with a struct literal `init` made a `MaybeUninit` on its stack and passed it to `init_in_place`, which wrote the twelve fields with `addr_of_mut!(..).write(..)`. `init` then returned `Ok(slot.assume_init())`. `init_in_place` was meant for callers that own the final storage, but it is `pub(crate)` and `init` was its only caller: the VM, in another crate, calls `init` and writes the result into its field. `init` now has the body of `init_in_place` and ends in `Ok(Transpiler { .. })`, with the fields in the same order, so the calls that compute them run in the same order. All the fallible work still comes first. No large value is moved more often than before. The options went from a local into the slot and now go from the same local into the literal. The resolver went from the result of `Resolver::init1` into the slot and now goes into the literal. The whole struct went from the slot into the `Ok` and now goes from the literal into the `Ok`. The message of the panic for a null `log` names `init` instead of `init_in_place`. --- src/bundler/options.rs | 4 +-- src/bundler/transpiler.rs | 68 +++++++++------------------------------ src/runtime/jsc_hooks.rs | 6 ++-- 3 files changed, 21 insertions(+), 57 deletions(-) diff --git a/src/bundler/options.rs b/src/bundler/options.rs index fdda7fc6f16c..3016339f831a 100644 --- a/src/bundler/options.rs +++ b/src/bundler/options.rs @@ -1692,9 +1692,9 @@ impl<'a> BundleOptions<'a> { /// Shared-borrow the per-Transpiler `Log`. /// - /// SAFETY: `self.log` is non-null: `Transpiler::init_in_place` validates + /// SAFETY: `self.log` is non-null: `Transpiler::init` validates /// the pointer via `NonNull::new(log).expect(..)` before storing it and - /// before calling `from_api` (which has no other callers). + /// before calling `from_api`. /// The pointee is the caller-owned arena `Log` which outlives `self`. The /// same allocation is aliased into `Transpiler.log` / `Resolver.log` / /// `Linker.log` as raw `*mut`; a `&` here is sound so long as no caller diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index b9901e1c76eb..1f4678fa6c28 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -1066,34 +1066,10 @@ impl<'a> Transpiler<'a> { opts: api::TransformOptions, env_loader_: Option<*mut dot_env::Loader>, ) -> crate::Result> { - let mut slot = core::mem::MaybeUninit::>::uninit(); - Self::init_in_place(&mut slot, arena, log, opts, env_loader_)?; - // SAFETY: `init_in_place` returned `Ok`, so every field of `slot` was - // written exactly once. - Ok(unsafe { slot.assume_init() }) - } - - /// In-place sibling of [`Self::init`]: builds the `Transpiler` directly into - /// `dst` rather than returning it by value, so callers that already own its - /// final storage — most importantly `VirtualMachine.transpiler`, written by - /// [`init_runtime_state`](../runtime/jsc_hooks.rs) once per VM — avoid the - /// multi-KB `stack temporary → return slot → final home` double `memcpy`. - /// - /// On `Ok(())`, every field of `dst` is initialised. On `Err`, `dst` is - /// untouched (all fallible work happens before the first field write), so the - /// caller must not `assume_init` it. - pub(crate) fn init_in_place( - dst: &mut core::mem::MaybeUninit>, - arena: &'a Arena, - log: *mut bun_ast::Log, - opts: api::TransformOptions, - env_loader_: Option<*mut dot_env::Loader>, - ) -> crate::Result<()> { // Caller contract: `log` is the freshly-boxed per-VM `Log` from // `VirtualMachine::init` and is never null. Validate up front so the // deref sites below go through `NonNull` rather than the raw argument. - let log_nn = - core::ptr::NonNull::new(log).expect("Transpiler::init_in_place: log is non-null"); + let log_nn = core::ptr::NonNull::new(log).expect("Transpiler::init: log is non-null"); bun_ast::expr::data::Store::create(); bun_ast::stmt::data::Store::create(); // These two `create()`s are eager (not deferred to the first `parse()`) @@ -1177,36 +1153,24 @@ impl<'a> Transpiler<'a> { let outbase = bundle_options.output_dir.clone(); let resolve_results = Box::new(ResolveResults::default()); - // Construct directly into the caller-owned storage instead of building a - // stack temporary and returning it. All fallible work is done; every - // field below is written exactly once. - let p = dst.as_mut_ptr(); - // SAFETY: `dst` is an exclusively-borrowed, currently-uninitialised - // `MaybeUninit`; each `write` initialises a distinct field - // and no field is read before it is written. - unsafe { - core::ptr::addr_of_mut!((*p).options).write(bundle_options); - core::ptr::addr_of_mut!((*p).log).write(log_nn.as_ptr()); - core::ptr::addr_of_mut!((*p).arena).write(arena); - core::ptr::addr_of_mut!((*p).result).write(options::TransformResult { + Ok(Transpiler { + options: bundle_options, + log: log_nn.as_ptr(), + arena, + result: options::TransformResult { outbase, ..Default::default() - }); - core::ptr::addr_of_mut!((*p).resolver).write(Resolver::init1( - log_nn, - fs, - resolver_opts, - )); - core::ptr::addr_of_mut!((*p).output_files).write(Vec::new()); - core::ptr::addr_of_mut!((*p).resolve_results).write(resolve_results); - core::ptr::addr_of_mut!((*p).resolve_queue).write(ResolveQueue::default()); - core::ptr::addr_of_mut!((*p).elapsed).write(0); + }, + resolver: Resolver::init1(log_nn, fs, resolver_opts), + output_files: Vec::new(), + resolve_results, + resolve_queue: ResolveQueue::default(), + elapsed: 0, // .thread_pool = pool, - core::ptr::addr_of_mut!((*p).linker).write(crate::linker::Linker::init(log)); - core::ptr::addr_of_mut!((*p).env).write(env_loader); - core::ptr::addr_of_mut!((*p).macro_context).write(None); - } - Ok(()) + linker: crate::linker::Linker::init(log), + env: env_loader, + macro_context: None, + }) } /// `'p` is the parse's own lifetime (arena, path, virtual source), not the `Transpiler`'s. diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index f9dcda770cac..3e609d4a9076 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -516,7 +516,7 @@ unsafe fn init_runtime_state( // Unwind the per-VM state this hook set up before the // `Transpiler::init` attempt: the `RuntimeState` box + its TLS // slot (set above) and the thread-local AST stores that - // `Transpiler::init_in_place` `create()`d before it failed. + // `Transpiler::init` `create()`d before it failed. // Mirrors `deinit_runtime_state`, which is the teardown the // `Ok` path would otherwise reach. RUNTIME_STATE.with(|c| c.set(ptr::null_mut())); @@ -734,7 +734,7 @@ unsafe fn deinit_runtime_state(_vm: *mut VirtualMachine, state: OpaqueRuntimeSta debug_assert!(state.cron_jobs.is_empty()); drop(state); } - // Free the thread-local AST stores allocated by `Transpiler::init_in_place` + // Free the thread-local AST stores allocated by `Transpiler::init` // (via `Store::create()`). They live in TLS without a Drop, so each worker // thread strands a `Box` plus its lazily-allocated block chain when // the thread exits. `deinit()` is a no-op if a bundler arena currently owns @@ -2474,7 +2474,7 @@ fn transpile_source_code_of_any_provenance( // "uninitialized Transpiler" gate was stale. // SAFETY: per fn contract — `jsc_vm` is the live per-thread VM. // `transpiler.log` / `args.log` are never-null (set in - // `Transpiler::init_in_place` / by the C++ caller respectively). + // `Transpiler::init` / by the C++ caller respectively). let old_log = unsafe { &*jsc_vm }.transpiler.log; let old_log_nn = core::ptr::NonNull::new(old_log).expect("transpiler.log is non-null"); let args_log_nn = core::ptr::NonNull::new(args.log).expect("args.log is non-null"); From 1e028bb83cc98e17a5760082d02907a84bd40e38 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:01 +0000 Subject: [PATCH 24/27] bundler: three worker contexts that only read the linker hold `&LinkerContext` `find_all_imported_parts_in_js_order` and `run_walks` put `ParentRef::from_ref_mut(this)` in the context they hand to `each_ptr`, and each task turned it back with `unsafe { &*ctx.c.as_mut_ptr() }`. The first of the two built a whole `GenerateChunkCtx` and used only its `c`. Step 5 of `scan_imports_and_exports` wrapped a `*mut LinkerContext` in a struct with its own `unsafe impl Sync`, and `do_step5` began with `&*this`. The three contexts now hold `&LinkerContext`. `each` and `each_ptr` ask for `Ctx: Sync`, which `&LinkerContext` has through the existing `unsafe impl Sync for LinkerContext`. `find_all_imported_parts_in_js_order` and `run_walks` took `&mut LinkerContext` for `from_ref_mut`, and `find_safe_walks` took it to call `run_walks`. All three take `&LinkerContext`, and `do_step5` takes `&self`. It stays an `unsafe fn`: the caller must give each concurrent call its own rows. While the pool runs, the calling thread is blocked in `each` / `each_ptr` and holds only shared borrows of the linker: the context and `worker_pool()`. The tasks hold `&LinkerContext`, as before, and write the rows and the one column they own through the raw pointers of `split_raw(&self)`, which come from the buffer pointer stored in the list and not from the reference. `unsafe impl Send for PrepareCssAstTask` is removed: no bound asks for it. The tasks stay in a local `Vec` and reach the pool as `*mut Task`. The change is in the types only, with two exceptions. `find_all_imported_parts_in_js_order` no longer forms the two unused pointers into `chunks`. And the attributes of the changed parameters follow their types: the three that were `&mut LinkerContext` lose `noalias` and `dereferenceable`, which a `&` to a type with atomics in it does not get, and `do_step5`'s, which was a raw pointer, gains `nonnull` and `align`. --- src/bundler/linker_context/computeChunks.rs | 2 +- src/bundler/linker_context/doStep5.rs | 14 ++++------ .../findAllImportedPartsInJSOrder.rs | 26 +++++++------------ .../linker_context/prepareCssAstsForChunk.rs | 11 +------- .../linker_context/scanImportsAndExports.rs | 20 ++++---------- 5 files changed, 21 insertions(+), 52 deletions(-) diff --git a/src/bundler/linker_context/computeChunks.rs b/src/bundler/linker_context/computeChunks.rs index 1d11728e08e1..efa9484a8668 100644 --- a/src/bundler/linker_context/computeChunks.rs +++ b/src/bundler/linker_context/computeChunks.rs @@ -589,7 +589,7 @@ pub(crate) fn compute_chunks<'a>( unsafe { &mut *LinkerContext::bundle_v2_ptr(std::ptr::from_mut::(this)) }; let kinds = this.graph.files.items_entry_point_kind(); let output_paths = this.graph.entry_points.items_output_path(); - // re-borrow after `find_all_imported_parts_in_js_order` released `&mut this`. + // re-borrow after `from_mut(this)` took `&mut *this`. let ast_targets = this.graph.ast.items_target(); let mut compile_entry_point_pending = !bv2.transpiler().options.compile_entry_point_name.is_empty(); diff --git a/src/bundler/linker_context/doStep5.rs b/src/bundler/linker_context/doStep5.rs index 14ecbf0b2c28..0d832498e1f9 100644 --- a/src/bundler/linker_context/doStep5.rs +++ b/src/bundler/linker_context/doStep5.rs @@ -39,32 +39,28 @@ impl LinkerContext<'_> { /// # Safety /// /// Runs concurrently on worker-pool threads (one task per `source_index`). - /// The body never materializes `&mut LinkerContext` — it derefs `this` to a + /// The body never materializes `&mut LinkerContext` — it uses `self`, a /// shared `&LinkerContext` for read-only access (`symbols`, `ts_enums`, /// `top_level_symbols_to_parts`, options) and writes only to its own /// `source_index` row of the `graph.{ast,meta}` SoA columns via raw /// per-row pointers obtained from `split_raw()` (root provenance, no /// `&mut [T]` intermediate). Disjoint rows ⇒ no overlapping `&mut`. - pub(crate) unsafe fn do_step5(this: *mut LinkerContext<'_>, source_index_: Index, _: usize) { + pub(crate) unsafe fn do_step5(&self, source_index_: Index, _: usize) { let source_index = source_index_.get(); let _trace = perf::trace("Bundler.CreateNamespaceExports"); - // SAFETY: shared-ref view for all read-only access. Multiple worker threads may - // hold `&LinkerContext` simultaneously; the SoA buffers live behind raw - // pointers inside `MultiArrayList`, so this borrow does not assert - // immutability over the heap cells we write below. - let c: &LinkerContext<'_> = unsafe { &*this }; + let c: &LinkerContext<'_> = self; let id = source_index; if id as usize >= c.graph.meta.len() { return; } - // SAFETY: `this` points to `BundleV2.linker` (caller is the worker-pool + // SAFETY: `c` is `BundleV2.linker` (caller is the worker-pool // dispatch from `scan_imports_and_exports`); `container_of` shape. // `Worker::get` only needs `&BundleV2`, so derive a shared ref — never // form `&mut BundleV2` here (concurrent tasks would alias it). - let bundle_v2: &BundleV2<'_> = unsafe { &*LinkerContext::bundle_v2_ptr(this) }; + let bundle_v2: &BundleV2<'_> = unsafe { &*LinkerContext::bundle_v2_const_ptr(c) }; let worker = ThreadPool::Worker::get(bundle_v2); // `Worker::get` returns the thread-local worker // (not RAII), so balance with `unget` explicitly via scopeguard. diff --git a/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs b/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs index 9d1337327da8..02ae5bc51d15 100644 --- a/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs +++ b/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs @@ -8,7 +8,7 @@ use crate::{Chunk, EntryPoint, Index, IndexInt, LinkerContext, PartRange, chunk, use bun_core::perf; pub(crate) fn find_all_imported_parts_in_js_order<'a>( - this: &mut LinkerContext<'a>, + this: &LinkerContext<'a>, chunks: &mut [Chunk<'a>], ) -> Result<(), crate::Error> { let _trace = perf::trace("Bundler.findAllImportedPartsInJSOrder"); @@ -42,7 +42,7 @@ pub(crate) fn find_all_imported_parts_in_js_order<'a>( let order = WalkOrder::collect(this, chunks.len(), walks); struct Ctx<'a, 'f> { - inner: crate::linker_context_mod::GenerateChunkCtx<'a>, + c: &'f LinkerContext<'a>, chunk_of_file: &'f [u32], order: &'f WalkOrder, } @@ -51,11 +51,7 @@ pub(crate) fn find_all_imported_parts_in_js_order<'a>( // server-component files, that file's `entry_point_chunk_index` slot (a // file belongs to exactly one chunk), and reads the graph columns. let ctx = Ctx { - inner: crate::linker_context_mod::GenerateChunkCtx { - chunk: bun_ptr::BackRef::new_mut(&mut chunks[0]), - c: bun_ptr::ParentRef::from_ref_mut(this), - chunks: bun_ptr::BackRef::new(&*chunks), - }, + c: this, chunk_of_file: &chunk_of_file, order: &order, }; @@ -68,10 +64,8 @@ pub(crate) fn find_all_imported_parts_in_js_order<'a>( if !matches!(chunk.content, chunk::Content::Javascript(_)) { return; // CSS: `find_imported_css_files_in_js_order`; HTML: nothing } - // SAFETY: shared for reading; see the note above on the one column written. - let c: &LinkerContext = unsafe { &*ctx.inner.c.as_mut_ptr() }; bun_core::handle_oom(find_imported_parts_in_js_order( - c, + ctx.c, chunk, u32::try_from(index).expect("int cast"), ctx.chunk_of_file, @@ -85,18 +79,18 @@ pub(crate) fn find_all_imported_parts_in_js_order<'a>( } fn run_walks<'a>( - this: &mut LinkerContext<'a>, + this: &LinkerContext<'a>, plan: &WalkPlan, chunks: &[Chunk<'a>], walks: &mut [EntryWalk], ) { struct WalkCtx<'a, 'f> { - c: bun_ptr::ParentRef, bun_ptr::Mut>, + c: &'f LinkerContext<'a>, plan: &'f WalkPlan, chunks: &'f [Chunk<'a>], } let walk_ctx = WalkCtx { - c: bun_ptr::ParentRef::from_ref_mut(this), + c: this, plan, chunks, }; @@ -105,9 +99,7 @@ fn run_walks<'a>( |ctx: &WalkCtx, walk: *mut EntryWalk, _: usize| { // SAFETY: `each_ptr` hands each task a distinct `*mut EntryWalk`. let walk = unsafe { &mut *walk }; - // SAFETY: the walks only read the graph. - let c: &LinkerContext = unsafe { &*ctx.c.as_mut_ptr() }; - walk.run(c, ctx.plan, ctx.chunks); + walk.run(ctx.c, ctx.plan, ctx.chunks); }, walks, ); @@ -340,7 +332,7 @@ impl WalkPlan { /// walks of the first such entry point by rank, each with the chunks that it lays out in place of their owner. fn find_safe_walks<'a>( &mut self, - this: &mut LinkerContext<'a>, + this: &LinkerContext<'a>, chunks: &[Chunk<'a>], walks: &[EntryWalk], ) -> Vec { diff --git a/src/bundler/linker_context/prepareCssAstsForChunk.rs b/src/bundler/linker_context/prepareCssAstsForChunk.rs index 59b53c7d4321..6cc70b24d1d9 100644 --- a/src/bundler/linker_context/prepareCssAstsForChunk.rs +++ b/src/bundler/linker_context/prepareCssAstsForChunk.rs @@ -30,22 +30,13 @@ pub(crate) struct PrepareCssAstTask { pub(crate) linker: *mut LinkerContext<'static>, } -// SAFETY: scheduled on the worker pool via raw `*mut Task` (bypassing the -// `OwnedTask: Send` route). Both raw-ptr fields point at `Send` types -// (`Chunk: Send`, `LinkerContext: Send`); the callback writes only the -// per-chunk `chunk.content.css` cell (see `prepare_css_asts_for_chunk` -// CONCURRENCY note). -unsafe impl Send for PrepareCssAstTask {} - // CONCURRENCY: thread-pool callback — runs on worker threads, one task per // CSS chunk. Writes: `chunk.content.css.{asts, ordered_import_records}` // (per-chunk disjoint via `*mut Chunk`). Reads `linker.parse_graph` // SoA columns + `linker.graph.ast.css` shared. Every CSS chunk gets exactly // one task, so `&mut *chunk` is unique; `linker` is shared across all tasks // and is therefore borrowed as `&LinkerContext` (the impl only reads `c` and -// only ever writes `chunk`). `PrepareCssAstTask` is `Send` by virtue of -// `LinkerContext: Send` + `Chunk: Send` (both raw-ptr fields point at types -// with `unsafe impl Send`). +// only ever writes `chunk`). /// # Safety /// /// `task` must be the intrusive `task` field of a live [`PrepareCssAstTask`] diff --git a/src/bundler/linker_context/scanImportsAndExports.rs b/src/bundler/linker_context/scanImportsAndExports.rs index e1790d6cce2f..c2de4b8dc18e 100644 --- a/src/bundler/linker_context/scanImportsAndExports.rs +++ b/src/bundler/linker_context/scanImportsAndExports.rs @@ -655,22 +655,12 @@ pub(crate) fn scan_imports_and_exports( // // `do_step5` only touches distinct SoA rows per `source_index` (the // columns are pre-sized and never reallocate during this step). - // We pass `*mut LinkerContext` through a - // `Sync` wrapper; the callee derefs it to `&LinkerContext` (shared) + // The tasks share one `&LinkerContext`; the callee uses it // for reads and writes per-row cells via raw `split_raw()` pointers — // mirroring `GenerateChunkCtx` (`generate_js_renamer` likewise never // forms `&mut LinkerContext`). { - #[repr(transparent)] - struct Step5Ctx<'a>(*mut LinkerContext<'a>); - // SAFETY: the pointer is only dereferenced to `&LinkerContext` for - // reads plus raw `*mut` per-row SoA cells for disjoint-row writes - // (see `do_step5` doc); lifetime is bounded by the blocking - // `each()` call below. The `ThreadPool::Worker` it acquires is - // per-OS-thread. - unsafe impl core::marker::Sync for Step5Ctx<'_> {} - - let ctx = Step5Ctx(core::ptr::from_mut::>(this)); + let ctx: &LinkerContext<'_> = this; // SAFETY: `parse_graph` is the `BundleV2.graph` backref (valid for // the link step); `pool` is the arena-allocated bundler ThreadPool // set in `BundleV2::init`. @@ -680,9 +670,9 @@ pub(crate) fn scan_imports_and_exports( // variant). Step 6 reuses it afterwards. worker_pool.each( ctx, - |ctx: &Step5Ctx<'_>, source_index: Index, i: usize| { - // SAFETY: see `Step5Ctx` Sync justification above. - unsafe { LinkerContext::do_step5(ctx.0, source_index, i) }; + |ctx: &&LinkerContext<'_>, source_index: Index, i: usize| { + // SAFETY: `reachable` names each file once, so each call has its own rows. + unsafe { ctx.do_step5(source_index, i) }; }, &mut reachable[..], ); From a3dff79ccc9d55007bab81b18f54cc3fa504edfe Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:01 +0000 Subject: [PATCH 25/27] bundler: `LinkerContext::load` and `link` take the fields they use, not `*mut BundleV2` Both were `unsafe fn(&mut self, bundle: *mut BundleV2, ..)`, because `self.linker.link(self, ..)` does not borrow-check. All five callers hold a `&mut BundleV2` or the `Box`, and each made a raw pointer from it in an `unsafe` block to project `linker`, `graph.entry_points` and `graph.server_component_boundaries`. Inside there were nine more blocks: five project or read a field of `*bundle` that is disjoint from `linker` (`graph`, `dynamic_import_entry_points`, `transpiler`, `has_any_top_level_await_modules`, `unique_key`), two read the graph through `self.parse_graph`, one builds the resolver's `ParentRef` through a pointer cast, and one is `link`'s call of `load`. `load` now takes `&mut Graph`, `&Transpiler` and the `&mut` map of dynamic import entry points, which the callers borrow as disjoint fields next to `&mut self.linker`. It reads the entry points, the boundaries, the sources and the original names from the graph. The pointer cast that "erased" the resolver's lifetime is gone: the lifetime was `'a` already. `link` takes the `bool` and the `u64` it read from the bundle and no longer calls `load`. The new `BundleV2::link` calls both, and four callers call that. The dev server only loads, and calls `load` as before. `load` stays an `unsafe fn`, with a smaller contract. That its arguments are valid and disjoint from `self` is now checked by the compiler. What is left is that `self` keeps the raw `parse_graph` pointer and a `ParentRef` to the transpiler's resolver after those borrows have ended, and that safe accessors such as `parse_graph()` and `resolver()` dereference them later. So both arguments must be those of the `BundleV2` that holds `self`. `BundleV2::link` and the dev server call `load` in one `unsafe` block each. `link` is a safe function. It relies on `load` having run, like its callees `parse_graph()` and `compute_chunks`, which are safe functions too. `load` is separate so that no `&mut Graph` is live while the pool works on this bundle. It runs after parsing is over and before any task of the link step is scheduled, and its borrows end when it returns. `BundleV2::link`'s own `&mut self` does span the link step, and the workers' pointers derive from it. `generate_from_cli` and `generate_from_bake_production_cli` held only the `Box` and a raw pointer there. `LinkerContext::link`, which schedules the source map tasks and step 5, holds `&mut LinkerContext` as before, and two fewer borrows into the graph: the entry points and the boundaries were parameters of it. `load` is called from `BundleV2::link` instead of from `LinkerContext::link`, and the two scalars are read before `LinkerContext::link` is called instead of during it. Nothing writes them while linking. The stores in `load` keep their order. --- src/bundler/LinkerContext.rs | 79 +++++++--------------- src/bundler/aot/page_bundles.rs | 11 +--- src/bundler/bundle_v2.rs | 98 +++++++++------------------- src/bundler/linker_context/README.md | 2 +- 4 files changed, 56 insertions(+), 134 deletions(-) diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index bfac37122ee5..de49190780d5 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -38,7 +38,7 @@ use crate::Graph::Graph; use crate::options::{CompileMode, Format, Loader, SourceMapOption, Target}; use crate::{ AdditionalFile, BundleV2, Chunk, CompileResultForSourceMap, ContentHasher, ImportTracker, - LinkerGraph, MangledProps, PartRange, StableRef, WrapKind, + LinkerGraph, MangledProps, PartRange, StableRef, Transpiler, WrapKind, }; /// `bun_event_loop` is a @@ -519,50 +519,26 @@ impl<'a> LinkerContext<'a> { true } - /// `bundle` is taken as a raw `*mut` because the caller invokes this as - /// `self.linker.load(self, …)` — `self` *is* - /// `(*bundle).linker`, so a `&mut BundleV2` here would alias the receiver - /// under Stacked Borrows. This body only reaches into fields of `*bundle` - /// that are disjoint from `linker` (`graph`, `transpiler`, - /// `dynamic_import_entry_points`) via `addr_of_mut!`, never materializing a - /// full `&mut BundleV2`. + /// `self` keeps pointers into `parse_graph` and `transpiler`, which its safe accessors dereference later. /// /// # Safety - /// `bundle` must be valid for the call and `self` must be `(*bundle).linker` - /// (or otherwise not overlap the fields named above). + /// Both must be those of the `BundleV2` that holds `self`. pub(crate) unsafe fn load( &mut self, - bundle: *mut BundleV2<'a>, - entry_points: &[Index], - server_component_boundaries: &bun_ast::server_component_boundary::List, + parse_graph: &mut Graph<'a>, + transpiler: &Transpiler<'a>, + dynamic_import_entry_points: &mut ArrayHashMap, reachable: &[Index], ) -> Result<(), BunError> { let _trace = bun::perf::trace("Bundler.CloneLinkerGraph"); - // SAFETY: field-disjoint with `self` (= `(*bundle).linker`); `parse_graph` - // is a `*mut Graph` backref so no `&mut` is materialized. - self.parse_graph = unsafe { core::ptr::addr_of_mut!((*bundle).graph) }; - // SAFETY: field-disjoint scalar read; `transpiler` is itself a `*mut`. - let dyn_entry_points = - unsafe { &mut *core::ptr::addr_of_mut!((*bundle).dynamic_import_entry_points) }; - - // SAFETY: `bundle.transpiler` is a `*mut Transpiler` backref valid for - // the bundle's lifetime; `resolver`/`log`/`options` are stable fields. - let transpiler = unsafe { &mut *(*bundle).transpiler }; + self.parse_graph = core::ptr::from_mut(parse_graph); + self.graph.code_splitting = transpiler.options.code_splitting; // `transpiler.log` is the canonical // `*mut Log` (same value aliased into `linker.log` / `resolver.log`). self.log = transpiler.log; - // Note: lifetime — `self.resolver` is `ParentRef>` - // but `transpiler.resolver` is `Resolver<'_>` (anonymous `bundle` - // lifetime); erase via a pointer cast (LIFETIMES.tsv: GRAPHBACKED — - // resolver outlives the link step). Read-only — `from_raw` provenance - // is sufficient. - // SAFETY: `transpiler.resolver` is a stable field of the - // bundle-lifetime `Transpiler`, valid for the entire link step. - self.resolver = Some(unsafe { - bun_ptr::ParentRef::from_raw(core::ptr::from_ref(&transpiler.resolver).cast()) - }); + self.resolver = Some(bun_ptr::ParentRef::new(&transpiler.resolver)); self.cycle_detector = Vec::new(); self.inits_already_done = None; @@ -570,18 +546,17 @@ impl<'a> LinkerContext<'a> { // caller-owned slice into the linker arena. self.graph.reachable_files = reachable.to_vec(); - // SAFETY: parse_graph is valid backref just assigned above - let sources: &[Source] = unsafe { (*self.parse_graph).input_files.items_source() }; + let entry_points: &[Index] = &parse_graph.entry_points; self.graph.load( entry_points, - sources, - server_component_boundaries, - dyn_entry_points.keys(), - // SAFETY: parse_graph backref - unsafe { &(*self.parse_graph).entry_point_original_names }, + parse_graph.input_files.items_source(), + // Borrowed, not taken: `StaticRouteVisitor` reads it through `parse_graph` later. + &parse_graph.server_component_boundaries, + dynamic_import_entry_points.keys(), + &parse_graph.entry_point_original_names, )?; - dyn_entry_points.clear_retaining_capacity(); + dynamic_import_entry_points.clear_retaining_capacity(); let runtime_named_exports = &self.graph.ast.items_named_exports()[Index::RUNTIME.get() as usize]; @@ -823,22 +798,14 @@ impl<'a> LinkerContext<'a> { chunk.content.is_javascript() && loader.is_javascript_like() } - /// See [`Self::load`] for why `bundle` is a raw `*mut` (caller passes - /// `self` while the receiver is `self.linker`; field-disjoint access only). - /// - /// # Safety - /// `bundle` must be valid for the call and `self` must be `(*bundle).linker`. + /// Call [`Self::load`] first. #[inline(never)] - pub(crate) unsafe fn link( + pub(crate) fn link( &mut self, - bundle: *mut BundleV2<'a>, - entry_points: &[Index], - server_component_boundaries: &bun_ast::server_component_boundary::List, + has_any_top_level_await_modules: bool, + unique_key: u64, reachable: &[Index], ) -> Result]>, LinkError> { - // SAFETY: forwarded; see fn-level contract. - unsafe { self.load(bundle, entry_points, server_component_boundaries, reachable)? }; - if self.options.source_maps != SourceMapOption::None { self.compute_data_for_source_map(reachable); } @@ -850,8 +817,7 @@ impl<'a> LinkerContext<'a> { } // Validate top-level await for all files first. - // SAFETY: scalar `bool` read of a field disjoint from `self` (= `(*bundle).linker`). - if unsafe { (*bundle).has_any_top_level_await_modules } { + if has_any_top_level_await_modules { // SAFETY: `parse_graph` is a backref to `BundleV2.graph`, disjoint // from `*self` (= `BundleV2.linker`). The SoA column slices below // are physically disjoint and the underlying slabs do not @@ -934,8 +900,7 @@ impl<'a> LinkerContext<'a> { self.check_for_memory_corruption(); } - // SAFETY: scalar `u64` read of a field disjoint from `self` (= `(*bundle).linker`). - let mut chunks = compute_chunks(self, unsafe { (*bundle).unique_key })?; + let mut chunks = compute_chunks(self, unique_key)?; if self.log().has_errors() { return Err(LinkError::BuildFailed); diff --git a/src/bundler/aot/page_bundles.rs b/src/bundler/aot/page_bundles.rs index 240ff0ae7a19..81f77e8e2a7c 100644 --- a/src/bundler/aot/page_bundles.rs +++ b/src/bundler/aot/page_bundles.rs @@ -194,16 +194,7 @@ fn run<'h>( bundle.process_files_to_copy(&reachable_files)?; bundle.add_server_component_boundaries_as_extra_entry_points()?; - // SAFETY: as in `run_from_js_in_new_thread`: every borrow comes from the one raw pointer, so projecting `linker` - // does not invalidate `entry_points` or `boundaries`. - let mut chunks = unsafe { - let bundle_pointer: *mut BundleV2<'_> = &raw mut *bundle; - let entry_points = (*bundle_pointer).graph.entry_points.as_slice(); - let boundaries = &(*bundle_pointer).graph.server_component_boundaries; - (*bundle_pointer) - .linker - .link(bundle_pointer, entry_points, boundaries, &reachable_files)? - }; + let mut chunks = bundle.link(&reachable_files)?; if bundle.transpiler.log().errors > 0 { return Err(crate::Error::BuildFailed); } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index f78d46273a97..d2d7a706fa49 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -4313,6 +4313,27 @@ pub mod bv2_impl { (fetcher.on_fetch)(fetcher.ctx, &mut result) } + /// `load` has returned its `&mut Graph` before `link` starts the workers that read the graph. + pub(crate) fn link( + &mut self, + reachable: &[Index], + ) -> Result]>, crate::linker_context_mod::LinkError> { + // SAFETY: `graph` and `transpiler` are those of `self`, which holds `linker`. + unsafe { + self.linker.load( + &mut self.graph, + &*self.transpiler, + &mut self.dynamic_import_entry_points, + reachable, + ) + }?; + self.linker.link( + self.has_any_top_level_await_modules, + self.unique_key, + reachable, + ) + } + pub fn generate_from_cli( transpiler: &'a mut Transpiler<'a>, alloc: &'a bun_alloc::Arena, @@ -4375,28 +4396,7 @@ pub mod bv2_impl { this.clone_ast()?; - // SAFETY: `LinkerContext::link` takes `bundle` as a raw `*mut BundleV2` and only - // touches fields disjoint from `this.linker` (`graph`, `transpiler`, - // `dynamic_import_entry_points`, scalar reads) via `addr_of_mut!`/place - // projection, so the `&mut this.linker` receiver and `*bundle_ptr` never produce - // overlapping `&mut`. - let mut chunks = unsafe { - let bundle_ptr: *mut BundleV2 = &raw mut *this; - // `Graph::entry_points: Vec` and `link()` takes `&[Index]` — - // both are `crate::Index` (= `bun_ast::Index`), so no cast is needed. - let ep = (*bundle_ptr).graph.entry_points.as_slice(); - // `this.graph.server_component_boundaries` must stay intact for - // `StaticRouteVisitor` (generateChunksInParallel) to read via - // `parse_graph`. Borrow — do NOT `take`, which would empty the - // graph slot and drop the moved-out `MultiArrayList` heap inside - // `load()` (ASAN use-after-poison / wrong `fully_static`). - let scbs = &(*bundle_ptr).graph.server_component_boundaries; - // Project `.linker` via `bundle_ptr` (not `this.linker`) so no - // second `Box::deref_mut` retag invalidates `ep`/`scbs` (SB). - (*bundle_ptr) - .linker - .link(bundle_ptr, ep, scbs, &reachable_files)? - }; + let mut chunks = this.link(&reachable_files)?; this.dump_pool_stats("link"); // Do this at the very end, after processing all the imports/exports so that we can follow exports as needed. @@ -4539,21 +4539,7 @@ pub mod bv2_impl { this.clone_ast()?; - // SAFETY: see `generate_from_cli` — raw-ptr borrow sidestep for - // `link` takes a raw `*mut BundleV2` and only touches fields disjoint - // from `this.linker`. - let mut chunks = unsafe { - let bundle_ptr: *mut BundleV2 = &raw mut *this; - let ep = (*bundle_ptr).graph.entry_points.as_slice(); - // Value-copy (original preserved for `StaticRouteVisitor`). - // Borrow — do NOT `take` (see `generate_from_cli`). - let scbs = &(*bundle_ptr).graph.server_component_boundaries; - // Project `.linker` via `bundle_ptr` so no second `Box::deref_mut` - // retag invalidates `ep`/`scbs` (SB hygiene). - (*bundle_ptr) - .linker - .link(bundle_ptr, ep, scbs, &reachable_files)? - }; + let mut chunks = this.link(&reachable_files)?; if chunks.is_empty() { return Ok(Vec::new()); @@ -5584,20 +5570,7 @@ pub mod bv2_impl { self.add_server_component_boundaries_as_extra_entry_points()?; - // SAFETY: see `generate_from_cli` — repr(transparent) Index slice cast + - // raw-ptr borrow sidestep for `&mut self.linker` / `&mut *self`. - let mut chunks = unsafe { - let bundle_ptr: *mut BundleV2 = self; - let ep = (*bundle_ptr).graph.entry_points.as_slice(); - // Value-copy (original preserved for `StaticRouteVisitor`). - // Borrow — do NOT `take` (see `generate_from_cli`). - let scbs = &(*bundle_ptr).graph.server_component_boundaries; - // Project `.linker` via `bundle_ptr` so no `&mut *self` reborrow - // retag invalidates `ep`/`scbs` (SB hygiene). - (*bundle_ptr) - .linker - .link(bundle_ptr, ep, scbs, &reachable_files)? - }; + let mut chunks = self.link(&reachable_files)?; if self.transpiler.log().errors > 0 { return Err(crate::Error::BuildFailed); @@ -5965,23 +5938,16 @@ pub mod bv2_impl { // The linker still has to be initialized as code generation expects // much of its state to be valid memory, even if empty. - // SAFETY: `LinkerContext::load` takes `bundle` as a raw `*mut BundleV2` and only - // touches fields disjoint from `self.linker` (`graph`, `transpiler`, - // `dynamic_import_entry_points`) via `addr_of_mut!`, so the `&mut self.linker` - // receiver and `*bundle_ptr` never produce overlapping `&mut`. Both Index newtypes - // are `#[repr(transparent)]` u32 — see `generate_from_cli` for the slice cast. + // SAFETY: `graph` and `transpiler` are those of `self`, which holds `linker`. unsafe { - let bundle_ptr: *mut BundleV2 = self; - let ep = (*bundle_ptr).graph.entry_points.as_slice(); - // Value-copy (original preserved). Borrow — do NOT `take`. - let scbs = &(*bundle_ptr).graph.server_component_boundaries; - // Project `.linker` via `bundle_ptr` so no `&mut *self` reborrow - // retag invalidates `ep`/`scbs` (SB hygiene). - (*bundle_ptr) - .linker - .load(bundle_ptr, ep, scbs, js_reachable_files) - .map_err(|_| AllocError)?; + self.linker.load( + &mut self.graph, + &*self.transpiler, + &mut self.dynamic_import_entry_points, + js_reachable_files, + ) } + .map_err(|_| AllocError)?; // HMR skips tree-shaking, so size and seed the part-liveness bitsets // here: every part of every JS file is considered live. diff --git a/src/bundler/linker_context/README.md b/src/bundler/linker_context/README.md index ed83384ce7fe..5e11df5a5172 100644 --- a/src/bundler/linker_context/README.md +++ b/src/bundler/linker_context/README.md @@ -38,7 +38,7 @@ The LinkerContext operates in several main phases: **What it does**: -1. Calls `load()` to initialize the context +1. Expects `load()` to have initialized the context (`BundleV2::link` calls both) 2. Computes source map data if needed 3. **Phase 1**: `scanImportsAndExports()` - Analyzes all imports/exports across modules 4. **Phase 2**: `treeShakingAndCodeSplitting()` - Eliminates dead code and determines chunk boundaries From 052ab80278fe24fde2c9f682af2779734009cf6f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:02 +0000 Subject: [PATCH 26/27] bundler: the back-pointers of the tasks name the bundle's lifetime `ParseTask<'a>` and `ServerComponentParseTask<'a>` carry the bundle's `'a`, but their `ctx` was a `ParentRef, Mut>`. Nine places fill it. The six in `ParseTask.rs` and `bundle_v2.rs` have the bundle as `BundleV2<'a>`, so each wrote `unsafe { ParentRef::from_raw_mut(p.cast::>()) }`. In the five in `bundle_v2.rs`, `p` is `ptr::from_mut` of the bundle. The sixth is `ParseTask::init`, a safe function that took `*mut BundleV2<'a>` and read through it; its five callers all pass `self`. The three places in `aot` (`plugins.rs`, `native_plugins.rs` and `components.rs`) fill `ctx` from a `BundleV2<'static>` carrier and do not change. `SourceMapDataTask` did the same as the six with `LinkerContext<'static>`. `ctx` is now `ParentRef, Mut>`, and those six places use the safe `ParentRef::from_ref_mut(self)`. `init` takes `&mut BundleV2<'a>`. `SourceMapDataTask` and `SourceMapData` get the linker's `'a`, and their `ctx` is `ParentRef::new(&*self)`. `ParseTask<'a>` is now invariant in `'a`. Only the runtime's task relied on shortening `'static`, so `RuntimeSource` and the functions that make it are generic over `'a`. The worker side does not change. The thread pool callbacks recover the task from its intrusive field as `ParseTask<'static>`, and so on, as before, so `Result.ctx` and the code in `aot` keep `BundleV2<'static>`. The pointers have the same value and provenance as before: `ptr::from_mut(self)` and `ptr::from_ref(self)` reborrowed `self` too. In `init` the reborrow is now the parameter. It lasts for that call, on the bundle thread, inside a caller that holds `&mut BundleV2` anyway. The change is in the types only, apart from the `noalias` and `dereferenceable` attributes that `init`'s parameter gets as a `&mut`. --- src/bundler/LinkerContext.rs | 30 +++++++------------ src/bundler/ParseTask.rs | 25 +++++++--------- src/bundler/ServerComponentParseTask.rs | 2 +- src/bundler/ThreadPool.rs | 2 +- src/bundler/bundle_v2.rs | 40 ++++--------------------- 5 files changed, 29 insertions(+), 70 deletions(-) diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index de49190780d5..0711691f5a78 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -112,7 +112,7 @@ pub struct LinkerContext<'a> { /// string buffer containing prefix for each unique keys pub(crate) unique_key_prefix: Box<[u8]>, - pub(crate) source_maps: SourceMapData, + pub(crate) source_maps: SourceMapData<'a>, /// This will eventually be used for reference-counting LinkerContext /// to know whether or not we can free it safely. @@ -641,18 +641,10 @@ impl<'a> LinkerContext<'a> { .collect::>() .into_boxed_slice(); - // Note: erase `'a` → `'static` for the task backref. The tasks are - // joined before `self` is dropped (see `SourceMapData.*_wait_group`). + // The tasks are joined before `self` is dropped (see `SourceMapData.*_wait_group`). // Shared provenance: worker tasks only read the context (they never // form `&mut LinkerContext`); peer tasks hold the same pointer. - // SAFETY: `self` outlives every task (joined before drop). - let ctx: Option>> = Some(unsafe { - bun_ptr::ParentRef::from_raw( - std::ptr::from_ref::>(self) - .cast::>() - .cast_mut(), - ) - }); + let ctx = Some(bun_ptr::ParentRef::new(&*self)); let mut batch = ThreadPoolLib::Batch::default(); let mut second_batch = ThreadPoolLib::Batch::default(); debug_assert_eq!(reachable.len(), self.source_maps.line_offset_tasks.len()); @@ -1468,23 +1460,23 @@ pub enum LinkerOptionsMode { } #[derive(Default)] -pub struct SourceMapData { +pub struct SourceMapData<'a> { pub(crate) line_offset_wait_group: WaitGroup, - pub(crate) line_offset_tasks: Box<[SourceMapDataTask]>, + pub(crate) line_offset_tasks: Box<[SourceMapDataTask<'a>]>, pub(crate) quoted_contents_wait_group: WaitGroup, - pub(crate) quoted_contents_tasks: Box<[SourceMapDataTask]>, + pub(crate) quoted_contents_tasks: Box<[SourceMapDataTask<'a>]>, } -pub struct SourceMapDataTask { +pub struct SourceMapDataTask<'a> { /// `None` only in `Default` (the per-index slot is overwritten before /// scheduling). - pub(crate) ctx: Option>>, + pub(crate) ctx: Option>>, pub(crate) source_index: crate::IndexInt, pub(crate) thread_task: ThreadPoolLib::Task, } -impl Default for SourceMapDataTask { +impl Default for SourceMapDataTask<'_> { fn default() -> Self { Self { ctx: None, @@ -1497,7 +1489,7 @@ impl Default for SourceMapDataTask { } } -impl SourceMapDataTask { +impl SourceMapDataTask<'_> { // CONCURRENCY: thread-pool callback — runs on worker threads, one task per // `source_index`. Writes: `ctx.graph.files[source_index].line_offset_table` // (per-row disjoint), `ctx.pending_task_count` (atomic), @@ -1588,7 +1580,7 @@ impl SourceMapDataTask { } } -impl SourceMapData { +impl SourceMapData<'_> { /// Runs concurrently across the worker pool (one task per `source_index`). /// Takes [`ParentRef`](bun_ptr::ParentRef) (not `&mut`) /// because peer tasks on other threads hold the same pointer — diff --git a/src/bundler/ParseTask.rs b/src/bundler/ParseTask.rs index 951939729e1b..467425f4e1bb 100644 --- a/src/bundler/ParseTask.rs +++ b/src/bundler/ParseTask.rs @@ -109,7 +109,7 @@ pub struct ParseTask<'a> { pub(crate) experimental_decorators: bool, pub(crate) use_define_for_class_fields: bool, // BACKREF; `None` only before enqueue (`Default`, runtime source). - pub ctx: Option, bun_ptr::Mut>>, + pub ctx: Option, bun_ptr::Mut>>, // Borrows package_json (resolver arena); valid for the bundle pass. pub(crate) package_version: ast::StoreStr, pub(crate) package_name: ast::StoreStr, @@ -243,7 +243,7 @@ impl<'a> ParseTask<'a> { /// /// If `ctx` is `None`. #[inline] - pub(crate) unsafe fn ctx<'r>(&self) -> &'r BundleV2<'static> { + pub(crate) unsafe fn ctx<'r>(&self) -> &'r BundleV2<'a> { // SAFETY: caller upholds: bundle outlives `'r`. unsafe { bun_ptr::detach_lifetime_ref(self.ctx.expect("ParseTask.ctx unset").get()) } } @@ -252,10 +252,10 @@ impl<'a> ParseTask<'a> { resolve_result: &_resolver::Result<'_>, path: Fs::Path<'a>, source_index: Index, - // Take `*mut` so the stored BACKREF retains + // Take `&mut` so the stored BACKREF retains // write provenance for `on_complete` (a `&BundleV2` param would shrink // provenance to read-only, making the later `&mut *ctx` UB). - ctx: *mut BundleV2<'a>, + ctx: &mut BundleV2<'a>, ) -> ParseTask<'a> { let (package_name, package_version) = match resolve_result.package_json { Some(pj) => ( @@ -264,10 +264,7 @@ impl<'a> ParseTask<'a> { ), None => (ast::StoreStr::EMPTY, ast::StoreStr::EMPTY), }; - // SAFETY: lifetime erased — `ctx` outlives the ParseTask (BACKREF); - // write provenance from the `*mut BundleV2` parameter; caller passes a - // live `&mut BundleV2` coerced to `*mut`. - let ctx_ref = unsafe { bun_ptr::ParentRef::from_raw_mut(ctx.cast::>()) }; + let ctx_ref = bun_ptr::ParentRef::from_ref_mut(ctx); let known_target = ctx_ref.get().transpiler().options.target; ParseTask { ctx: Some(ctx_ref), @@ -310,7 +307,7 @@ impl<'a> ParseTask<'a> { /// Re-export of `parse_worker::get_runtime_source` as an associated fn so /// callers can spell it `ParseTask::get_runtime_source`. #[inline] - pub(crate) fn get_runtime_source(target: options::Target) -> RuntimeSource { + pub(crate) fn get_runtime_source(target: options::Target) -> RuntimeSource<'a> { parse_worker::get_runtime_source(target) } } @@ -404,8 +401,8 @@ unsafe fn task_callback(task: *mut ThreadPoolLib::Task) { // RuntimeSource // ─────────────────────────────────────────────────────────────────────────── -pub(crate) struct RuntimeSource { - pub(crate) parse_task: ParseTask<'static>, +pub(crate) struct RuntimeSource<'a> { + pub(crate) parse_task: ParseTask<'a>, pub(crate) source: Source<'static>, } @@ -564,7 +561,7 @@ export var __callDispose = (stack, error, hasError) => { pub mod parse_worker { use super::*; - fn get_runtime_source_comptime(target: options::Target) -> RuntimeSource { + fn get_runtime_source_comptime<'a>(target: options::Target) -> RuntimeSource<'a> { // The runtime module is the shared `runtime.js` body plus a per-target // `__require`/`__using` tail. Concatenating at compile time would embed // four copies of the 13 KB body, so each variant is assembled once on @@ -655,7 +652,7 @@ pub mod parse_worker { RuntimeSource { parse_task, source } } - pub(crate) fn get_runtime_source(target: options::Target) -> RuntimeSource { + pub(crate) fn get_runtime_source<'a>(target: options::Target) -> RuntimeSource<'a> { get_runtime_source_comptime(target) } @@ -2940,7 +2937,7 @@ pub mod parse_worker { let r = unsafe { &mut *result }; let ctx = r.ctx; // SAFETY: `ctx` is a ParentRef stored with write provenance - // (`from_raw_mut` in `ParseTask::init`); the BundleV2 outlives the bundle + // (`from_ref_mut` in `ParseTask::init`); the BundleV2 outlives the bundle // pass and no other `&mut BundleV2` is live on this (main) thread when the // event-loop callback fires. `r` and `*ctx` are disjoint allocations. BundleV2::on_parse_task_complete(r, unsafe { ctx.assume_mut() }); diff --git a/src/bundler/ServerComponentParseTask.rs b/src/bundler/ServerComponentParseTask.rs index 7b40bec65a08..f54c4b6d2f31 100644 --- a/src/bundler/ServerComponentParseTask.rs +++ b/src/bundler/ServerComponentParseTask.rs @@ -31,7 +31,7 @@ pub(crate) struct ServerComponentParseTask<'a> { // BACKREF (LIFETIMES.tsv) — written through in `on_complete`. // `ParentRef` (write-provenance via `NonNull::from(&mut self)` at construction) // so deref sites are safe; `None` only for the FRU `Default` placeholder. - pub ctx: Option, bun_ptr::Mut>>, + pub ctx: Option, bun_ptr::Mut>>, pub source: Source<'a>, } diff --git a/src/bundler/ThreadPool.rs b/src/bundler/ThreadPool.rs index 6bf48bcceccd..e17065cb6201 100644 --- a/src/bundler/ThreadPool.rs +++ b/src/bundler/ThreadPool.rs @@ -67,7 +67,7 @@ pub struct ThreadPool { /// freed slot), so each pool draws a fresh `u64` from [`POOL_GENERATION`]. pub(crate) generation: u64, // BACKREF (LIFETIMES.tsv row 170: ThreadPool.v2). `BundleV2` is generic - // over `'a`; erase to `'static` behind the raw pointer like ParseTask.ctx. + // over `'a`; erase to `'static` behind the raw pointer. pub(crate) v2: *const BundleV2<'static>, } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index d2d7a706fa49..225fe3568023 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -3716,12 +3716,7 @@ pub mod bv2_impl { .put(&b"bun:wrap"[..], Index::RUNTIME.get()) .expect("oom"); let runtime_parse_task: &mut ParseTask = self.arena().alloc(rt.parse_task); - // SAFETY: BACKREF — `self` outlives the task; lifetime erased per ParseTask::ctx convention. - runtime_parse_task.ctx = Some(unsafe { - bun_ptr::ParentRef::from_raw_mut( - std::ptr::from_mut(self).cast::>(), - ) - }); + runtime_parse_task.ctx = Some(bun_ptr::ParentRef::from_ref_mut(self)); runtime_parse_task.loader = Some(Loader::Js); self.increment_scan_counter(); self.graph.pool().schedule(runtime_parse_task); @@ -4101,12 +4096,7 @@ pub mod bv2_impl { self.transpiler_for_target(known_target).options.jsx.clone() }; let task: &mut ParseTask = self.arena().alloc(ParseTask { - // SAFETY: BACKREF — `self` outlives the task; lifetime erased per ParseTask::ctx convention. - ctx: Some(unsafe { - bun_ptr::ParentRef::from_raw_mut( - std::ptr::from_mut(self).cast::>(), - ) - }), + ctx: Some(bun_ptr::ParentRef::from_ref_mut(self)), path: task_path, contents_or_fd: parse_task::ContentsOrFd::Contents(contents), side_effects: bun_ast::SideEffects::HasSideEffects, @@ -4172,14 +4162,7 @@ pub mod bv2_impl { // result posts back to the bundle thread. let task = bun_core::heap::into_raw(Box::new(ServerComponentParseTask { data, - // SAFETY: `from_mut(self)` is the live bundle (write provenance for - // `on_complete`'s `assume_mut`) and outlives the task; `'a` erased to - // `'static` for the BACKREF. - ctx: Some(unsafe { - bun_ptr::ParentRef::from_raw_mut( - core::ptr::from_mut(&mut *self).cast::>(), - ) - }), + ctx: Some(bun_ptr::ParentRef::from_ref_mut(self)), source: task_source, // `..Default::default()` supplies `task: ThreadPoolTask { callback: task_callback_wrap }`. ..Default::default() @@ -5271,14 +5254,7 @@ pub mod bv2_impl { }) .expect("unreachable"); let task_val = ParseTask { - // SAFETY: `from_mut(this)` is the live bundle (write provenance); - // outlives the task. - ctx: Some(unsafe { - bun_ptr::ParentRef::from_raw_mut( - std::ptr::from_mut::(this) - .cast::>(), - ) - }), + ctx: Some(bun_ptr::ParentRef::from_ref_mut(this)), path, // unknown at this point: contents_or_fd: parse_task::ContentsOrFd::Fd { @@ -7180,13 +7156,7 @@ pub mod bv2_impl { // path map are both needed across the loop body. We (a) capture a raw self ptr for // ParseTask.ctx, (b) hoist dev_server check, and (c) scope the map // borrow to the `build_graphs` field so later `self.graph.*` writes don't overlap. - // SAFETY: write provenance from `ptr::from_mut`; outlives every ParseTask. - let self_ptr: Option, bun_ptr::Mut>> = - Some(unsafe { - bun_ptr::ParentRef::from_raw_mut( - std::ptr::from_mut::(self).cast::>(), - ) - }); + let self_ptr = Some(bun_ptr::ParentRef::from_ref_mut(self)); let dev_server_is_none = self.dev_server.is_none(); for (key, value) in resolve_queue.iter() { let value: *mut ParseTask = *value; From 5c2f91bce1b60007383114a4c55833516fd95a01 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 14:51:02 +0000 Subject: [PATCH 27/27] threading: `ThreadPool::each_ptr` hands each task a `&mut V` `each_ptr` takes `values: &mut [V]`, makes one task per index, and blocks until all of them are done. It passed each task a `*mut V`. All six callbacks, which are all in the bundler, dereferenced it with the reason "`each_ptr` hands each task a distinct pointer". Five began with `unsafe { &mut *ptr }`. The sixth, `run_nested_rename_task`, read two fields at its start and wrote one at its end. That the pointers are distinct and live is something only the pool can know. The callback is now `Fn(&Ctx, &mut V, usize)`. `each_impl` has the exclusive borrow of the slice, does not use it while the tasks run, and gives each index to one task, so the block that computed `&raw mut (*values)[i]` makes `&mut (*values)[i]` instead. The bound is higher-ranked, so a callback cannot keep the reference, and `V: Send` was required already. `EachCall::call` is a safe function now. Its `ByValue` impl, which serves `each` and `each_while`, copies out of the reference. Apart from step 5 of the linker, which passes its own snapshot of the reachable files, every caller of those two passes a fresh `vec![(); n]`. No `&mut` lives longer than before in the five callbacks that made one on entry. `generate_chunk` reads the other chunks through `ctx.chunks` while each task holds the `&mut` to its own, as it did before. The address and the bounds check are the same. What changes are the attributes of the callbacks' parameter: as a `&mut V` it is `noalias` and `dereferenceable`. Four of the six callbacks pass it on, and their callees had these attributes on the same pointer already: `post_process_js_chunk`, `post_process_css_chunk` and `post_process_html_chunk` under `generate_chunk`, `generate_js_renamer_` under `generate_js_renamer`, and `find_imported_parts_in_js_order` and `EntryWalk::run` under the two closures. For `finish_js_renamer` and `run_nested_rename_task` they are new; neither uses another way to reach its value. The doc of `GenerateChunkCtx` said that `each_ptr` asks for `Ctx: Sync + Copy`. It asks for `Sync`. --- src/bundler/LinkerContext.rs | 32 ++++++------------- .../findAllImportedPartsInJSOrder.rs | 8 ++--- .../linker_context/renameSymbolsInChunk.rs | 9 ++---- src/threading/ThreadPool.rs | 28 ++++++++-------- 4 files changed, 28 insertions(+), 49 deletions(-) diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index 0711691f5a78..eb0aa63db10c 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -1043,17 +1043,10 @@ impl<'a> LinkerContext<'a> { // CONCURRENCY: `each_ptr` callback — runs on worker threads, one task per // `chunk_index`. Writes: `chunk.intermediate_output`, `chunk.isolated_hash`, - // `chunk.output_source_map` (per-chunk, disjoint by `*mut Chunk`). Reads - // `ctx.c`/`ctx.chunks` shared. Never forms `&mut LinkerContext` — the - // `post_process_*` callees take `GenerateChunkCtx` by value and deref - // `ctx.c` to `&LinkerContext` for read-only graph access plus per-chunk - // raw-ptr writes (see `postProcessJSChunk.rs`). - pub(crate) fn generate_chunk(ctx: &GenerateChunkCtx, chunk: *mut Chunk, chunk_index: usize) { - // SAFETY: `each_ptr` hands us a unique `*mut Chunk` per task; deref for - // the duration of this body. ctx.c points into BundleV2.linker; - // container_of pattern. `Worker::get` only reads `bundle.graph.pool` - // (shared), so a `&` is sufficient and avoids aliasing. - let chunk: &mut Chunk = unsafe { &mut *chunk }; + // `chunk.output_source_map` (per-chunk, disjoint by `&mut Chunk`). Reads + // `ctx.chunks` shared. The `post_process_*` callees take `GenerateChunkCtx` + // by value and get `&mut LinkerContext` from `ctx.c()`. + pub(crate) fn generate_chunk(ctx: &GenerateChunkCtx, chunk: &mut Chunk, chunk_index: usize) { let worker = crate::thread_pool::Worker::get(ctx.bundle()); let mut worker = scopeguard::guard(worker, |w| w.unget()); let worker: &mut crate::thread_pool::Worker = &mut **worker; @@ -1073,18 +1066,15 @@ impl<'a> LinkerContext<'a> { // CONCURRENCY: `each_ptr` callback — runs on worker threads, one task per // `chunk_index`. Writes: `chunk.renamer` only (per-chunk, disjoint by - // `*mut Chunk`). Reads `ctx.c.graph.{ast,meta,symbols}` SoA columns and + // `&mut Chunk`). Reads `ctx.c.graph.{ast,meta,symbols}` SoA columns and // `ctx.c.options` shared. `rename_symbols_in_chunk` takes `*mut // LinkerContext` raw and never materializes `&mut LinkerContext` while // peer renamer tasks are live (see its CONCURRENCY note). pub(crate) fn generate_js_renamer( ctx: &GenerateChunkCtx, - chunk: *mut Chunk, + chunk: &mut Chunk, chunk_index: usize, ) { - // SAFETY: `each_ptr` hands us a unique `*mut Chunk` per task; deref for - // the body. container_of pattern — see `generate_chunk` above. - let chunk: &mut Chunk = unsafe { &mut *chunk }; let worker = crate::thread_pool::Worker::get(ctx.bundle()); let mut worker = scopeguard::guard(worker, |w| w.unget()); if let crate::chunk::Content::Javascript(_) = chunk.content { @@ -1096,11 +1086,9 @@ impl<'a> LinkerContext<'a> { /// slot `assign_cross_chunk_names` did not pin. Writes `chunk.renamer` only. pub(crate) fn finish_js_renamer( _ctx: &GenerateChunkCtx, - chunk: *mut Chunk, + chunk: &mut Chunk, _chunk_index: usize, ) { - // SAFETY: `each_ptr` hands us a unique `*mut Chunk` per task. - let chunk: &mut Chunk = unsafe { &mut *chunk }; if let crate::bun_renamer::ChunkRenamer::Minify(r) = &mut chunk.renamer { // Only allocation can fail here. bun_core::handle_oom(r.finish()); @@ -1749,8 +1737,8 @@ pub struct ChunkMeta { pub(crate) type ChunkMetaMap = ArrayHashMap; /// Note: raw-pointer fields (was `&'a mut`) because `each_ptr` requires -/// `Ctx: Sync + Copy` and the same context is observed from every worker -/// thread. Each task only writes to its own `*mut Chunk` slot; reads of +/// `Ctx: Sync` and the same context is observed from every worker +/// thread. Each task only writes to its own `Chunk` slot; reads of /// `c`/`chunks` are disjoint or read-only. #[derive(Clone, Copy)] pub struct GenerateChunkCtx<'a> { @@ -1760,7 +1748,7 @@ pub struct GenerateChunkCtx<'a> { /// `GenerateChunkCtx` (joined via the batch's `group.wait()`), so [`bun_ptr::BackRef`]'s /// owner-outlives-holder invariant holds and per-task reads go through /// safe `Deref`. Read-only: each task writes only through its own - /// `*mut Chunk`. + /// `&mut Chunk` or `*mut Chunk`. pub(crate) chunks: bun_ptr::BackRef<[Chunk<'a>]>, /// Backref to this task's `Chunk` (an element of `chunks`). Constructed /// via [`bun_ptr::BackRef::new_mut`] so the stored `NonNull` carries write diff --git a/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs b/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs index 02ae5bc51d15..89c826796ea3 100644 --- a/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs +++ b/src/bundler/linker_context/findAllImportedPartsInJSOrder.rs @@ -58,9 +58,7 @@ pub(crate) fn find_all_imported_parts_in_js_order<'a>( let chunks_len = chunks.len(); this.worker_pool().each_ptr( ctx, - |ctx: &Ctx, chunk: *mut Chunk, index: usize| { - // SAFETY: `each_ptr` hands each task a distinct `*mut Chunk`. - let chunk = unsafe { &mut *chunk }; + |ctx: &Ctx, chunk: &mut Chunk, index: usize| { if !matches!(chunk.content, chunk::Content::Javascript(_)) { return; // CSS: `find_imported_css_files_in_js_order`; HTML: nothing } @@ -96,9 +94,7 @@ fn run_walks<'a>( }; this.worker_pool().each_ptr( walk_ctx, - |ctx: &WalkCtx, walk: *mut EntryWalk, _: usize| { - // SAFETY: `each_ptr` hands each task a distinct `*mut EntryWalk`. - let walk = unsafe { &mut *walk }; + |ctx: &WalkCtx, walk: &mut EntryWalk, _: usize| { walk.run(ctx.c, ctx.plan, ctx.chunks); }, walks, diff --git a/src/bundler/linker_context/renameSymbolsInChunk.rs b/src/bundler/linker_context/renameSymbolsInChunk.rs index 7841dcacca2d..4949addf0cf2 100644 --- a/src/bundler/linker_context/renameSymbolsInChunk.rs +++ b/src/bundler/linker_context/renameSymbolsInChunk.rs @@ -534,12 +534,10 @@ pub(crate) fn nested_rename_tasks(chunks: &[Chunk]) -> Vec { // `task.names`. pub(crate) fn run_nested_rename_task( ctx: &crate::linker_context_mod::GenerateChunkCtx, - task: *mut NestedRenameTask, + task: &mut NestedRenameTask, _task_index: usize, ) { - // SAFETY: `each_ptr` hands us a unique `*mut NestedRenameTask`; nothing - // below re-enters it. - let (chunk_index, scope_range) = unsafe { ((*task).chunk_index, (*task).scopes.clone()) }; + let (chunk_index, scope_range) = (task.chunk_index, task.scopes.clone()); let c: &LinkerContext<'_> = &ctx.c; let chunk = &ctx.chunks[chunk_index as usize]; let ChunkRenamer::Number(root) = &chunk.renamer else { @@ -576,6 +574,5 @@ pub(crate) fn run_nested_rename_task( } let names = nested.into_names(); drop(worker); - // SAFETY: as above. - unsafe { (*task).names = Some(names) }; + task.names = Some(names); } diff --git a/src/threading/ThreadPool.rs b/src/threading/ThreadPool.rs index ac8899f69b56..9ed410aa69aa 100644 --- a/src/threading/ThreadPool.rs +++ b/src/threading/ThreadPool.rs @@ -541,10 +541,9 @@ impl Batch { } } -/// Dispatch trait for `each_impl`: the by-value vs by-pointer branch as two impls. +/// Dispatch trait for `each_impl`: the by-value vs by-`&mut` branch as two impls. trait EachCall: core::marker::Sync { - /// SAFETY: `value` must point to a live `V` exclusively owned by this call. - unsafe fn call(&self, ctx: &Ctx, value: *mut V, i: usize); + fn call(&self, ctx: &Ctx, value: &mut V, i: usize); } struct ByValue(F); @@ -553,19 +552,18 @@ where F: Fn(&Ctx, V, usize) + core::marker::Sync, { #[inline] - unsafe fn call(&self, ctx: &Ctx, value: *mut V, i: usize) { - // SAFETY: caller guarantees `value` is a live `V`; `V: Copy` so deref is a copy. - (self.0)(ctx, unsafe { *value }, i); + fn call(&self, ctx: &Ctx, value: &mut V, i: usize) { + (self.0)(ctx, *value, i); } } struct ByPtr(F); impl EachCall for ByPtr where - F: Fn(&Ctx, *mut V, usize) + core::marker::Sync, + F: Fn(&Ctx, &mut V, usize) + core::marker::Sync, { #[inline] - unsafe fn call(&self, ctx: &Ctx, value: *mut V, i: usize) { + fn call(&self, ctx: &Ctx, value: &mut V, i: usize) { (self.0)(ctx, value, i); } } @@ -577,7 +575,7 @@ impl ThreadPool { /// This function does not shut down or deinit the thread pool. /// /// `V: Send` is required because each `values[i]` is handed (by copy or by - /// `*mut V`) to an arbitrary worker thread; the raw-pointer round-trip + /// `&mut V`) to an arbitrary worker thread; the raw-pointer round-trip /// through the intrusive `Task` callback would otherwise smuggle `!Send` /// data across threads with no compiler check. pub fn each(&self, ctx: Ctx, run_fn: F, values: &mut [V]) @@ -623,13 +621,13 @@ impl ThreadPool { usize::from(sync.idle()) + not_started } - /// Like `each`, but calls `run_fn` with a pointer to the value. + /// Like `each`, but calls `run_fn` with a `&mut` to the value. /// - /// `V: Send` — see [`each`](Self::each); the `*mut V` is dereferenced on a + /// `V: Send` — see [`each`](Self::each); the `&mut V` is used on a /// worker thread, which is a cross-thread move of the pointee. pub fn each_ptr(&self, ctx: Ctx, run_fn: F, values: &mut [V]) where - F: Fn(&Ctx, *mut V, usize) + core::marker::Sync, + F: Fn(&Ctx, &mut V, usize) + core::marker::Sync, Ctx: core::marker::Sync, V: core::marker::Sync + core::marker::Send, { @@ -677,9 +675,9 @@ impl ThreadPool { let wctx = runner_task.ctx.get(); // SAFETY: `values` slice outlives all RunnerTasks (`group.wait()` blocks until // every task finishes); each task owns a distinct index `i`. - let value: *mut V = unsafe { &raw mut (*wctx.values)[i] }; - // SAFETY: `value` is live and exclusively owned by this task per the index. - unsafe { wctx.run_fn.call(&wctx.ctx, value, i) }; + // `each_impl` holds the exclusive borrow of `values` and does not use it between `schedule` and `wait()`. + let value: &mut V = unsafe { &mut (*wctx.values)[i] }; + wctx.run_fn.call(&wctx.ctx, value, i); } let wait_context = WaitContext {