diff --git a/src/runtime/api/bun/js_bun_spawn_bindings.rs b/src/runtime/api/bun/js_bun_spawn_bindings.rs index bf9cb66a6043..f5f831ffbcbe 100644 --- a/src/runtime/api/bun/js_bun_spawn_bindings.rs +++ b/src/runtime/api/bun/js_bun_spawn_bindings.rs @@ -15,6 +15,7 @@ use bun_jsc::{ JsResult, SystemError, }; use bun_jsc::{JsCell, SysErrorJsc as _}; +use bun_ptr::OwnedRef; #[cfg(unix)] use bun_sys::Fd; use bun_sys::UV_E; @@ -74,11 +75,6 @@ impl TerminalCreateResult { } } -#[inline] -fn subprocess_ipc_owner(ptr: *mut SubprocessT<'_>) -> Option { - core::ptr::NonNull::new(ptr.cast::>()).map(IPC::SendQueueOwner::Subprocess) -} - bun_output::declare_scope!(Subprocess, hidden); // Stdio is platform-dependent: process.rs defines `PosixStdio` / `WindowsStdio` @@ -1294,7 +1290,7 @@ fn spawn_maybe_sync( // Note: build // the struct once with its final field values, then fill in the // address-dependent fields (maxbufs, ipc_data on Windows) afterward. - let subprocess_ptr = bun_core::heap::into_raw(Box::new(SubprocessT { + let spawn_ref = OwnedRef::new(SubprocessT { global_this: bun_ptr::BackRef::new(global_this), // SAFETY: `to_process` returns a non-null `Box::into_raw` pointer; the // intrusive ref is released in `Subprocess::finalize`. @@ -1313,9 +1309,11 @@ fn spawn_maybe_sync( stdin: JsCell::new(Writable::Ignore), stdout: JsCell::new(Readable::Ignore), stderr: JsCell::new(Readable::Ignore), - // 1=JS (released in Subprocess::finalize), 2=Process exit handler - // (released in Subprocess::on_process_exit; stranded if child outlives VM teardown). - ref_count: bun_ptr::RefCount::init_exact_refs(2), + // The one ref is `spawn_ref`, which this function holds until it + // returns (spawnSync: until it tears the Subprocess down). The exit + // handler and the JS wrapper take their own refs where they are + // installed below. + ref_count: bun_ptr::RefCount::init(), stdio_pipes: JsCell::new(core::mem::take(&mut spawned_extra_pipes)), ipc_data: Cell::new(None), flags: Cell::new(if IS_SYNC { @@ -1342,15 +1340,13 @@ fn spawn_maybe_sync( crate::timer::EventLoopTimerTag::SubprocessTimeout, )), exited_due_to_maxbuf: Cell::new(None), - })); - // SAFETY: subprocess_ptr is a freshly-boxed Subprocess; we hold the only reference. - let subprocess = unsafe { &mut *subprocess_ptr }; + }); + // Shared, because the callbacks wired below re-enter the Subprocess before + // this function returns (R-2); `spawn_ref` keeps it alive for every use. + let subprocess: &SubprocessT<'static> = &spawn_ref; #[cfg(windows)] - SubprocessT::record_stdio_pipe_ownership(subprocess_ptr); - // Erase the borrow lifetime to 'static for the intrusive back-pointer - // (PipeReader stores it as raw NonNull). subprocess_ptr is non-null (just boxed). - let subprocess_nn: NonNull> = - NonNull::new(subprocess_ptr.cast()).expect("Box::into_raw returned null"); + SubprocessT::record_stdio_pipe_ownership(spawn_ref.as_ptr()); + let subprocess_nn = spawn_ref.as_non_null(); // Address-dependent fields, filled now that `subprocess` has a stable address. { @@ -1373,7 +1369,7 @@ fn spawn_maybe_sync( .ipc_data .set(core::ptr::NonNull::new(IPC::SendQueue::new( ipc_mode, - subprocess_ipc_owner(subprocess_ptr), + Some(IPC::SendQueueOwner::Subprocess(subprocess_nn)), IPC::SocketUnion::Uninitialized, ))); } @@ -1391,13 +1387,13 @@ fn spawn_maybe_sync( ) { Ok(v) => subprocess.stdin.set(v), Err(err) => { - // ref_count = 2 from the aggregate above, but neither the JS - // wrapper nor the process exit handler are wired up yet, so - // release both. stdout/stderr are still `.ignore` — close the raw - // spawned pipe handles directly since `Readable.init()` will not - // run. `finalizeStreams()` here only closes `stdio_pipes` and the - // pidfd; stdin/stdout/stderr are `.ignore` so their `closeIO` is a - // no-op. + // Neither the JS wrapper nor the process exit handler is wired up + // yet, so `spawn_ref` is the only ref and dropping it at the end + // of this arm frees the Subprocess. stdout/stderr are still + // `.ignore` — close the raw spawned pipe handles directly since + // `Readable.init()` will not run. `finalizeStreams()` here only + // closes `stdio_pipes` and the pidfd; stdin/stdout/stderr are + // `.ignore` so their `closeIO` is a no-op. #[cfg(unix)] { if let Some(fd) = spawned_stdout { @@ -1449,8 +1445,7 @@ fn spawn_maybe_sync( let mut mb = subprocess.stderr_maxbuf.get(); MaxBuf::remove_from_subprocess(&mut mb); subprocess.stderr_maxbuf.set(mb); - subprocess.deref(); - subprocess.deref(); + drop(spawn_ref); // Note: `Writable::init` returns // `crate::Error`. Map non-thrown to OOM. if global_this.has_exception() { @@ -1501,10 +1496,16 @@ fn spawn_maybe_sync( } // existing_terminal: don't close slave_fd - user manages lifecycle and can reuse - // SAFETY: `subprocess_ptr` is the live JSC-allocated Subprocess that owns - // `process` and outlives it (handler ctx invariant). + // The handler holds its own ref, released at the end of + // `Subprocess::on_process_exit`, or by `finalize` if it never runs. + // SAFETY: `Subprocess` is the type the `Subprocess` variant dispatches to, + // and the ref handed over here keeps it live until the handler has run or + // been detached. subprocess.process_mut().set_exit_handler(unsafe { - bun_spawn::ProcessExit::new(bun_spawn::ProcessExitKind::Subprocess, subprocess_ptr) + bun_spawn::ProcessExit::new( + bun_spawn::ProcessExitKind::Subprocess, + spawn_ref.clone().into_raw(), + ) }); promise_for_stream.ensure_still_alive(); @@ -1557,7 +1558,7 @@ fn spawn_maybe_sync( .ipc_data .set(core::ptr::NonNull::new(IPC::SendQueue::new( mode, - subprocess_ipc_owner(subprocess_ptr), + Some(IPC::SendQueueOwner::Subprocess(subprocess_nn)), IPC::SocketUnion::Uninitialized, ))); posix_ipc_info = Some(IPC::Socket::from(socket)); @@ -1614,33 +1615,29 @@ fn spawn_maybe_sync( .as_err() { let err_js = err.to_js(global_this); - subprocess.deref(); + // `spawn_ref` drops on the way out; the exit handler's ref + // keeps the Subprocess alive until the child has been reaped. return Err(global_this.throw_value(err_js)); } } ipc_data.write_version_packet(global_this); } - if matches!(subprocess.stdin.get(), Writable::Pipe(_)) && promise_for_stream == JSValue::ZERO { - // Store the whole-allocation `*mut Subprocess` so Writable::on_close can raw-project stdin. - // SAFETY: `subprocess_ptr` is the stable boxed Subprocess; stdin was just confirmed `Pipe`. - unsafe { - if let Writable::Pipe(pipe) = (*subprocess_ptr).stdin.get() { - (*pipe.as_ptr()) - .source - .set(WebCore::streams::SourceHandle::Subprocess( - bun_ptr::BackRef::from_raw(subprocess_ptr.cast::>()), - )); - } - } + if let Writable::Pipe(pipe) = subprocess.stdin.get() + && promise_for_stream == JSValue::ZERO + { + Writable::pipe_sink(*pipe) + .source + .set(WebCore::streams::SourceHandle::Subprocess( + bun_ptr::BackRef::new(subprocess), + )); } let out = if !IS_SYNC { - // `subprocess_ptr` came from `heap::alloc` above and has not yet been - // wrapped; ownership transfers to the C++ JS cell (released via - // `SubprocessClass__finalize`). Use the raw-ptr entrypoint instead of - // the by-value `JsClass::to_js` (which would re-box). - SubprocessT::to_js_from_ptr(subprocess_ptr, global_this) + // The wrapper gets a ref of its own (released by + // `SubprocessClass__finalize`); `spawn_ref` stays with this function, + // which keeps using the Subprocess below. + SubprocessT::to_js_from_ref(spawn_ref.clone(), global_this) } else { JSValue::ZERO }; @@ -1650,7 +1647,11 @@ fn spawn_maybe_sync( subprocess.update_has_pending_activity(); } - let mut send_exit_notification = false; + // Set when `watch()` fails below (the child may already be gone); the + // guard after it then delivers the exit on the way out of this function, + // once the readers have been started. It carries its own ref rather than + // borrowing `spawn_ref`, which the spawnSync tail consumes. + let mut deliver_exit: Option>> = None; if !IS_SYNC { // This must go before other things happen so that the exit handler is @@ -1711,20 +1712,15 @@ fn spawn_maybe_sync( match subprocess.process_mut().watch() { sys::Result::Ok(()) => {} sys::Result::Err(_) => { - send_exit_notification = true; + deliver_exit = Some(spawn_ref.clone()); lazy = false; } } } - // Note: reshaped for borrowck — copy `subprocess_ptr` so the - // non-`move` `defer!` closure captures a disjoint place from the - // `(*subprocess_ptr).abort_signal = …` writes that follow. - let subprocess_ptr_exit = subprocess_ptr; scopeguard::defer! { - if send_exit_notification { - // SAFETY: subprocess_ptr is live for the lifetime of this defer. - let proc = unsafe { &*subprocess_ptr_exit }.process_mut(); + if let Some(subprocess) = &deliver_exit { + let proc = subprocess.process_mut(); if proc.has_exited() { // process has already exited, we called wait4(), but we did not call onProcessExit() // SAFETY: all-zero is a valid Rusage (POD). @@ -1742,9 +1738,6 @@ fn spawn_maybe_sync( // the writer's start() throws below, both PipeReaders have taken their // start() ref and on_process_exit's later drain is refcount-balanced. if let Readable::Pipe(pipe) = subprocess.stdout.get() { - // Note: pass `subprocess_nn` (the `NonNull>` - // captured above) instead of the live `&mut subprocess`, which would - // alias with the `&mut subprocess.stdout` borrow held by `pipe`. Readable::pipe_reader_mut(pipe).start(subprocess_nn, event_loop_nn, !IS_SYNC && lazy); if (IS_SYNC || !lazy) && matches!(subprocess.stdout.get(), Readable::Pipe(_)) { if let Readable::Pipe(pipe) = subprocess.stdout.get() { @@ -1754,7 +1747,6 @@ fn spawn_maybe_sync( } if let Readable::Pipe(pipe) = subprocess.stderr.get() { - // Note: see stdout arm above — avoid aliased &mut. Readable::pipe_reader_mut(pipe).start(subprocess_nn, event_loop_nn, !IS_SYNC && lazy); if (IS_SYNC || !lazy) && matches!(subprocess.stderr.get(), Readable::Pipe(_)) { @@ -1802,14 +1794,12 @@ fn spawn_maybe_sync( if let Some(signal) = abort_signal.take() { // SAFETY: `signal` is a live *mut AbortSignal carrying the +1 ref taken // above; ownership of that ref transfers to `subprocess.abort_signal`. - // `add_listener` may synchronously fire `on_abort_signal` (already - // aborted), which re-enters via `subprocess_ptr` — write through the - // raw pointer so no `&mut Subprocess` is held across the call. unsafe { (*signal).pending_activity_ref(); - let _ = (*signal).add_listener(subprocess_ptr.cast(), Subprocess::on_abort_signal); - (*subprocess_ptr).abort_signal.set(NonNull::new(signal)); + let _ = + (*signal).add_listener(subprocess.as_ctx_ptr().cast(), Subprocess::on_abort_signal); } + subprocess.abort_signal.set(NonNull::new(signal)); } if !IS_SYNC { @@ -1849,10 +1839,10 @@ fn spawn_maybe_sync( // SAFETY: see the matching block above. unsafe { (*signal).pending_activity_ref(); - let _ = - (*signal).add_listener(subprocess_ptr.cast(), Subprocess::on_abort_signal); - (*subprocess_ptr).abort_signal.set(NonNull::new(signal)); + let _ = (*signal) + .add_listener(subprocess.as_ctx_ptr().cast(), Subprocess::on_abort_signal); } + subprocess.abort_signal.set(NonNull::new(signal)); } } sys::Result::Err(_) => { @@ -2016,6 +2006,12 @@ fn spawn_maybe_sync( } } } + + // There is no JS wrapper whose finalizer would tear the Subprocess down + // later, so from here on every exit does it, the throwing ones included. + let spawn_ref = scopeguard::guard(spawn_ref, SubprocessT::finalize_owned); + let subprocess: &SubprocessT<'static> = &spawn_ref; + if global_this.has_exception() { // e.g. a termination exception. return Ok(JSValue::ZERO); @@ -2039,10 +2035,8 @@ fn spawn_maybe_sync( let exited_due_to_timeout = did_timeout; let exited_due_to_max_buffer = subprocess.exited_due_to_maxbuf.get(); let result_pid = JSValue::js_number_from_int32(subprocess.pid()); - // SAFETY: `subprocess_ptr` was produced by `heap::into_raw(Box::new(...))` - // above (spawnSync path: never handed to a JS wrapper); reclaim ownership. - // `subprocess` (`&mut *subprocess_ptr`) is not used after this line. - SubprocessT::finalize(unsafe { Box::from_raw(subprocess_ptr) }); + // Tears the Subprocess down; nothing below reads it. + drop(spawn_ref); let sync_value = JSValue::create_empty_object(global_this, 0); sync_value.put(global_this, b"exitCode", exit_code); diff --git a/src/runtime/api/bun/subprocess.rs b/src/runtime/api/bun/subprocess.rs index e8797b6454b1..9f7912cfa1c2 100644 --- a/src/runtime/api/bun/subprocess.rs +++ b/src/runtime/api/bun/subprocess.rs @@ -5,7 +5,7 @@ use core::cell::Cell; use core::ffi::c_void; use core::ptr::NonNull; -use bun_ptr::RefCount; +use bun_ptr::{OwnedRef, RefCount}; use bun_jsc::{ self as jsc, CallFrame, JSGlobalObject, JSPromise, JSValue, JsCell, JsRef, JsResult, @@ -113,8 +113,10 @@ pub use bun_spawn::process::StdioKind; // codegen shim hands to whichever method JS calls next. `UnsafeCell`-backed // fields suppress `noalias` on the outer `&Subprocess`, making the miscompile // structurally impossible. -// Intrusive ref-count: `RefPtr` provides ref/deref and frees the -// Box when ref_count → 0; `deinit` runs when the last ref drops. +// Intrusive ref-count, freed with the Box when it reaches zero. Holders: the +// JS wrapper (`to_js_from_ref` hands it its ref, `finalize` releases it; for +// spawnSync, `spawn_maybe_sync` plays that part itself), the process exit +// handler, and the stdio pipes while they are open. #[derive(bun_ptr::RefCounted)] pub struct Subprocess<'a> { pub(crate) ref_count: RefCount>, @@ -179,23 +181,19 @@ const _: () = { use crate::generated_classes::js_Subprocess as js; impl<'a> Subprocess<'a> { - /// Wrap an already-heap-allocated `Subprocess` (via `heap::alloc`) in - /// its JS cell. `Bun.spawn` boxes early so address-dependent - /// back-pointers (`stdin.pipe.signal`, MaxBuf owner, IPC owner) can be - /// wired before `subprocess.toJS(globalThis)` runs; this is the raw-ptr - /// entrypoint that avoids re-boxing. - /// - /// `ptr` must come from `heap::alloc(Box::new(Subprocess { .. }))` and - /// not yet be owned by any JS wrapper; ownership transfers to the C++ - /// side (released via `SubprocessClass__finalize`). Thin forwarder to - /// the (already safe) generated `js_Subprocess::to_js`, which - /// encapsulates the FFI `__create` call internally. + /// Wrap an already-allocated `Subprocess` in its JS cell. `this` + /// becomes the wrapper's ref (stored as `m_ctx`); [`Self::finalize`] + /// releases it. `Bun.spawn` allocates before it has a wrapper so that + /// address-dependent back-pointers (`stdin.pipe.signal`, MaxBuf owner, + /// IPC owner) can be wired first; this is the entrypoint that avoids + /// re-boxing. Thin forwarder to the (already safe) generated + /// `js_Subprocess::to_js`, which encapsulates the FFI `__create` call. #[inline] - pub(crate) fn to_js_from_ptr(ptr: *mut Self, global: &JSGlobalObject) -> JSValue { + pub(crate) fn to_js_from_ref(this: OwnedRef, global: &JSGlobalObject) -> JSValue { // The codegen wrapper is monomorphized at `'static`; the lifetime // parameter is purely a borrow-checker artifact (C++ stores the // pointer as opaque `m_ctx`), so erase it via `cast`. - js::to_js(ptr.cast(), global) + js::to_js(this.into_raw().cast(), global) } } @@ -1287,12 +1285,22 @@ impl Subprocess<'_> { } } + /// JS wrapper finalizer. The `Box` is the codegen's spelling of the ref + /// `to_js_from_ref` gave the wrapper; the allocation outlives this call if + /// the exit handler or a pipe still holds a ref, so it is turned back into + /// that ref rather than dropped as a `Box`. pub fn finalize(self: Box) { + // SAFETY: the wrapper holds exactly the one ref `to_js_from_ref` + // handed it, and the finalizer runs once. + Self::finalize_owned(unsafe { OwnedRef::from_raw(bun_core::heap::into_raw(self)) }); + } + + /// Tear down and release the ref that stands for the JS wrapper: the + /// wrapper's own ref from the finalizer above, or, for `spawnSync`, which + /// never creates a wrapper, the ref `spawn_maybe_sync` holds. + pub(crate) fn finalize_owned(owned: OwnedRef) { bun_output::scoped_log!(Subprocess, "finalize"); - // Refcounted: the trailing `this.deref()` releases the JS wrapper's +1; - // allocation may outlive this call if other refs remain, so hand - // ownership back to the raw refcount. - let this = bun_core::heap::release(self); + let this: &Self = &owned; // Ensure any code which references the "this" value doesn't attempt to // access it after it's been freed We cannot call any methods which // access GC'd values during the finalizer @@ -1364,7 +1372,9 @@ impl Subprocess<'_> { } this.update_flags(|f| f.insert(Flags::FINALIZED)); - this.deref(); + // Releases the ref; the allocation goes with it unless a pipe or the + // exit handler still holds one. + drop(owned); } pub(crate) fn get_exited(&self, this_value: JSValue, global_this: &JSGlobalObject) -> JSValue { diff --git a/src/runtime/api/bun/subprocess/Writable.rs b/src/runtime/api/bun/subprocess/Writable.rs index 4d34112c196e..34f28e99dcd9 100644 --- a/src/runtime/api/bun/subprocess/Writable.rs +++ b/src/runtime/api/bun/subprocess/Writable.rs @@ -37,7 +37,7 @@ impl<'a> Writable<'a> { /// i.e. the owner-outlives-holder `BackRef` invariant holds (single JS /// thread). #[inline] - pub(super) fn pipe_sink(pipe: NonNull) -> bun_ptr::BackRef { + pub(in crate::api) fn pipe_sink(pipe: NonNull) -> bun_ptr::BackRef { bun_ptr::BackRef::from(pipe) } @@ -169,7 +169,7 @@ impl<'a> Writable<'a> { pub(crate) fn init( stdio: &mut Stdio, event_loop: &EventLoop, - subprocess: &mut Subprocess<'a>, + subprocess: &Subprocess<'a>, result: StdioResult, promise_for_stream: &mut JSValue, ) -> crate::Result> { @@ -253,7 +253,7 @@ impl<'a> Writable<'a> { }; return Ok(Writable::Buffer(StaticPipeWriter::create( evtloop, - subprocess as *mut Subprocess<'a>, + subprocess.as_ctx_ptr(), result, super::source_from_blob(blob), ))); @@ -261,7 +261,7 @@ impl<'a> Writable<'a> { Stdio::ArrayBuffer(array_buffer) => { return Ok(Writable::Buffer(StaticPipeWriter::create( evtloop, - subprocess as *mut Subprocess<'a>, + subprocess.as_ctx_ptr(), result, super::source_from_array_buffer(core::mem::take(array_buffer)), ))); @@ -358,14 +358,14 @@ impl<'a> Writable<'a> { }; Ok(Writable::Buffer(StaticPipeWriter::create( evtloop, - std::ptr::from_mut::>(subprocess), + subprocess.as_ctx_ptr(), result, super::source_from_blob(blob), ))) } Stdio::ArrayBuffer(array_buffer) => Ok(Writable::Buffer(StaticPipeWriter::create( evtloop, - std::ptr::from_mut::>(subprocess), + subprocess.as_ctx_ptr(), result, super::source_from_array_buffer(core::mem::take(array_buffer)), ))),