Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,10 @@ pub struct VirtualMachine {
pub(crate) macro_event_loop: EventLoop,
pub regular_event_loop: EventLoop,
pub event_loop: *mut EventLoop, // BORROW_FIELD — points at sibling regular_event_loop/macro_event_loop
/// See [`crate::any_task_job::AnyTaskGate`]. `Option` only so the field is
/// zero-valid for `init()`'s `alloc_zeroed`; written there, always `Some`
/// until `destroy()`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub any_task_gate: Option<std::sync::Arc<crate::any_task_job::AnyTaskGate>>,

pub(crate) ref_strings: crate::ref_string::Map,
pub(crate) ref_strings_mutex: bun_threading::Mutex,
Expand Down Expand Up @@ -752,6 +756,16 @@ impl VirtualMachine {
unsafe { &*self.event_loop }
}

/// See [`crate::any_task_job::AnyTaskGate`]. Always `Some` between
/// `init()` and `destroy()`.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[inline]
pub fn any_task_gate(&self) -> &std::sync::Arc<crate::any_task_job::AnyTaskGate> {
debug_assert!(self.any_task_gate.is_some());
// SAFETY: written in `init()`, taken in `destroy()`; every caller is
// between the two.
unsafe { self.any_task_gate.as_ref().unwrap_unchecked() }
}

/// Alias for [`Self::event_loop_mut`]. Kept for callers migrated on the
/// `runtime-hostfn-safe` branch; both names funnel into the single audited
/// `unsafe` deref above.
Expand Down Expand Up @@ -1555,6 +1569,11 @@ impl VirtualMachine {
// drain below) or observes the flag under m_lock and drops.
// destructOnExit sets it again (idempotently).
Bun__JSCTaskScheduler__markShuttingDown(self.global());
// Same mirror for work-pool `AnyTaskJob`s (see web_worker.rs
// shutdown()): wait out any in-flight `ctx.run()` (which may be
// reading/writing a JSC `ArrayBuffer`) before `destructOnExit`
// frees the JSC heap.
Comment thread
robobun marked this conversation as resolved.
Outdated
self.any_task_gate().close();

// Every worker has now posted its close task to our concurrent
// queue (OUTSTANDING is decremented after dispatchExit). Drop
Expand Down Expand Up @@ -2136,6 +2155,7 @@ impl VirtualMachine {
(*regular).virtual_machine = NonNull::new(vm);
let _ = (*regular).tasks.ensure_unused_capacity(64);
addr_of_mut!((*vm).event_loop).write(regular);
addr_of_mut!((*vm).any_task_gate).write(Some(crate::any_task_job::AnyTaskGate::new()));

// `source_mappings.map` is a sibling-field backref onto
// `saved_source_map_table`.
Expand Down Expand Up @@ -4475,6 +4495,11 @@ impl VirtualMachine {
// so reclaim it here or every Worker leaks it.
drop(core::mem::take(&mut self.preload));

// Release this VM's `AnyTaskGate` ref; the gate itself is freed once
// every in-flight pool-thread job that observed the close has dropped
// its clone.
Comment thread
robobun marked this conversation as resolved.
Outdated
drop(self.any_task_gate.take());

// SAFETY: this VM is raw-`dealloc`'d (no field `Drop` runs), so
// `transpiler` is never auto-dropped after `deinit` clears its fields.
unsafe { self.transpiler.deinit() };
Expand Down
101 changes: 92 additions & 9 deletions src/jsc/any_task_job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,66 @@

use core::ffi::c_void;
use core::ptr::NonNull;
use std::sync::Arc;

use bun_event_loop::AnyTask::AnyTask;
use bun_io::KeepAlive;
use bun_threading::RwLock;
use bun_threading::work_pool::{IntrusiveWorkTask as _, Task as WorkPoolTask, WorkPool};

use crate::event_loop::ConcurrentTask;
use crate::{JSGlobalObject, JsResult, VirtualMachineRef as VirtualMachine};

/// Close-once fence between a VM and every [`AnyTaskJob`] it schedules, so the
/// pool-thread work body and completion either run entirely before
/// `worker.terminate()` tears the VM down, or not at all.
///
/// Each VM owns one `Arc` clone; each in-flight job holds another (so the gate
/// itself outlives both). The pool thread runs `ctx.run()` and the
/// `enqueue_task_concurrent` push under a read lock: several ctxs read inputs
/// from (and `Scrypt` writes its output into) JSC-heap-backed `ArrayBuffer`s
/// that `Heap::lastChanceToFinalize` frees regardless of `protect()`, and the
/// enqueue itself dereferences the embedded `EventLoop`, so both must be
/// ordered before JSC-heap teardown and the VM free.
///
/// [`crate::web_worker::WebWorker`]'s shutdown calls [`Self::close`] before
/// draining the concurrent queue, before JSC teardown, and before deallocating
/// the VM. Taking the write lock waits out every in-flight reader (so the KDF
/// finishes and its push is visible to the drain), and once `closed` is set
/// any later pool task skips its body entirely. Same fence-then-drain shape as
/// `ScriptExecutionContext::markTerminating` for the C++ `postTaskTo` path.
/// Matches Node's behaviour of draining in-flight libuv work on env teardown.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub struct AnyTaskGate {
closed: RwLock<bool>,
}

impl AnyTaskGate {
pub fn new() -> Arc<Self> {
Arc::new(Self {
closed: RwLock::new(false),
})
}

/// Mark the owning VM as going away. Takes the write lock, so it returns
/// only after every concurrent [`Self::run_gated`] reader has released.
Comment thread
robobun marked this conversation as resolved.
pub fn close(&self) {
*self.closed.write() = true;
}

/// Run `body` under a read lock if the gate is open, else skip it. The
/// lock held across `body` orders it entirely before [`Self::close`]
/// returns. Returns `false` when closed (body not run).
Comment thread
robobun marked this conversation as resolved.
fn run_gated(&self, body: impl FnOnce()) -> bool {
let guard = self.closed.read();
if *guard {
return false;
}
body();
drop(guard);
true
}
}

/// Per-job payload trait. Implementors own the off-thread work body and the
/// JS-thread completion; the surrounding heap/queue/keep-alive plumbing is
/// supplied by [`AnyTaskJob`].
Expand Down Expand Up @@ -49,6 +101,10 @@ pub trait AnyTaskJobCtx: Sized {
/// e.g. a `JSPromiseStrong` field after scheduling.
pub struct AnyTaskJob<C> {
vm: bun_ptr::BackRef<VirtualMachine>,
/// The pool-thread [`Self::run_task`] only dereferences `vm` (and the
/// JSC-heap buffers `ctx` borrows) under this gate's read lock, so a
/// worker VM freed by terminate() is never touched. See [`AnyTaskGate`].
Comment thread
robobun marked this conversation as resolved.
Outdated
gate: Arc<AnyTaskGate>,
task: WorkPoolTask,
any_task: AnyTask,
poll: KeepAlive,
Expand All @@ -72,9 +128,11 @@ impl<C: AnyTaskJobCtx> AnyTaskJob<C> {
/// (running `Drop for C`). The returned pointer is owned by the caller
/// until handed to [`Self::schedule`].
pub fn create(global: &JSGlobalObject, ctx: C) -> JsResult<*mut Self> {
let vm = bun_ptr::BackRef::new(global.bun_vm());
let vm_ref = global.bun_vm();
let vm = bun_ptr::BackRef::new(vm_ref);
let job = bun_core::heap::into_raw(Box::new(Self {
vm,
gate: Arc::clone(vm_ref.any_task_gate()),
task: WorkPoolTask {
node: Default::default(),
callback: Self::run_task,
Expand Down Expand Up @@ -137,14 +195,39 @@ impl<C: AnyTaskJobCtx> AnyTaskJob<C> {
fn run_task(task: *mut WorkPoolTask) {
// SAFETY: only reachable via the `WorkPoolTask::callback` slot wired
// in `create`; `task` points to `Self.task` and the job is live until
// `run_from_js` reclaims it.
let job = unsafe { &mut *Self::from_task_ptr(task) };
let vm = job.vm;
job.ctx.run(vm.global);
// `ConcurrentTask::create` heap-allocates a fresh task; the queue takes
// ownership of it.
vm.event_loop_shared()
.enqueue_task_concurrent(ConcurrentTask::create(job.any_task.task()));
// `run_from_js` reclaims it (or is leaked below).
let this = unsafe { Self::from_task_ptr(task) };
// SAFETY: `gate` was written in `create`; this thread exclusively owns
// `*this` until the enqueue below hands it to the JS thread. Cloned so
// the read lock can be held while `this` is reborrowed `&mut` inside.
let gate = Arc::clone(unsafe { &(*this).gate });
let ran = gate.run_gated(|| {
// SAFETY: gate open ⇒ worker shutdown hasn't passed `close()` ⇒
// the owning VM, its JSC heap (which `ctx.run` may read/write via
// `ArrayBuffer`-backed inputs/outputs), and its embedded event
// loop are all still live.
let job = unsafe { &mut *this };
let vm = job.vm;
job.ctx.run(vm.global);
// `ConcurrentTask::create` heap-allocates; the queue takes
// ownership.
Comment thread
robobun marked this conversation as resolved.
vm.event_loop_shared()
.enqueue_task_concurrent(ConcurrentTask::create(job.any_task.task()));
});
drop(gate);
if ran {
return;
}
// Gate closed (worker terminated before this task was picked up).
// SAFETY: `this` is the sole owner (`run_gated` did not touch it);
// `gate` was written in `create` and is never read again past this
// point (the box is leaked below).
unsafe { core::ptr::drop_in_place(core::ptr::addr_of_mut!((*this).gate)) };
// The remainder of the box (`poll`, `ctx`) is intentionally leaked:
// `ctx` holds `Strong` JSC handles into the dead VM's heap and
// `poll.unref` would touch the freed event loop. This matches the fate
// of tasks already queued on a terminated worker's never-drained
// concurrent queue.
Comment thread
robobun marked this conversation as resolved.
Outdated
}

/// `AnyTask` callback — runs ON the JS thread. Reclaims the heap
Expand Down
7 changes: 7 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1302,6 +1302,13 @@ impl WebWorker {
// or observes m_isShuttingDown under m_lock and drops. Idempotent;
// teardownJSCVM sets it again.
Bun__JSCTaskScheduler__markShuttingDown(vm.global());
// Same fence for work-pool jobs (`AnyTaskJob`: node:crypto KDFs/
// HKDF/primes/keypairs/sign, Bun.secrets, Bun.zstd*). `close()`'s
// write lock waits out every pool thread that is inside
// `ctx.run()` or the enqueue (both touch the JSC heap / `vm`), so
// they finish before teardownJSCVM and step 5's free; a job the
// pool picks up after this observes `closed` and skips its body.
Comment thread
robobun marked this conversation as resolved.
Outdated
vm.any_task_gate().close();
// Reclaim queued CppTasks (the per-worker stdio/messaging
// MessagePort drain tasks that can be in self.tasks mid-tick when
// terminate() lands, and any Worker dispatchExit close task from a
Expand Down
57 changes: 57 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -455,3 +455,60 @@
},
120_000,
);

// Regression: worker.terminate() while an async crypto.pbkdf2()/scrypt() was
// running on the shared work pool freed the worker VM (and its JSC heap) out
// from under the pool thread. AnyTaskJob::run_task both (a) handed the
// caller's ArrayBuffer-backed password/salt to BoringSSL (freed by
// Heap::lastChanceToFinalize) and (b) dereferenced the VM box to post the
// completion (freed by the worker's dealloc). Release builds segfaulted the
// whole process; ASAN reported the UAF on a "Bun Pool" thread inside
// PKCS5_PBKDF2_HMAC / EVP_PBE_scrypt or VirtualMachine::event_loop_shared.
test(
"terminate() while async crypto.pbkdf2()/scrypt() with Buffer inputs is running on the work pool does not UAF",
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const { Worker } = require("node:worker_threads");
const src =
'const { parentPort } = require("node:worker_threads");' +
'const crypto = require("node:crypto");' +
// Salt length NOT a multiple of 64: SHA-256's block function is
// uninstrumented assembly, only the <64-byte tail goes through
// memcpy where ASAN can observe the freed read.
'const salt = Buffer.alloc((4 << 20) - 17, 0xaa);' +
'const pw = salt.subarray(0, 1 << 20);' +
// Heavy enough params that each job is still inside BoringSSL on a
// pool thread when terminate() lands (so the worker event loop never
// reaches the JS-thread completion), but bounded so the fix's
// close() wait stays well under the test timeout.
'for (let k = 0; k < 2; k++) crypto.pbkdf2(pw, salt, 200000, 64, "sha256", () => {});' +
'for (let k = 0; k < 2; k++) crypto.scrypt(salt, pw, 32, { N: 1 << 14, r: 8, p: 1, maxmem: 128 << 20 }, () => {});' +
'parentPort.postMessage("up");';
for (let r = 0; r < ${rounds}; r++) {
const w = new Worker(src, { eval: true });
w.on("error", () => {});
await new Promise((res) => w.once("message", res));

Check warning on line 494 in test/js/web/workers/worker-terminate-lifetime.test.ts

View check run for this annotation

Claude / Claude Code Review

Readiness wait swallows worker startup errors instead of rejecting

The readiness wait registers the blanket `w.on("error", () => {})` suppressor *before* awaiting the "up" message, and the promise has no `error`/`exit` rejection wired — so a worker startup failure would surface as a non-diagnostic `{stdout:'', stderr:'', exitCode:13}` mismatch rather than the actual cause. The three sibling tests in this file (the `ready()` helper at ~lines 300/439, and the require() test at ~394) wire `error`/`exit` to reject the readiness promise and only attach the suppresso
Comment thread
robobun marked this conversation as resolved.
Outdated
await w.terminate();
}
console.log("ok");
`,
],
// A job still on the pool when the gate closes is leaked by design (its
// ctx holds Strong/JSPromiseStrong handles into the dead VM's JSC heap,
// and poll.unref would touch the freed event loop). Bounded per
// terminated worker; opt this subprocess out of LSan so that stranded
// accounting is not asserted as a regression.
env: { ...bunEnv, ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":") },
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 });
},
timeout,
);
Loading