Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 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
148 changes: 148 additions & 0 deletions docs/ROOT-B-SHUTDOWN-FENCE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
# Root B: worker teardown vs. cross-thread completions

## The failure class

`WebWorker::shutdown` (`src/jsc/web_worker.rs:1216`) tears down a worker's
`VirtualMachine` in five ordered steps. Step 2 sets `is_shutting_down`, drains
timers, force-closes sockets and the c-ares channel, calls
`ScriptExecutionContext::markTerminating()` (so C++ `postTaskTo` posters are
fenced), and drains the concurrent queue via
`release_queued_tasks_for_shutdown()`. Step 5 `std::alloc::dealloc`'s the raw
`VirtualMachine` box (≈`web_worker.rs:1383`) and frees the uws loop.

Nothing in that sequence cancels or awaits work already handed to a
process-global thread (WorkPool, the HTTP thread, the bundle thread). Those
jobs complete later and post back through a pointer captured at schedule time:
a `BackRef<EventLoop>`, a `&'static VirtualMachine`, or a `*const
JSGlobalObject`. Every one of those pointers is into the freed VM box or the
freed JSC heap.

All reproduced cross-thread UAFs in this class converge on
**`EventLoop::enqueue_task_concurrent`** (`src/jsc/event_loop.rs:997`). Putting
a check _inside_ the funnel does not help: `&self` there is already a pointer
into the freed box. Several callers also "guard" with an off-thread
`vm.is_shutting_down()` read; that read is itself a UAF face (the flag lives in
the freed box).

## Shutdown map (reference)

| step | action | fences |
| ---- | -------------------------------------------------------------- | ----------------------------- |
| 1 | `self.vm = null` under `vm_lock` | parent-thread readers |
| 2a | `is_shutting_down = true`, `on_exit`, drain timers/sockets/DNS | on-thread re-entry |
| 2b | `ScriptExecutionContext::markTerminating()` | C++ `postTaskTo` posters |
| 2c | `Bun__JSCTaskScheduler__markShuttingDown()` | `Atomics.notify` posters |
| 2d | `release_queued_tasks_for_shutdown()` | tasks already queued |
| 3 | `WebWorker__teardownJSCVM` | GC finalizers, JSC heap freed |
| 4 | `WebWorker__dispatchExit` | parent releases its ref |
| 5 | `vm.destroy()`; `dealloc(vm_ptr)`; free uws loop | VM box freed |

Rust-side cross-thread posters were not serialized with any of 2b/2c/2d.

## The fence: enqueue by identifier

Off-thread jobs now carry the worker's `ScriptExecutionContextIdentifier` (a
`u32`) instead of a `BackRef<EventLoop>` / `&VirtualMachine`, and post through

```rust
ScriptExecutionContextIdentifier::post_concurrent_task(id, task) -> bool
```

backed by the same locked-registry + `isTerminating()` gate that
`ScriptExecutionContext::postTaskTo` already uses:

```cpp
extern "C" bool ScriptExecutionContext__postConcurrentTask(Identifier id, void* task) {
Locker locker { allScriptExecutionContextsMapLock };
auto* ctx = allScriptExecutionContextsMap().get(id);
if (!ctx || ctx->isTerminating()) return false;
Bun__EventLoop__enqueueConcurrentTask(ctx->globalObject(), task);
return true;
}
```

`markTerminating()` (shutdown step 2b) takes the same lock to set the flag, so
every poster serializes into exactly one of two cases:

1. The poster's whole critical section ran before `markTerminating()`: the task
is in the concurrent queue, and step 2d's drain observes and reclaims it.
2. `markTerminating()` ran first: the poster sees `isTerminating()` and returns
`false` without touching the VM. The caller owns the task and runs its
abandon path.

A `u32` identifier cannot dangle. The funnel's `bool` return replaces every
stale off-thread `is_shutting_down()` read.

Companion helpers on the same lock:

- `ScriptExecutionContextIdentifier::is_alive()` — "should I even start?" check
for work bodies that write into JSC-heap buffers (e.g. `Scrypt`'s output
`ArrayBuffer`). A best-effort fast drop; the authoritative gate is
`post_concurrent_task`.
- `ScriptExecutionContextIdentifier::unref_event_loop_concurrently()` — the
`concurrent_ref` decrement that `ConcurrentCppTask` (WebCrypto) needs after
its body ran on the pool thread, without dereferencing the VM.

## Abandon path

On `post_concurrent_task` → `false` the target VM and its JSC heap are gone (or
about to be). The abandon path:

- **must not** touch `Strong`/`Weak`/`JSPromiseStrong`/`JSGlobalObject`/
`VirtualMachine`/`KeepAlive::unref` — the HandleSet is freed, the loop is
freed;
- **may** free any pure-Rust heap it owns (body buffers, `Vec`s, `Box<[u8]>`);
- **must** free a freshly heap-allocated `ConcurrentTask` node (ownership was
not transferred);
- **may** leak the job box when it holds JSC handles. Bounded: one per
terminated worker per in-flight op.

## Coverage

Three generic helpers carry most of the surface:

| helper | users |
| -------------------------- | ----------------------------------------------------------- |
| `WorkTask<C>` | `ReadFile`, `WriteFile`, `GetAddrInfoRequest` |
| `ConcurrentPromiseTask<C>` | `CopyFile`, `TransformTask`, `WalkTask`, `PipelineTask` |
| `AnyTaskJob<C>` | `Pbkdf2Ctx`, `CryptoJob<Scrypt/…>`, `ZstdCtx`, `SecretsCtx` |
Comment thread
robobun marked this conversation as resolved.
| `ConcurrentCppTask` | WebCrypto (`PhonyWorkQueue::dispatch`) |

Direct callers converted alongside: `FetchTasklet`, `PasswordJob`,
`CompressionStream` (zlib/brotli/zstd), `AsyncFSTask` / `NewAsyncCpTask` /
`AsyncReaddirRecursiveTask`, `S3HttpSimpleTask` / `S3HttpDownloadStreamingTask`,
`Archive::AsyncTask`, `JSBundleCompletionTask`, `TranspilerJob`.

Same enqueue shape, deferred to follow-up (not in the verify harness):
`napi_async_work` / `ThreadSafeFunction`, `fs.watch` / `fs.watchFile`
(PathWatcherManager reader thread), the shell WorkPool tasks, `AsyncModule`
package-manager wake, and the Windows-only `WriteFileWindows` / `CopyFileWindows`
`AsyncMkdirp` completion (the POSIX `WriteFile`/`CopyFile` paths above go
through the converted `WorkTask`/`ConcurrentPromiseTask`).

Explicitly **not** enqueue-shaped and left for their own fixes: nested-worker
child-init reading a freed parent VM, `node:quic` finalizer ordering,
`RedisClient::finalize` free-then-read, `Bun.SQL` handle crashes during
terminating-VM JS execution, the `serve.listen`/JS-re-entry assert zone.

## Reserve alternative (not taken)

Refcount-deferred VM dealloc + a closed-flag at the funnel: each off-thread job
takes an `Arc` clone of a per-VM gate and brackets its enqueue with a read
lock; `shutdown` takes the write lock before dealloc. Same sweep, but:

- `shutdown` then _waits_ on in-flight pool jobs (a slow argon2/RSA can stall
terminate for seconds);
- more atomics on the hot enqueue path;
- the gate must be `Arc`'d so it outlives the VM box anyway.

The identifier route reuses an existing lock, adds no wait, and matches what
the C++ side already does.

## Post-fix gate

`repro/rootB-verify/verify.mjs`: one worker per iteration arms one in-flight op
of every cross-thread source above (self-contained, loopback-only, public API),
the parent `terminate()`s mid-flight, ×100. PASS = rc 0, `ROOT-B VERIFY: PASS`,
zero ASan/assert/panic. Baseline on current canary: SIGSEGV on teardown 1
(release), heap-use-after-free on teardown 1-3 (debug+ASAN).
45 changes: 45 additions & 0 deletions repro/rootB-verify/verify.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Root-B post-fix gate: one worker per iteration arms one in-flight op of
// every cross-thread completion source (public API, loopback-only), parent
// terminates mid-flight, x ITERATIONS. PASS = rc 0 + the PASS line + zero
// ASan/assert/panic. On stock canary this SIGSEGVs on the first teardown.
//
// Run under the debug+ASAN build so the fail-before is deterministic:
// bun bd repro/rootB-verify/verify.mjs

Comment thread
robobun marked this conversation as resolved.
import { Worker } from "node:worker_threads";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";

const ITERATIONS = Number(process.env.ROOTB_ITER ?? 100);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const body = new URL("./worker-body.mjs", import.meta.url);
const scratch = fs.mkdtempSync(path.join(os.tmpdir(), "rootB-verify-"));
process.env.ROOTB_SCRATCH = scratch;
process.on("exit", () => fs.rmSync(scratch, { recursive: true, force: true }));

for (let i = 0; i < ITERATIONS; i++) {
const w = new Worker(body);
// Wait for the worker to finish arming (or give up after a bound).
const armed = await new Promise((resolve) => {
const t = setTimeout(() => resolve("timeout"), 5000);
w.once("message", (m) => {
clearTimeout(t);
resolve(m);
});
w.once("error", (e) => {
clearTimeout(t);
resolve(e);
});
});
if (armed instanceof Error) {
console.error(`iter ${i}: worker error before terminate:`, armed);
process.exit(1);
}
// Small jitter so terminate lands at varying points in the in-flight work.
const jitter = (i * 2654435761 >>> 0) % 5;
if (jitter) await new Promise((r) => setTimeout(r, jitter));
await w.terminate();
if (i % 10 === 9) console.log(`… ${i + 1}/${ITERATIONS} teardowns clean`);
}

console.log(`ROOT-B VERIFY: PASS (${ITERATIONS} teardowns)`);
107 changes: 107 additions & 0 deletions repro/rootB-verify/worker-body.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// One worker instance: arm one in-flight op of every cross-thread completion
// source, then sit. Parent terminates us mid-flight. Every op is self-contained
// and loopback-only (no public internet). Catch-and-ignore everywhere: the
// point is to have work airborne on a process-global thread when the VM dies,
// not to observe the result.

import { parentPort, threadId } from "node:worker_threads";
import fs from "node:fs";
import fsp from "node:fs/promises";
import zlib from "node:zlib";
import crypto from "node:crypto";
import dns from "node:dns/promises";
import os from "node:os";
import path from "node:path";

const sink = () => {};
const swallow = (p) => Promise.resolve(p).then(sink, sink);

// One scratch root for all iterations (verify.mjs removes it on exit).
const root = process.env.ROOTB_SCRATCH ?? path.join(os.tmpdir(), "rootB-verify");
const tmp = path.join(root, String(threadId));
fs.mkdirSync(tmp, { recursive: true });
const tmpFile = path.join(tmp, "a.txt");
fs.writeFileSync(tmpFile, Buffer.alloc(1 << 16, "x").toString());

// fetch / HTMLRewriter / TLS (HTTP thread -> FetchTasklet)
{
const srv = Bun.serve({
port: 0,
fetch: () => new Response("x".repeat(1 << 14)),
});
swallow(fetch(`http://127.0.0.1:${srv.port}/`).then((r) => r.text()));
swallow(
fetch(`http://127.0.0.1:${srv.port}/`).then((r) =>
new HTMLRewriter().on("*", { text() {} }).transform(r).text(),
),
);
}

// Bun.file / Bun.write (WorkPool -> WorkTask<WriteFile>/<ReadFile>)
swallow(Bun.write(path.join(tmp, "b.txt"), "y".repeat(1 << 16)));
swallow(Bun.file(tmpFile).text());

// node:fs promises (WorkPool -> AsyncFSTask)
swallow(fsp.readFile(tmpFile));
swallow(fsp.stat(tmpFile));
swallow(fsp.readdir(tmp, { recursive: true }));

// pbkdf2 / scrypt / generateKeyPair (WorkPool -> AnyTaskJob)
crypto.pbkdf2("p", "s", 100000, 64, "sha512", sink);
crypto.scrypt("p", "saltsalt", 64, sink);
crypto.generateKeyPair("rsa", { modulusLength: 2048 }, sink);

// Bun.password (WorkPool -> PasswordJob)
swallow(Bun.password.hash("hunter2", { algorithm: "bcrypt", cost: 8 }));

// zlib (WorkPool -> CompressionStream async_job_run)
zlib.deflate(Buffer.alloc(1 << 18), sink);
zlib.gzip(Buffer.alloc(1 << 18), sink);

// S3 (HTTP thread -> S3HttpSimpleTask); loopback endpoint that never answers.
{
const srv = Bun.serve({ port: 0, fetch: () => new Promise(sink) });
const s3 = new Bun.S3Client({
accessKeyId: "x",
secretAccessKey: "y",
endpoint: `http://127.0.0.1:${srv.port}`,
bucket: "b",
});
swallow(s3.file("k").text());
}

// Bun.build (BundleThread -> JSBundleCompletionTask)
{
const entry = path.join(tmp, "entry.ts");
fs.writeFileSync(entry, `export const x: number = 1;\n`);
swallow(Bun.build({ entrypoints: [entry], target: "bun" }));
}

// Transpiler.transform (WorkPool -> ConcurrentPromiseTask<TransformTask>)
swallow(new Bun.Transpiler({ loader: "tsx" }).transform("const x: number = 1;"));

// dns.lookup (c-ares; clean-by-construction via close_dns_for_terminate)
swallow(dns.lookup("localhost"));

// WebCrypto (WorkPool -> ConcurrentCppTask)
swallow(
crypto.subtle.digest("SHA-256", new Uint8Array(1 << 16)),
);
swallow(
crypto.subtle.generateKey({ name: "AES-GCM", length: 256 }, true, [
"encrypt",
"decrypt",
]),
);

// Glob (WorkPool -> ConcurrentPromiseTask<WalkTask>)
swallow(Array.fromAsync(new Bun.Glob("**/*").scan(tmp)));

// zstd (WorkPool -> AnyTaskJob<ZstdCtx>)
swallow(Bun.zstdCompress(Buffer.alloc(1 << 16)));

// Signal parent that everything is airborne.
parentPort.postMessage("armed");

// Keep the loop alive so terminate() lands mid-flight.
setInterval(sink, 1 << 30);
15 changes: 15 additions & 0 deletions src/event_loop/ConcurrentTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,21 @@ impl ConcurrentTask {
Self::create(ManagedTask::ManagedTask::new(ptr, callback))
}

/// Reclaim a node produced by [`Self::from_callback`] that was never
/// enqueued. Frees both the outer `ConcurrentTask` and the inner
/// `ManagedTask` box; does not call the callback.
///
/// # Safety
/// `node` must be a `from_callback`-allocated node whose ownership was not
/// transferred to a queue.
Comment thread
robobun marked this conversation as resolved.
pub unsafe fn destroy_from_callback(node: core::ptr::NonNull<ConcurrentTask>) {
// SAFETY: caller contract — `from_callback` wraps a heap `ManagedTask`.
let outer = unsafe { bun_core::heap::take(node.as_ptr()) };
debug_assert!(outer.task.tag == task_tag::ManagedTask);
// SAFETY: `ManagedTask::new` produced the inner box via `heap::into_raw`.
drop(unsafe { bun_core::heap::take(outer.task.ptr.cast::<ManagedTask::ManagedTask>()) });
}

pub fn from<T: Taskable>(
&mut self,
of: *mut T,
Expand Down
21 changes: 7 additions & 14 deletions src/jsc/ConcurrentPromiseTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ use bun_event_loop::ConcurrentTask::{AutoDeinit, ConcurrentTask, TaskTag, Taskab
use bun_io::{self as Async, KeepAlive};
use bun_threading::{IntrusiveWorkTask as _, WorkPoolTask, work_pool::WorkPool};

use crate::event_loop::EventLoop;
use crate::js_global_object::ScriptExecutionContextIdentifier;
use crate::js_promise::{JSPromise, Strong as JSPromiseStrong};
use crate::virtual_machine::VirtualMachine;
use crate::{JSGlobalObject, JsTerminated};
use bun_ptr::BackRef;

/// The `Context` type parameter for [`ConcurrentPromiseTask`] must implement this trait:
/// - `run(&mut self)` — performs the work on the thread pool
Expand All @@ -31,9 +29,8 @@ pub struct ConcurrentPromiseTask<'a, Context: ConcurrentPromiseTaskContext> {
// Owned here so dropping the task frees the context.
pub ctx: Box<Context>,
pub task: WorkPoolTask,
/// BACKREF — captured from the JS-thread VM at create time; the VM (and its
/// `EventLoop`) outlives every task scheduled on it.
pub event_loop: BackRef<EventLoop>,
/// See [`ScriptExecutionContextIdentifier::post_concurrent_task`].
pub context_id: ScriptExecutionContextIdentifier,
pub promise: JSPromiseStrong,
pub global_this: &'a JSGlobalObject,
pub concurrent_task: ConcurrentTask,
Expand All @@ -57,11 +54,8 @@ impl<Context: ConcurrentPromiseTaskContext> Taskable for ConcurrentPromiseTask<'

impl<'a, Context: ConcurrentPromiseTaskContext> ConcurrentPromiseTask<'a, Context> {
pub fn create_on_js_thread(global_this: &'a JSGlobalObject, value: Box<Context>) -> Box<Self> {
// `VirtualMachine::get()` returns the JS-thread singleton; the VM and
// its `EventLoop` outlive every task scheduled on it.
let event_loop = BackRef::new(VirtualMachine::get().as_mut().event_loop_shared());
let mut this = Box::new(Self {
event_loop,
context_id: global_this.script_execution_context_identifier(),
ctx: value,
task: WorkPoolTask {
node: Default::default(),
Expand Down Expand Up @@ -108,15 +102,14 @@ impl<'a, Context: ConcurrentPromiseTaskContext> ConcurrentPromiseTask<'a, Contex
// `this` while holding `&mut *this` is sound because `from` only stores
// the pointer (does not dereference it).
let this_ref = unsafe { &mut *this };
let event_loop = this_ref.event_loop;
let context_id = this_ref.context_id;
let task = core::ptr::NonNull::from(
this_ref
.concurrent_task
.from(this, AutoDeinit::ManualDeinit),
);
// `task` is the live `concurrent_task` field of the heap-allocated
// job; the queue takes ownership of its intrusive `next` link.
event_loop.enqueue_task_concurrent(task);
// Abandon: JSC handles cannot drop off-thread, leak the box (task is intrusive).
let _ = context_id.post_concurrent_task(task);
}

/// Frees the heap allocation backing this task.
Expand Down
Loading
Loading