Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
16 changes: 16 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ 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 for
/// `alloc_zeroed` validity; always `Some` between `init()` and `destroy()`.
Comment thread
robobun marked this conversation as resolved.
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 +755,14 @@ impl VirtualMachine {
unsafe { &*self.event_loop }
}

#[inline]
pub fn any_task_gate(&self) -> &std::sync::Arc<crate::any_task_job::AnyTaskGate> {
debug_assert!(self.any_task_gate.is_some());
// SAFETY: `Some` between `init()` and `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 +1566,8 @@ impl VirtualMachine {
// drain below) or observes the flag under m_lock and drops.
// destructOnExit sets it again (idempotently).
Bun__JSCTaskScheduler__markShuttingDown(self.global());
// Same fence for work-pool `AnyTaskJob`s; see `AnyTaskGate`.
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 +2149,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 +4489,8 @@ impl VirtualMachine {
// so reclaim it here or every Worker leaks it.
drop(core::mem::take(&mut self.preload));

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
85 changes: 76 additions & 9 deletions src/jsc/any_task_job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,54 @@

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.
/// [`AnyTaskJob::run_task`] runs `ctx.run()` and the completion enqueue under
/// a read lock (both touch the JSC heap / VM box; `Heap::lastChanceToFinalize`
/// frees `ArrayBuffer` inputs regardless of `protect()`). Worker shutdown and
/// `global_exit` call [`Self::close`] (write lock, waits out readers) before
/// JSC teardown and the VM free. Same fence-then-drain shape as
/// `ScriptExecutionContext::markTerminating`.
Comment thread
robobun marked this conversation as resolved.
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 +89,9 @@ pub trait AnyTaskJobCtx: Sized {
/// e.g. a `JSPromiseStrong` field after scheduling.
pub struct AnyTaskJob<C> {
vm: bun_ptr::BackRef<VirtualMachine>,
/// [`Self::run_task`] only dereferences `vm`/JSC-heap buffers under this
/// gate's read lock. See [`AnyTaskGate`].
Comment thread
robobun marked this conversation as resolved.
gate: Arc<AnyTaskGate>,
task: WorkPoolTask,
any_task: AnyTask,
poll: KeepAlive,
Expand All @@ -72,9 +115,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 +182,36 @@ 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: sole owner of `*this` (`run_gated` did not touch it); `gate`
// is never read again (the box is leaked below).
unsafe { core::ptr::drop_in_place(core::ptr::addr_of_mut!((*this).gate)) };
// `poll`/`ctx` are intentionally leaked: both would touch the freed
// VM/JSC heap on Drop. Same fate as tasks stranded on a terminated
// worker's never-drained concurrent queue.
Comment thread
robobun marked this conversation as resolved.
}

/// `AnyTask` callback — runs ON the JS thread. Reclaims the heap
Expand Down
2 changes: 2 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1302,6 +1302,8 @@ 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 `AnyTaskJob`s; see `AnyTaskGate`.
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
61 changes: 61 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,64 @@ test.skipIf(!isDebug)(
},
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 });
await new Promise((res, rej) => {
w.once("message", res);
w.once("error", rej);
w.once("exit", (c) => rej(new Error("worker exited " + c + " before ready")));
});
w.on("error", () => {});
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