Skip to content
Merged
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
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
9 changes: 9 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,15 @@ 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.
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
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();
Comment thread
robobun marked this conversation as resolved.
}

Check failure on line 2159 in src/runtime/dns_jsc/dns.rs

View check run for this annotation

Claude / Claude Code Review

EDESTRUCTION callbacks enqueue reject_later ManagedTasks whose ctx leaks at shutdown

Running `ares_destroy()` while the VM is live means each in-flight query's `ARES_EDESTRUCTION` callback now reaches `reject_later(global_this)`, which heap-allocates a `Box<Context>` and enqueues it via `ManagedTask::new` (`cleanup: None`). That task never runs — `release_queued_tasks_for_shutdown` re-queues `ManagedTask` entries and `EventLoop::deinit` only frees `ctx` when `cleanup` is `Some` — so the `Context` / `ErrorDeferred` / hostname / `JSPromiseStrong` slot are orphaned per in-flight qu
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 @@
terminate_all_workers_and_wait,
retroactively_report_discovered_tests,
cancel_all_timers,
close_dns_for_terminate,
};

// ════════════════════════════════════════════════════════════════════════════
Expand Down Expand Up @@ -1619,6 +1620,22 @@
}
}

/// `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();
}
}

Check warning on line 1637 in src/runtime/jsc_hooks.rs

View check run for this annotation

Claude / Claude Code Review

close_dns_for_terminate only closes the global resolver, not per-instance new dns.Resolver() channels

This hook only reaches `(*state).global_dns_data` — per-instance `new dns.Resolver()` channels (each with their own `c_ares::Channel`) are not touched, so a worker terminated with an in-flight `r.resolve4()` leaks the resolver, its channel/fds, the heap request, and its `JSPromiseStrong`. This is a pre-existing leak (not the UAF fixed here) and would need a per-VM registry of live resolvers to close, so a follow-up is fine — just noting the gap since the PR description cites Node's `CleanupHandl
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
43 changes: 43 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,46 @@
},
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(
'const dns = require("dns");' +
'dns.lookup("nonexistent.org", () => {});' +
'dns.resolve4("nonexistent.org", () => {});' +
Comment thread
robobun marked this conversation as resolved.
Outdated
'require("worker_threads").parentPort.postMessage(0);',
{ eval: true },
);
w.on("message", () => w.terminate().then(() => {
if (++done === 4) console.log("ok");
}));
}
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("ok\n");
expect(exitCode).toBe(0);

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

View check run for this annotation

Claude / Claude Code Review

ASAN-only test asserts stderr is exactly empty

nit: REVIEW.md's subprocess-tests rule says "Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined `{ stdout, stderr, exitCode }` object" — and this test runs *only* under ASAN. The three sibling tests in this file use the same shape and don't flake today, so this is non-blocking, but `expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 })` matches the repo rule and gives a much better failure diff (you see the ASAN re
Comment thread
robobun marked this conversation as resolved.
Outdated
},
timeout,
);
Loading