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
17 changes: 17 additions & 0 deletions src/bundler/BundleThread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@
/// The trait accessors keep the generic `BundleThread<C>`
/// layout-agnostic. The concrete impl lives in T6 (`bun_bundler_jsc`).
pub trait CompletionStruct: Node + Send + 'static {
/// Whether the JS context that scheduled this build is still alive. When
/// `false`, the bundle thread must not run `generate_in_new_thread`: the
/// task borrows worker-owned resources (env loader, event loop) that may
/// already be freed. Defaults to `true` so non-worker callers stay no-op.
fn is_owner_alive(&self) -> bool {
true
}
/// `bump` is the per-build mimalloc heap that backs `transpiler`, so the
/// two share lifetime `'a` (option fields like `optimize_imports: &'a
/// StringSet` borrow from `bump`).
Expand Down Expand Up @@ -221,6 +228,16 @@
// SAFETY: queue stores non-null *mut C pushed via enqueue(); owner keeps it alive
// until complete_on_bundle_thread() signals completion.
let completion = unsafe { &mut *completion };
if !completion.is_owner_alive() {
// The worker that scheduled this build has begun shutdown.
// `create_and_configure_transpiler` would dereference
// worker-owned state (env loader) that is already freed.
// `complete_on_bundle_thread` observes the same dead
// context and drops the post.
completion.complete_on_bundle_thread();
has_bundled = true;
continue;
}

Check failure on line 240 in src/bundler/BundleThread.rs

View check run for this annotation

Claude / Claude Code Review

is_owner_alive() TOCTOU: env loader can still be freed between check and use

The `is_owner_alive()` pre-check is a check-then-act TOCTOU that narrows but does not close the env-loader UAF: `ScriptExecutionContext__isAlive` releases the contexts-map lock before `generate_in_new_thread` runs, so nothing prevents the worker from calling `markTerminating()` and freeing `env_loader` (web_worker.rs:1401) between the check and `Transpiler::init` writing through it — and once the bundle is running, `resolver.env_loader` is held and dereferenced (e.g. the `NODE_PATH` lookup at re
Comment thread
robobun marked this conversation as resolved.
// 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
Expand Down
41 changes: 41 additions & 0 deletions src/jsc/JSGlobalObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1567,6 +1567,22 @@ unsafe extern "C" {
}

impl ScriptExecutionContextIdentifier {
/// Enqueue a heap-allocated [`ConcurrentTaskItem`] onto this context's JS
/// event loop from any thread. Serializes with worker termination on the
/// C++ contexts-map lock (the same lock `markTerminating()` takes before
/// the worker's VM is freed), so this is safe to call without holding any
/// pointer into the target VM.
///
/// Returns `true` if the task was enqueued (ownership transferred).
/// Returns `false` if the context is gone or terminating; the caller
/// retains ownership of `task` and must free it.
pub fn post_concurrent_task(
self,
task: core::ptr::NonNull<crate::event_loop::ConcurrentTaskItem>,
) -> bool {
ScriptExecutionContext__postConcurrentTask(self.0, task.as_ptr().cast::<c_void>())
}

/// Returns `None` if the context referred to by `self` no longer exists.
pub fn global_object(self) -> Option<GlobalRef> {
// FFI call returns a valid pointer or null; the JSGlobalObject is owned
Expand All @@ -1587,9 +1603,34 @@ impl ScriptExecutionContextIdentifier {
pub fn valid(self) -> bool {
self.global_object().is_some()
}

/// `true` if the context still exists and has not been marked terminating.
/// Serializes with `markTerminating()` on the C++ contexts-map lock.
pub fn is_alive(self) -> bool {
ScriptExecutionContext__isAlive(self.0)
}
}

unsafe extern "C" {
// safe: by-value `u32` in, raw nullable pointer out (caller checks before deref).
safe fn ScriptExecutionContextIdentifier__getGlobalObject(id: u32) -> *mut JSGlobalObject;
// safe: by-value `u32` in, opaque pointer C++ never dereferences (it only
// passes it back to Rust under the contexts-map lock); bool out.
safe fn ScriptExecutionContext__postConcurrentTask(id: u32, task: *mut c_void) -> bool;
// safe: by-value `u32` in; bool out.
safe fn ScriptExecutionContext__isAlive(id: u32) -> bool;
// safe: `&JSGlobalObject` is a live opaque handle; returns the context's id.
safe fn ScriptExecutionContextIdentifier__forGlobalObject(
global: &JSGlobalObject,
) -> u32;
}

impl JSGlobalObject {
/// The stable identifier for this global's `ScriptExecutionContext`.
/// Safe to capture for later use from another thread via
/// [`ScriptExecutionContextIdentifier::post_concurrent_task`].
#[inline]
pub fn script_execution_context_identifier(&self) -> ScriptExecutionContextIdentifier {
ScriptExecutionContextIdentifier(ScriptExecutionContextIdentifier__forGlobalObject(self))
}
}
28 changes: 28 additions & 0 deletions src/jsc/bindings/ScriptExecutionContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,34 @@ extern "C" JSC::JSGlobalObject* ScriptExecutionContextIdentifier__getGlobalObjec
return context->globalObject();
}

// Checks under the contexts-map lock whether the context still exists and has
// not been marked terminating. Used by off-thread work (bundle thread) that
// borrows worker-owned resources, to skip a queued unit of work whose owning
// worker has already begun shutdown.
extern "C" bool ScriptExecutionContext__isAlive(ScriptExecutionContextIdentifier id)
{
Locker locker { allScriptExecutionContextsMapLock };
auto* context = allScriptExecutionContextsMap().get(id);
return context && !context->isTerminating();
}

extern "C" void Bun__EventLoop__enqueueConcurrentTask(JSC::JSGlobalObject*, void* task);

// postTaskTo() for a Rust-allocated ConcurrentTask. Returns true if the task
// was enqueued (ownership transferred to the target context's concurrent
// queue); false if the context is gone or terminating, in which case the
// caller retains ownership. The map lock is held across the enqueue so this
// serializes with markTerminating() the same way postTaskTo() does.
extern "C" bool ScriptExecutionContext__postConcurrentTask(ScriptExecutionContextIdentifier id, void* task)
{
Locker locker { allScriptExecutionContextsMapLock };
auto* context = allScriptExecutionContextsMap().get(id);
if (!context || context->isTerminating())
return false;
Bun__EventLoop__enqueueConcurrentTask(context->globalObject(), task);
return true;
}

extern "C" void ScriptExecutionContext__markTerminating(JSC::JSGlobalObject* globalObject)
{
if (auto* context = defaultGlobalObject(globalObject)->scriptExecutionContext())
Expand Down
19 changes: 19 additions & 0 deletions src/jsc/virtual_machine_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,25 @@
}
}

/// Called from `ScriptExecutionContext__postConcurrentTask` with the contexts
/// map lock held, so `global`'s VM and its `EventLoop` are guaranteed live for
/// the duration of this call (worker `markTerminating()` serializes on the
/// same lock before any dealloc). `task` is a heap `ConcurrentTaskItem` the
/// caller has already allocated.
// HOST_EXPORT(Bun__EventLoop__enqueueConcurrentTask, c)
pub fn event_loop_enqueue_concurrent_task(
global: &JSGlobalObject,
task: *mut crate::event_loop::ConcurrentTaskItem,
) {
crate::mark_binding!();
// SAFETY: see fn doc — VM/EventLoop live under the held contexts-map lock;
// `task` is a non-null heap allocation handed over from the Rust caller.
unsafe {
(*(*global.bun_vm_concurrently()).event_loop())
.enqueue_task_concurrent(core::ptr::NonNull::new_unchecked(task));

Check failure on line 159 in src/jsc/virtual_machine_exports.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

this public function might dereference a raw pointer but is not marked `unsafe`
}
}

// HOST_EXPORT(Bun__handleRejectedPromise, c)
pub fn handle_rejected_promise(global: &JSGlobalObject, promise: &mut JSPromise) {
crate::mark_binding!();
Expand Down
46 changes: 35 additions & 11 deletions src/runtime/api/js_bundle_completion_task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use bun_jsc::AnyTask::AnyTask;
use bun_jsc::WorkPool;
use bun_jsc::event_loop::EventLoop;
use bun_jsc::js_global_object::ScriptExecutionContextIdentifier;
use bun_jsc::{self as jsc, JSGlobalObject, JSPromise, JSValue};
use bun_options_types::WindowsOptions;
use bun_options_types::schema::api;
Expand Down Expand Up @@ -58,9 +59,14 @@
// `unsafe impl Send` below for the thread-affinity constraint this imposes.
pub ref_count: RefCount<Self>,
pub config: JSBundlerConfig,
// BACKREF — the JS-thread `EventLoop` outlives every completion task; safe
// `Deref` so call sites read `self.jsc_event_loop.enqueue_task_concurrent(..)`.
pub jsc_event_loop: BackRef<EventLoop>,

Check warning on line 64 in src/runtime/api/js_bundle_completion_task.rs

View check run for this annotation

Claude / Claude Code Review

Dead jsc_event_loop field left behind after switching to context_id

After rerouting both consumers to `context_id.post_concurrent_task`, nothing reads `jsc_event_loop` anymore — the field, its now-false doc comment ('call sites read `self.jsc_event_loop.enqueue_task_concurrent(..)`'), and the `event_loop: *mut EventLoop` parameter of `create_and_schedule_completion_task` (now used only to populate this dead field) should be removed in this PR. The stale comments at `src/bundler/bundle_v2.rs:1504` and `src/bundler/BundleThread.rs:85` referencing `jsc_event_loop.e
Comment thread
robobun marked this conversation as resolved.
Outdated
/// Stable identifier for the originating `ScriptExecutionContext`. The
/// bundle thread posts completions via this id (under the C++ contexts-map
/// lock) instead of dereferencing `jsc_event_loop`, which dangles once a
/// worker owning the build is terminated mid-bundle.
pub context_id: ScriptExecutionContextIdentifier,
pub task: AnyTask,
pub global_this: BackRef<JSGlobalObject>,
pub promise: jsc::JSPromiseStrong,
Expand Down Expand Up @@ -125,6 +131,7 @@
// `event_loop` is the live JS-thread loop (caller derives it from
// `vm.event_loop()`); never null once `Bun.build` is reachable.
jsc_event_loop: BackRef::from(core::ptr::NonNull::new(event_loop).expect("event_loop")),
context_id: global_this.script_execution_context_identifier(),
task: AnyTask::default(),
global_this: BackRef::new(global_this),
promise: jsc::JSPromiseStrong::default(),
Expand Down Expand Up @@ -761,13 +768,16 @@
static COMPLETION_VTABLE: dispatch::CompletionDispatch = dispatch::CompletionDispatch {
result_is_err: |c| matches!(from_completion_handle(c).result, BundleV2Result::Err(_)),
enqueue_task_concurrent: |c, task| {
// `jsc_event_loop` is a `BackRef<EventLoop>` — safe Deref.
// SAFETY: `task` is a fresh heap-allocated non-null `ConcurrentTaskItem`
// passed through from the bundler vtable; the queue takes ownership.
unsafe {
from_completion_handle(c)
.jsc_event_loop
.enqueue_task_concurrent(core::ptr::NonNull::new_unchecked(task))
// passed through from the bundler vtable.
let task = unsafe { core::ptr::NonNull::new_unchecked(task) };
if !from_completion_handle(c).context_id.post_concurrent_task(task) {
// Target context is gone or terminating (worker was terminated
// mid-bundle). The queue would never drain this task; reclaim the
// heap `ConcurrentTaskItem` ourselves.
// SAFETY: `post_concurrent_task` returned false so ownership was
// not transferred; `task` was `ConcurrentTask::create`-allocated.
drop(unsafe { bun_core::heap::take(task.as_ptr()) });
}
},
};
Expand All @@ -784,6 +794,10 @@
}

impl CompletionStruct for JSBundleCompletionTask {
fn is_owner_alive(&self) -> bool {
self.context_id.is_alive()
}

/// Port of `JSBundleCompletionTask.configureBundler` — the post-init half
/// (everything after `transpiler.* = try Transpiler.init(...)`).
/// `Transpiler::init` itself is called by `create_and_configure_transpiler`
Expand Down Expand Up @@ -980,11 +994,21 @@
}

fn complete_on_bundle_thread(&mut self) {
// `jsc_event_loop` is a `BackRef<EventLoop>` — safe Deref.
// `ConcurrentTask::create` heap-allocates a fresh task; the
// queue takes ownership of it.
self.jsc_event_loop
.enqueue_task_concurrent(jsc::ConcurrentTask::create(self.task.task()));
// Post by stable context id so a worker terminated mid-bundle cannot
// leave `jsc_event_loop` dangling under us: `post_concurrent_task`
// serializes with `markTerminating()` on the C++ contexts-map lock and
// only dereferences the target VM while that lock is held.
let task = jsc::ConcurrentTask::create(self.task.task());
if !self.context_id.post_concurrent_task(task) {
// Target context is gone or terminating. `on_complete_anytask`
// will never run; reclaim the heap `ConcurrentTaskItem`. The
// `JSBundleCompletionTask` itself is deliberately leaked: its
// `deinit` touches JS-thread-owned state (`JSPromiseStrong`,
// `Plugin::destroy`, `KeepAlive`) that is already freed.
// SAFETY: `post_concurrent_task` returned false so ownership was
// not transferred; `task` was `ConcurrentTask::create`-allocated.
drop(unsafe { bun_core::heap::take(task.as_ptr()) });
}
}
fn set_result(&mut self, result: BundleV2Result) {
self.result = result;
Expand Down
58 changes: 58 additions & 0 deletions test/bundler/bun-build-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1513,3 +1513,61 @@
expect(stdout.trim()).toBe("OK 400");
expect(exitCode).toBe(0);
}, 180_000);

// Regression: terminating a worker with an in-flight Bun.build() would crash
// the whole process. The bundler runs on its own thread and posted the
// completion back into the terminated worker's freed EventLoop/VM
// (JSBundleCompletionTask::complete_on_bundle_thread -> enqueue_task_concurrent
// -> heap-use-after-free, or SIGSEGV on release builds). A second queued
// build then dereferenced the worker's freed env loader in Transpiler::init.
// Under ASAN (the gate's default build) this was deterministic; on release it
// SIGSEGV'd in a few rounds.
test(
"terminating a worker mid-Bun.build() does not crash the process",
async () => {
// ASAN catches the UAF on the first round; release needs the terminate to
// land mid-bundle, so scale the module graph / round count by build speed.
const slow = isDebug || isASAN;
const modules = slow ? 100 : 400;
const rounds = slow ? 4 : 12;

const files: Record<string, string> = {};
const lines: string[] = [];
for (let i = 0; i < modules; i++) {
files[`m${i}.ts`] =
`export const v${i}: number = ${i};\n` +
`export function f${i}(x: number) { return x + ${i}; }\n`;
lines.push(`import * as m${i} from "./m${i}.ts"; globalThis.k${i} = m${i};`);
}
files["entry.ts"] = lines.join("\n");
files["run.ts"] = `
const { Worker } = require("node:worker_threads");
const entry = process.argv[2];
const src = \`const { parentPort, workerData } = require("node:worker_threads");
const lanes = (n, f) => { for (let i = 0; i < n; i++) (async () => { for (;;) { try { await f(i); } catch {} } })(); };
lanes(2, () => Bun.build({ entrypoints: [workerData.entry], target: "bun", minify: true }));
parentPort.postMessage("up");\`;
for (let r = 0; r < ${rounds}; r++) {
const w = new Worker(src, { eval: true, workerData: { entry } });
await new Promise(res => w.once("message", res));
Comment thread
robobun marked this conversation as resolved.
Outdated
await Bun.sleep(60 + (r * 41) % 220);
await w.terminate();
}
console.log("OK " + ${rounds});
`;
using dir = tempDir("bun-build-worker-terminate", files);

await using proc = Bun.spawn({
cmd: [bunExe(), join(String(dir), "run.ts"), join(String(dir), "entry.ts")],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("use-after-free");

Check warning on line 1567 in test/bundler/bun-build-api.test.ts

View check run for this annotation

Claude / Claude Code Review

Test asserts on absence of crash strings in stderr (CLAUDE.md violation)

These two `.not.toContain` assertions on "use-after-free" / "Segmentation fault" are the pattern CLAUDE.md explicitly forbids ('NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.'). The load-bearing checks — `expect(stdout.trim()).toBe("OK " + rounds)` and `expect(exitCode).toBe(0)` two lines below — already catch a SIGSEGV/ASAN abort; drop these two lines (or fold everything into a single `expect({ stdout, stderr
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(stderr).not.toContain("Segmentation fault");
expect(stdout.trim()).toBe("OK " + rounds);
expect(exitCode).toBe(0);
},
isDebug || isASAN ? 120_000 : 45_000,
);
Loading