Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
15 changes: 15 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1595,6 +1595,13 @@ impl VirtualMachine {
.unwrap()
.close_all_socket_groups(vm_ref);
}
// Destroy the per-VM c-ares channel while JSC / `RareData.file_polls`
// / `runtime_state` are all still live — `ares_destroy()` re-enters
// them from its EDESTRUCTION and socket-state callbacks. Mirrors
// `WebWorker::shutdown`.
if let Some(hooks) = runtime_hooks() {
(hooks.close_dns_for_terminate)();
}

// The HTTP daemon thread holds a `Box<ThreadlocalAsyncHTTP>` per
// in-flight request; with the JS thread exiting those never reach
Expand Down Expand Up @@ -1825,6 +1832,14 @@ pub struct RuntimeHooks {
/// `vm` is the live per-thread VM; `runtime_state` must still be installed
/// and the JSC heap must not have been swept yet.
pub cancel_all_timers: unsafe fn(vm: *mut VirtualMachine),
/// Destroy the per-VM global DNS resolver's c-ares channel now, while JSC,
/// the event loop, `RareData.file_polls`, and `runtime_state` are all
/// live. `ares_destroy()` re-enters the resolver's socket-state and query
/// callbacks; deferring it to `deinit_runtime_state`'s `RuntimeState` drop
/// runs those callbacks against freed state. No-op when the resolver was
/// never lazily created. Called from `WebWorker::shutdown` / `global_exit`
/// right after `close_all_socket_groups`.
pub close_dns_for_terminate: fn(),
}

/// Canonical `EventLoopCtx` vtable for a `*mut VirtualMachine` owner — the JS
Expand Down
13 changes: 13 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,19 @@ impl WebWorker {
// is step 3 below).
rare.close_all_socket_groups(unsafe { &*vm_ptr });
}
// Destroy the per-VM c-ares channel now: `ares_destroy()` fires
// every pending query callback with `ARES_EDESTRUCTION` and then
// the socket-state callback for each fd it closes, both of which
// dereference state (`JSGlobalObject`, `RareData.file_polls`,
// `runtime_state().timer`) that step 3/5 below free. Deferring it
// to `destroy()`'s `deinit_runtime_state` is a UAF. Must FOLLOW
// `close_all_socket_groups`: its on_close JS can call
// `dns.resolve*()`, and `Resolver::get_channel()` lazily re-inits
// on `channel == None` — running this earlier lets a re-created
// channel survive to `GlobalData::drop` (the original UAF).
if let Some(hooks) = runtime_hooks() {
(hooks.close_dns_for_terminate)();
}
// Stop cross-thread posters first: markTerminating() serializes
// with postTaskTo() on the contexts-map lock, so after this call
// every task another thread has already enqueued is visible to the
Expand Down
14 changes: 11 additions & 3 deletions src/runtime/dns_jsc/cares_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,16 +718,24 @@ impl ErrorDeferred {
}
}

let vm = global_this.bun_vm();
// Worker terminate's `close_dns_for_terminate` fires EDESTRUCTION with
// `is_shutting_down` already set; the task queue is about to be
// drained-without-run and ManagedTask has no cleanup here, so enqueuing
// would leak the `Context` and its `JSPromiseStrong` box. Drop now while
// JSC is still live so the Strong handle releases cleanly.
if vm.is_shutting_down() {
return;
}

let context = bun_core::heap::into_raw(Box::new(Context {
deferred: self,
global_this: bun_ptr::BackRef::new(global_this),
}));
// TODO(@heimskr): new custom Task type
// SAFETY: `bun_vm()` returns a non-null VM pointer (VM-owned for the lifetime of
// the JSGlobalObject).
global_this
.bun_vm()
.as_mut()
vm.as_mut()
.enqueue_task(bun_jsc::ManagedTask::ManagedTask::new(
context,
Context::callback,
Expand Down
21 changes: 21 additions & 0 deletions src/runtime/dns_jsc/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2138,6 +2138,27 @@
}
}

impl Resolver {
/// Worker-terminate / main-VM-destruct hook: tear down the c-ares channel
/// while the JSC VM, `RareData.file_polls`, event loop, and `runtime_state`
/// are all still live. `ares_destroy()` synchronously fires every pending
/// query callback with `ARES_EDESTRUCTION` and then the socket-state
/// callback for each fd it closes; those callback chains dereference
/// `DNSLookup::global_this` (to enqueue the rejection task) and the hive
/// `FilePoll` (to unregister it from the loop). Running this after either
/// is freed is a UAF (Node `test-worker-dns-terminate.js`).
pub fn close_channel_for_terminate(&self) {
if let Some(channel) = self.channel.take() {
// SAFETY: `channel` is the live handle from `ares_init_options`, owned by this resolver.
unsafe { c_ares::Channel::destroy(channel) };
}
// `GetAddrInfoRequest`'s EDESTRUCTION path does not call
// `request_completed()`, so the c-ares timeout timer (and its +1 ref on
// this resolver plus the uws active-handle bump) can still be linked.
self.remove_timer();

Check warning on line 2158 in src/runtime/dns_jsc/dns.rs

View check run for this annotation

Claude / Claude Code Review

Stale SAFETY proof in remove_timer() after adding close_channel_for_terminate caller

nit: `close_channel_for_terminate()` calls `self.remove_timer()` directly, but the SAFETY comment inside `remove_timer()`'s `scopeguard::defer!` (dns.rs:4222-4225) still says "all callers of `request_completed` (**the only path here**) hold an `IntrusiveRc<Resolver>`". That clause is now factually false — the invariant *is* still upheld on the new path, but by the permanent `+1` pin from `global_resolver()`'s `get_or_init` (dns.rs:79 `gd.resolver.ref_()`), not by the reason the comment states. W
Comment thread
robobun marked this conversation as resolved.
}
Comment thread
robobun marked this conversation as resolved.
}

// ──────────────────────────────────────────────────────────────────────────
// internal — process-wide DNS cache used by usockets connect path
// ──────────────────────────────────────────────────────────────────────────
Expand Down
17 changes: 17 additions & 0 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1486,6 +1486,7 @@ pub(crate) static __BUN_RUNTIME_HOOKS: RuntimeHooks = RuntimeHooks {
terminate_all_workers_and_wait,
retroactively_report_discovered_tests,
cancel_all_timers,
close_dns_for_terminate,
};

// ════════════════════════════════════════════════════════════════════════════
Expand Down Expand Up @@ -1619,6 +1620,22 @@ unsafe fn cancel_all_timers(vm: *mut VirtualMachine) {
}
}

/// `RuntimeHooks::close_dns_for_terminate` — destroy the per-VM global DNS
/// resolver's c-ares channel now so its `ARES_EDESTRUCTION` and socket-state
/// callbacks run while the JSC VM, `RareData.file_polls`, and `runtime_state`
/// are all still live. See `Resolver::close_channel_for_terminate`.
fn close_dns_for_terminate() {
let state = runtime_state();
if state.is_null() {
return;
}
// SAFETY: `state` is the live per-thread `RuntimeState` box; shared borrow
// of the `OnceCell` only (the resolver's own state is interior-mutable).
if let Some(gd) = unsafe { &(*state).global_dns_data }.get() {
gd.resolver.close_channel_for_terminate();
}
}
Comment thread
robobun marked this conversation as resolved.

pub(crate) fn close_isolation_handles(vm: &mut VirtualMachine) {
let state = runtime_state();
if state.is_null() {
Expand Down
56 changes: 56 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isDebug } from "harness";
import { join } from "path";

// Worker VM startup/teardown is much slower under debug and/or ASAN; these
// tests spawn many workers, so scale iteration counts and timeouts down.
Expand Down Expand Up @@ -119,3 +120,58 @@
},
timeout,
);

// Regression: the per-VM c-ares channel was destroyed in deinit_runtime_state
// (RuntimeState drop) AFTER JSC teardown and RareData.file_polls drop.
// ares_destroy() synchronously fires EDESTRUCTION query callbacks and socket-
// state callbacks, which then dereferenced the freed JSGlobalObject and the
// freed FilePoll hive. ASAN-only: release builds read freed memory without
// crashing. Upstream Node test-worker-dns-terminate.js.
test.skipIf(!isASAN)(
"terminate() while dns.lookup() is in flight does not UAF on c-ares channel teardown",
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const { Worker } = require("worker_threads");
let done = 0;
for (let i = 0; i < 4; i++) {
const w = new Worker(
// Hermetic: point the global resolver's c-ares channel at a local
// UDP socket that never replies, so both queries are guaranteed
// in-flight (socket registered, no completion) when terminate()
// lands. In bun, dns.lookup() uses the c-ares backend on Linux
// and so also respects setServers().
'const dgram = require("dgram");' +
'const dns = require("dns");' +
'const s = dgram.createSocket("udp4");' +
's.bind(0, "127.0.0.1", () => {' +

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

View check run for this annotation

Claude / Claude Code Review

dns.lookup() still hits live network on macOS ASAN builds (System backend ignores setServers)

nit: the `dns.lookup("example.org")` line still hits the live system resolver on macOS ASAN builds — `Backend::default()` there is `System` (`src/dns/lib.rs:276-279`), which routes to Apple's `getaddrinfo_async_start` (`src/runtime/dns_jsc/dns.rs:5265-5268`) and ignores `dns.setServers()`, and `bun bd` on arm64 macOS is ASAN-instrumented by default so this test runs there. `dns.resolve4()` stays hermetic (c-ares) and still exercises the fix, so this is false-pass-only; simplest fix is to gate th
Comment thread
robobun marked this conversation as resolved.
' dns.setServers(["127.0.0.1:" + s.address().port]);' +
' dns.lookup("example.org", () => {});' +
' dns.resolve4("example.org", () => {});' +
' require("worker_threads").parentPort.postMessage(0);' +
'});',
{ eval: true },
);
w.on("message", () => w.terminate().then(() => {
if (++done === 4) console.log("ok");
}));
}
`,
],
env: {
...bunEnv,
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"),
LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`,
},
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