Skip to content
Merged
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
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
29 changes: 26 additions & 3 deletions src/runtime/dns_jsc/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2138,6 +2138,27 @@ impl Drop for GlobalData {
}
}

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();
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 Expand Up @@ -4199,9 +4220,11 @@ impl Resolver {
let this = self.as_ctx_ptr();
scopeguard::defer! {
// SAFETY: `this` is the heap allocation from `init`. This releases the
// ref taken by `add_timer`; all callers of `request_completed` (the only
// path here) hold an `IntrusiveRc<Resolver>`, so the timer ref is never
// the last and this `deref` cannot reach 0 while `&self` is live.
// ref taken by `add_timer`. Callers via `request_completed` hold an
// `IntrusiveRc<Resolver>`; the `close_channel_for_terminate` caller is
// the global resolver, which carries a permanent +1 pin from
// `global_resolver()`. Either way the timer ref is never the last and
// this `deref` cannot reach 0 while `&self` is live.
unsafe {
let uws_loop = (*this).vm().uws_loop();
let state = crate::jsc_hooks::runtime_state();
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
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
@@ -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,59 @@ test(
},
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. dns.lookup() only respects setServers() where the c-ares
// backend is the default (Linux); elsewhere resolve4 alone still
// covers the socket-state and EDESTRUCTION paths hermetically.
'const dgram = require("dgram");' +
'const dns = require("dns");' +
'const s = dgram.createSocket("udp4");' +
's.bind(0, "127.0.0.1", () => {' +
Comment thread
robobun marked this conversation as resolved.
' dns.setServers(["127.0.0.1:" + s.address().port]);' +
' if (process.platform === "linux") 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