diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index b5a58a65b564..6320313b7bb3 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1780,8 +1780,7 @@ pub struct RuntimeHooks { /// the caller writes the returned bool back into /// `transform_options.allow_addons` so the override semantics /// ("override the existing even if it was set") match. - pub parse_worker_exec_argv_allow_addons: - unsafe fn(exec_argv: &[bun_core::WTFStringImpl]) -> Option, + pub parse_worker_exec_argv_allow_addons: fn(exec_argv: &[Box<[u8]>]) -> Option, /// `CronJob.clearAllForVM(vm, .teardown)`. `CronJob` lives in /// `bun_runtime::api::cron`. pub cron_clear_all_teardown: fn(vm: &mut VirtualMachine), @@ -2327,16 +2326,7 @@ impl VirtualMachine { || self .worker_ref() .and_then(crate::web_worker::WebWorker::exec_argv) - .is_some_and(|exec_argv| { - use bun_core::WTFStringImplExt as _; - exec_argv.iter().any(|&arg| { - // SAFETY: each entry borrows the C++ `WorkerOptions` - // array, kept alive by the owning `WebCore::Worker` - // for the worker's lifetime (see `WebWorker::argv`). - !arg.is_null() - && is_bootstrap_flag(unsafe { &*arg }.to_owned_slice_z().as_bytes()) - }) - }); + .is_some_and(|exec_argv| exec_argv.iter().any(|arg| is_bootstrap_flag(arg))); if needs_pre_execution { // The C++ side catches and reports any JS exception thrown while // evaluating `internal/process/pre_execution`. diff --git a/src/jsc/web_worker.rs b/src/jsc/web_worker.rs index 55e9736de75c..0dca7bf4e0e6 100644 --- a/src/jsc/web_worker.rs +++ b/src/jsc/web_worker.rs @@ -64,7 +64,7 @@ use core::ffi::c_void; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicU32, Ordering}; -use bun_core::{String as BunString, WTFStringImpl}; +use bun_core::{String as BunString, WTFStringImpl, WTFStringImplExt as _}; use bun_io::KeepAlive; use bun_threading::{Futex, Mutex}; @@ -97,12 +97,16 @@ pub struct WebWorker { mini: bool, eval_mode: bool, store_fd: bool, - /// Borrowed from C++ `WorkerOptions` (kept alive by the owning `Worker`). - argv_ptr: *const WTFStringImpl, - argv_len: usize, - exec_argv_ptr: *const WTFStringImpl, - exec_argv_len: usize, - inherit_exec_argv: bool, + /// Owned UTF-8 copies of the `WorkerOptions` argv/execArgv strings, made + /// on the parent thread in [`Self::create`]. The worker thread must never + /// see the parent's `WTF::StringImpl`s: wrapping one in a worker-heap + /// `JSString` lets worker-side atomization insert the shared impl into the + /// worker's thread-local atom table, and the parent's final deref (GC + /// sweep of the `Worker` wrapper) then aborts in `AtomStringImpl::remove` + /// ("the atom is in the string table of an other thread"). + argv: Vec>, + /// `None` when the worker inherits the parent's execArgv. + exec_argv: Option>>, /// Heap-owned by this struct; freed in `destroy()`. unresolved_specifier: Box<[u8]>, preloads: Vec>, @@ -436,26 +440,17 @@ impl WebWorker { self.eval_mode } - /// Borrowed from the C++ `WorkerOptions` (kept alive by the owning - /// `WebCore::Worker`). + /// Worker-owned UTF-8 copies of the `WorkerOptions` argv strings. #[inline] - pub fn argv(&self) -> &[WTFStringImpl] { - // SAFETY: `argv_ptr[..argv_len]` is borrowed from C++ WorkerOptions - // (BACKREF — kept alive by the owning Worker for `self`'s lifetime). - // `(null, 0)` is tolerated by `ffi::slice`. - unsafe { bun_core::ffi::slice(self.argv_ptr, self.argv_len) } + pub fn argv(&self) -> &[Box<[u8]>] { + &self.argv } - /// `None` when - /// `inherit_exec_argv` (the worker inherits the parent's execArgv), - /// otherwise `Some(slice)` (possibly empty) borrowed from C++ WorkerOptions. + /// `None` when the worker inherits the parent's execArgv, otherwise + /// `Some(slice)` (possibly empty) of worker-owned UTF-8 copies. #[inline] - pub fn exec_argv(&self) -> Option<&[WTFStringImpl]> { - if self.inherit_exec_argv { - return None; - } - // SAFETY: see `argv()`. - Some(unsafe { bun_core::ffi::slice(self.exec_argv_ptr, self.exec_argv_len) }) + pub fn exec_argv(&self) -> Option<&[Box<[u8]>]> { + self.exec_argv.as_deref() } fn set_requested_terminate(&self) -> bool { @@ -541,6 +536,28 @@ impl WebWorker { // SAFETY: `parent` is live (see above); borrow ends at `;`. let store_fd = unsafe { (*parent).transpiler.resolver.store_fd }; + // Copy argv/execArgv to worker-owned UTF-8 while still on the parent + // thread; see the `argv` field doc for why the `WTF::StringImpl`s must + // not cross into the worker thread. + let copy_args = |ptr: *const WTFStringImpl, len: usize| -> Vec> { + // SAFETY: caller passed a valid (ptr, len) pair (or `(null, 0)`, + // tolerated by `ffi::slice`) of live `WTF::StringImpl*`s kept + // alive by `Worker::create` across this call. + unsafe { bun_core::ffi::slice(ptr, len) } + .iter() + .map(|&s| { + // SAFETY: each element is a live `WTF::StringImpl*` (see above). + unsafe { &*s }.to_utf8().slice().to_vec().into_boxed_slice() + }) + .collect() + }; + let argv = copy_args(argv_ptr, argv_len); + let exec_argv = if inherit_exec_argv { + None + } else { + Some(copy_args(exec_argv_ptr, exec_argv_len)) + }; + let worker = bun_core::heap::into_raw(Box::new(WebWorker { cpp_worker, // `parent` is the calling thread's live VM; non-null by FFI contract. @@ -549,11 +566,8 @@ impl WebWorker { mini, eval_mode, store_fd, - argv_ptr, - argv_len, - exec_argv_ptr, - exec_argv_len, - inherit_exec_argv, + argv, + exec_argv, unresolved_specifier: spec_slice.slice().to_vec().into_boxed_slice(), preloads, name: if name_str.is_empty() { @@ -861,14 +875,9 @@ impl WebWorker { // RunCommand param table. The param table lives in // `bun_runtime::cli` (forward-dep), so dispatch through // `RuntimeHooks::parse_worker_exec_argv_allow_addons`. Currently - // only honours `--no-addons`; the hook owns the temporary UTF-8 - // alloc + clap parse + `args.deinit()`. `None` on parse failure + // only honours `--no-addons`. `None` on parse failure // (the parent's setting is kept). - - // SAFETY: `exec_argv` borrows C++ `WorkerOptions` kept alive by the - // owning `WebCore::Worker` for `self`'s lifetime; the hook only - // reads the slice and owns its own temporary allocations. - let parsed = unsafe { (hooks.parse_worker_exec_argv_allow_addons)(exec_argv) }; + let parsed = (hooks.parse_worker_exec_argv_allow_addons)(exec_argv); if let Some(allow_addons) = parsed { let parent_allows = transform_options.allow_addons.unwrap_or(true); transform_options.allow_addons = Some(parent_allows && allow_addons); diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index cd6e9a438d65..9469b3b3bdd8 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -20,7 +20,6 @@ //! 4. `__bun_get_vm_ctx` / `__bun_stdio_blob_store_new` / //! `__bun_http_sync_download_*` — low-tier extern impls. -use bun_core::WTFStringImplExt as _; use bun_options_types::LoaderExt as _; use core::cell::Cell; use core::ffi::c_void; @@ -1513,27 +1512,16 @@ unsafe fn apply_standalone_runtime_flags( /// /// Note: the Rust `bun_clap::parse_ex` port currently constrains /// `ArgIter<'static>` (parsed values are stored by reference), which would -/// force leaking the per-call UTF-8 copies of `exec_argv`. Spec only ever +/// force leaking a copy of `exec_argv`. Spec only ever /// reads the single `--no-addons` flag from the result (per the in-tree /// `// TODO: currently this only checks for --no-addons`), so this body scans -/// the converted argv directly with the same `stop_after_positional_at = 1` +/// the argv directly with the same `stop_after_positional_at = 1` /// short-circuit. Full clap routing can return when `ComptimeClap` grows a /// borrowed-lifetime variant. -/// -/// # Safety -/// Each `WTFStringImpl` in `exec_argv` is a live WTF string (the C++ -/// `Worker::create` array, kept alive for the worker's lifetime). -unsafe fn parse_worker_exec_argv_allow_addons( - exec_argv: &[bun_core::WTFStringImpl], -) -> Option { +fn parse_worker_exec_argv_allow_addons(exec_argv: &[Box<[u8]>]) -> Option { let mut no_addons = false; - for &arg in exec_argv { - if arg.is_null() { - continue; - } - // SAFETY: per fn contract — `arg` is a live `WTFStringImpl*`. - let owned = unsafe { &*arg }.to_owned_slice_z(); - let bytes = owned.as_bytes(); + for arg in exec_argv { + let bytes: &[u8] = arg; // `stop_after_positional_at = 1` — first non-flag token ends parsing. if bytes.first() != Some(&b'-') { break; diff --git a/src/runtime/node/node_process.rs b/src/runtime/node/node_process.rs index 088f0f53d1aa..99c1764ea564 100644 --- a/src/runtime/node/node_process.rs +++ b/src/runtime/node/node_process.rs @@ -234,8 +234,8 @@ mod _impl { if let Some(worker) = vm.worker_ref() { // was explicitly overridden for the worker? if let Some(exec_argv) = worker.exec_argv() { - return JSValue::create_array_from_iter(global_object, exec_argv.iter(), |&wtf| { - BunString::init(wtf).to_js(global_object) + return JSValue::create_array_from_iter(global_object, exec_argv.iter(), |arg| { + BunString::borrow_utf8(arg).to_js(global_object) }); } } @@ -402,8 +402,8 @@ mod _impl { } if let Some(worker) = worker { - for &arg in worker.argv() { - args_list.push(BunString::init(arg)); + for arg in worker.argv() { + args_list.push(BunString::borrow_utf8(arg)); } } else { for arg in &vm.argv { diff --git a/src/runtime/shell/interpreter.rs b/src/runtime/shell/interpreter.rs index 4e925f76cfb2..9691dfaeddb4 100644 --- a/src/runtime/shell/interpreter.rs +++ b/src/runtime/shell/interpreter.rs @@ -26,7 +26,6 @@ //! own data up via `interp.node_mut(this)` / `interp.nodes[this]`. use bun_collections::VecExt; -use bun_core::WTFStringImplExt as _; use bun_jsc::JsCell; use core::cell::Cell; use core::fmt; @@ -308,10 +307,6 @@ pub struct Interpreter { pub(crate) cleanup_state: Cell, pub(crate) estimated_size_for_gc: Cell, - /// Lazily-populated UTF-8 cache for the JS-side argv (`$@`/`$N` expansion - /// when running under a Worker). See [`Interpreter::get_vm_args_utf8`]. - pub(crate) vm_args_utf8: JsCell>, - /// `bun run` CLI context for `$N` expansion on the mini event loop. /// Null when constructed from JS (no `ContextData` is reachable). pub(crate) command_ctx: *mut bun_options_types::context::ContextData, @@ -581,7 +576,6 @@ impl Interpreter { this_jsvalue: Cell::new(crate::jsc::JSValue::ZERO), cleanup_state: Cell::new(CleanupState::NeedsFullCleanup), estimated_size_for_gc: Cell::new(0), - vm_args_utf8: JsCell::new(Vec::new()), command_ctx: ctx, }); // Wire the interpreter backref into root stdin so async poll @@ -621,8 +615,6 @@ impl Interpreter { // Free buffered IO, env // maps, cwd fd; do NOT free the struct itself (it's embedded). self.root_shell.with_mut(|rs| rs.deinit_embedded(true)); - // `vm_args_utf8` slices Drop themselves (`ZigStringSlice` has a Drop - // impl that derefs the WTF backing); the Vec frees on box drop. } /// Standalone-shell entrypoint for `bun .sh`: parse `src` (already @@ -1025,11 +1017,6 @@ impl Interpreter { size += self.root_shell.get().memory_cost(); size += self.root_io.get().memory_cost(); size += self.jsobjs.len() * core::mem::size_of::(); - let vm_args = self.vm_args_utf8.get(); - for arg in vm_args { - size += arg.slice().len(); - } - size += vm_args.capacity() * core::mem::size_of::(); size } @@ -1381,9 +1368,7 @@ impl Interpreter { } this.keep_alive.with_mut(|k| k.disable()); - // `args: Box` and `vm_args_utf8: Vec` drop - // with the box; `ZigStringSlice` has a `Drop` impl that derefs its - // WTF backing. + // `args: Box` drops with the box. } pub(crate) fn is_running( @@ -1470,7 +1455,6 @@ impl Interpreter { original_int: u8, event_loop: EventLoopHandle, command_ctx: *mut bun_options_types::context::ContextData, - vm_args_utf8: &mut Vec, ) { let mut int = original_int; match event_loop { @@ -1505,19 +1489,9 @@ impl Interpreter { // SAFETY: `vm.worker` is set in `VirtualMachine::initWorker` // to a live `*WebWorker` for the worker's lifetime. let worker = unsafe { &*worker_ptr.cast::() }; - let argv = worker.argv(); - if int as usize >= argv.len() { - return; - } - if vm_args_utf8.len() != argv.len() { - vm_args_utf8.reserve(argv.len()); - for arg in argv { - // SAFETY: each `WTFStringImpl` in `argv` is a live - // `*WTF::StringImpl` borrowed from `worker.argv`. - vm_args_utf8.push(unsafe { (**arg).to_utf8() }); - } + if let Some(arg) = worker.argv().get(int as usize) { + out.extend_from_slice(arg); } - out.extend_from_slice(vm_args_utf8[int as usize].slice()); return; } diff --git a/src/runtime/shell/states/Expansion.rs b/src/runtime/shell/states/Expansion.rs index df8edbcf202d..4a4a0f168d3f 100644 --- a/src/runtime/shell/states/Expansion.rs +++ b/src/runtime/shell/states/Expansion.rs @@ -118,15 +118,8 @@ impl Expansion { /// `child_done` advances `word_idx`. pub(crate) fn next(interp: &Interpreter, this: NodeId) -> Yield { loop { - // Split-borrow: `me` from `nodes`, `vm_args_utf8` from its own - // field, so `expand_simple_no_io` can expand `$N` without aliasing. - // R-2: both are `JsCell`-backed; `as_ptr()`/`node_mut()` project - // disjoint `&mut` from `&Interpreter`. let event_loop = interp.event_loop; let command_ctx = interp.command_ctx; - // SAFETY: single-JS-thread; `vm_args_utf8` and `nodes` are - // disjoint `JsCell` fields (no aliasing between the two borrows). - let vm_args_utf8 = unsafe { &mut *interp.vm_args_utf8.as_ptr() }; let me = interp.as_expansion_mut(this); match me.state { ExpansionState::Idle => { @@ -180,7 +173,6 @@ impl Expansion { true, event_loop, command_ctx, - vm_args_utf8, ); if !is_cmd_subst { me.word_idx += 1; @@ -462,7 +454,6 @@ impl Expansion { expand_tilde: bool, event_loop: EventLoopHandle, command_ctx: *mut bun_options_types::context::ContextData, - vm_args_utf8: &mut Vec, ) -> bool { use crate::shell::env_str::EnvStr; match atom { @@ -486,8 +477,7 @@ impl Expansion { } } ast::SimpleAtom::VarArgv(int) => { - // SAFETY: `command_ctx` is the live VM ctx; `vm_args_utf8` borrows it. - Interpreter::append_var_argv(out, *int, event_loop, command_ctx, vm_args_utf8); + Interpreter::append_var_argv(out, *int, event_loop, command_ctx); } ast::SimpleAtom::Asterisk => { meta_offsets.push(out.len() as u32); diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index aa3bf5d714d8..15ad12777a15 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, setDefaultTimeout, test } from "bun:test"; -import { bunEnv, bunExe, isDebug, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isDebug, tempDir, tmpdirSync } from "harness"; import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; @@ -345,6 +345,94 @@ describe("execArgv option", async () => { // TODO(@190n) get our handling of non-string array elements in line with Node's }); +// The worker thread used to receive the parent's argv/execArgv WTF::StringImpls +// by pointer. Using one as a property key atomizes it in the worker's +// thread-local atom table, and when the parent GC'd the Worker wrapper while +// the worker thread was still exiting, the parent-side final deref aborted in +// AtomStringImpl::remove ("The string being removed is an atom in the string +// table of an other thread!") - on Windows a silent 0xC0000409 fastfail that +// took down `bun test --parallel` workers. The fixture forces that exact +// interleaving: pin the process to one CPU, idle-schedule the dying worker +// thread so it cannot reach its thread-local destructors, and run a full GC +// from the first microtask after the exit event. taskset/chrt are best-effort; +// without them (non-Linux) the fixture still exercises the path. +test("argv/execArgv atomized inside the worker don't abort when the parent collects the Worker first", async () => { + using dir = tempDir("worker-argv-atom", { + "repro.js": ` + const { Worker } = require("worker_threads"); + const { readdirSync, readFileSync } = require("fs"); + const { spawnSync } = require("child_process"); + + function pinAllThreadsToOneCpu() { + try { + const m = readFileSync("/proc/self/status", "utf8").match(/Cpus_allowed_list:\\s*(\\d+)/); + if (m) spawnSync("taskset", ["-a", "-pc", m[1], String(process.pid)]); + } catch {} + } + function idleSchedule(threadName) { + try { + for (const t of readdirSync("/proc/self/task")) { + if (readFileSync("/proc/self/task/" + t + "/comm", "utf8").trim() === threadName) { + spawnSync("chrt", ["-i", "-p", "0", t]); + return; + } + } + } catch {} + } + + const victimCode = \` + const { parentPort } = require("worker_threads"); + const sink = {}; + for (const a of process.argv) sink[a] = 1; + for (const a of process.execArgv) sink[a] = 1; + parentPort.postMessage("ready"); + setInterval(() => {}, 1000); + \`; + + (async () => { + pinAllThreadsToOneCpu(); + for (let r = 0; r < 3; r++) { + const name = "atomvictim" + r; + let w = new Worker(victimCode, { + eval: true, + name, + argv: ["--atom-argv-a-" + r, "--atom-argv-b-" + r], + execArgv: ["--no-addons"], + }); + await new Promise(res => w.once("message", res)); + idleSchedule(name); + const exited = new Promise(res => { + w.once("exit", () => { + // First microtask after the exit event: the wrapper is off the + // dispatch stack and the thread-held ref is gone, so this full + // GC destroys the WebCore::Worker and derefs the option strings. + queueMicrotask(() => { + Bun.gc(true); + res(); + }); + }); + }); + w.terminate(); + w = null; + await exited; + } + console.log("OK"); + process.exit(0); + })(); + `, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "repro.js"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "inherit", + }); + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); +}); + test("eval does not leak source code", async () => { const proc = Bun.spawn({ cmd: [bunExe(), "eval-source-leak-fixture.js"],