dns: destroy the per-VM c-ares channel before JSC/RareData teardown on worker terminate - #34455
Conversation
…n worker terminate ares_destroy() synchronously invokes every pending query callback with ARES_EDESTRUCTION and then the socket-state callback for each fd it closes. On worker terminate the channel was destroyed in GlobalData::drop during deinit_runtime_state, which runs after WebWorker__teardownJSCVM (JSGlobalObject freed) and after drop(rare_data) (the FilePoll hive freed). The socket-state callback then called FilePoll::deinit_with_vm on a freed hive slot (ASAN heap-use-after-free), and the query callbacks dereferenced the freed global and a nulled runtime_state(). Add a RuntimeHooks::close_dns_for_terminate slot that tears the channel down in WebWorker::shutdown (and the BUN_DESTRUCT_VM_ON_EXIT main-thread path) right after close_all_socket_groups, while everything the callbacks touch is still live. Resolver::close_channel_for_terminate also removes the resolver's c-ares timeout timer, which GetAddrInfoRequest's EDESTRUCTION path never unwinds. Surfaced by Node's upstream test/parallel/test-worker-dns-terminate.js.
|
Updated 3:28 AM PT - Jul 17th, 2026
❌ @robobun, your commit 2e68559 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34455That installs a local version of the PR into your bun-34455 --bun |
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe change adds a runtime hook that closes each VM’s c-ares channel during VM and worker shutdown, removes its timer, prevents deferred DNS rejection enqueueing during shutdown, and adds ASAN-only worker termination coverage. ChangesDNS termination teardown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the new test's dns.lookup("nonexistent.org") / dns.resolve4(...) violate the hermetic-tests rule — the queries are terminated mid-flight and the test asserts nothing about their result, so it doesn't depend on external resolution (matches upstream Node's test-worker-dns-terminate.js shape).
Extended reasoning...
The inline findings already cover what needs attention here — in particular the reject_later → ManagedTask::new(cleanup: None) leak on the EDESTRUCTION path, which is newly reachable now that ares_destroy() runs while the VM is live. This is a worker-shutdown-ordering / memory-safety change and warrants a human look regardless.
reject_later() enqueued a ManagedTask with cleanup=None; on the worker terminate path the task queue is drained-without-run so the Context box (and its JSPromiseStrong) leaked per in-flight query. Check vm.is_shutting_down() and drop the ErrorDeferred inline instead, while JSC is still live so the Strong handle releases cleanly. Also switch the new test's assertion to the combined-object shape.
…t_groups; opt the test into LSan The hook's placement after close_all_socket_groups is load-bearing: on_close JS can call dns.resolve*() and Resolver::get_channel() lazily re-inits when channel is None, so running the hook earlier lets a re-created channel survive to GlobalData::drop (the original UAF). Also opt the new test into detect_leaks=1 with the repo suppression file so it catches the EDESTRUCTION-path reject_later leak class regardless of the runner's ambient ASAN_OPTIONS.
|
Status: UAF fix verified locally (fail-before against main's Follow-up commits address review feedback:
CI (build 74482, complete: 284/286 passed): the diff is green; the new test passed on every ASAN lane. The two red lanes are unrelated to this change:
Both are tracked separately. Ready for maintainer review. |
Point the global resolver's c-ares channel at a local UDP socket that never replies, so both dns.lookup() and dns.resolve4() are guaranteed in-flight when terminate() lands regardless of the runner's DNS cache speed, and no external DNS traffic is sent.
…okup to Linux in the hermetic test remove_timer()'s SAFETY comment said request_completed was the only path; close_channel_for_terminate is now a second caller. The invariant still holds via the global resolver's permanent +1 pin from global_resolver(); state that in the proof. On macOS dns.lookup() uses libinfo (ignores setServers()), so gate it to Linux where the c-ares backend is the default. dns.resolve4() stays hermetic everywhere and covers the same callback chains.
There was a problem hiding this comment.
No new findings — all prior feedback (the reject_later leak, hermetic test, SAFETY proof, macOS dns.lookup gate) is addressed and resolved. This reorders worker-VM teardown around ares_destroy() re-entry and adds an is_shutting_down gate on a JS-visible rejection path, so it's worth a maintainer's final look at the shutdown-step ordering invariants.
What was reviewed:
- Placement of
close_dns_for_terminateafterclose_all_socket_groupsand before JSC teardown in bothWebWorker::shutdownandglobal_exit— the re-init hazard from on_close JS is documented and the ordering matches. reject_later's newis_shutting_downearly return — dropsErrorDeferred/JSPromiseStrongwhile JSC is live; checked that other callers reaching this during shutdown (e.g.on_exit()user handlers) are also safe to short-circuit.remove_timer()'s updated SAFETY proof for the new direct caller — the global resolver's +1 pin keeps the deref from reaching 0.- The hermetic ASAN/LSan test: local UDP never-replies socket, Linux-gated
dns.lookup, combined-object assertion,leaksan.supppath.
Extended reasoning...
Overview
The PR fixes a heap-use-after-free on worker terminate when a dns.lookup()/dns.resolve*() is in flight: ares_destroy() was firing its EDESTRUCTION and socket-state callbacks from GlobalData::drop in deinit_runtime_state, after JSC and RareData.file_polls were already freed. The fix adds a RuntimeHooks::close_dns_for_terminate slot invoked from WebWorker::shutdown (and the BUN_DESTRUCT_VM_ON_EXIT main-thread path) right after close_all_socket_groups, while all dependent state is still live. Follow-up commits addressed a task-leak on the now-reachable EDESTRUCTION rejection path (reject_later drops inline when is_shutting_down), made the regression test hermetic (local never-replying UDP socket via dns.setServers), gated dns.lookup to Linux, and updated remove_timer()'s SAFETY proof for the new caller.
Security risks
None identified. This is a lifetime/teardown-ordering fix in the worker shutdown path; no user-facing API surface, parsing, or trust-boundary changes.
Level of scrutiny
High. Worker VM teardown ordering is one of the most invariant-dense parts of the runtime — the shutdown() doc block enumerates five ordered barriers, and this PR inserts a new step whose callbacks re-enter three of them (JSGlobalObject, the FilePoll hive, runtime_state().timer). The placement (after close_all_socket_groups, before markTerminating/JSC teardown) is documented in-line with the specific hazard (on_close JS lazily re-initing the channel), and the GlobalData::drop path remains as a fallback. The is_shutting_down gate in reject_later also affects any DNS error deferred during on_exit() user handlers on both worker and main-thread shutdown; dropping the Strong handle inline there is strictly safer than the prior enqueue-and-orphan, but it's a behavior change on a shared helper.
Other factors
Three prior review rounds; every inline finding is resolved. The ASAN-gated test opts into LSan with the repo's leaksan.supp, uses the combined-object assertion, and was verified fail-before/pass-after. rust:check-all reported clean across targets. The one acknowledged gap (per-instance new dns.Resolver() channels leak on terminate) is pre-existing, a different failure mode, and needs a per-VM registry to fix — reasonably deferred. Given the subtlety of the teardown-ordering contract and the new RuntimeHooks surface, deferring to a maintainer for the final sign-off rather than auto-approving.
2 participants
What
Fixes a heap-use-after-free when a Worker with an in-flight
dns.lookup()/dns.resolve*()is terminated.Surfaced by Node's upstream
test/parallel/test-worker-dns-terminate.js(being vendored in #34441), on the debian 13 x64-asan lane:Repro
Cause
WebWorker::shutdown()runs, in order:WebWorker__teardownJSCVM(frees theJSGlobalObject), thenVirtualMachine::destroy()which dropsrare_data(frees theFilePollhiveStore) and finally callsdeinit_runtime_statewhich dropsRuntimeState. That last drop runsGlobalData::dropwhich callsares_destroy()on the per-VM c-ares channel.ares_destroy()synchronously fires every pending query callback withARES_EDESTRUCTIONand then the socket-state callback for each fd it closes. Those callback chains re-enter:Resolver::on_dns_socket_state->FilePoll::deinit_with_vmon the already-freed hive slot (the ASAN trace above)GetAddrInfoRequest::on_cares_complete->DNSLookup::process_get_addr_info->reject_later(global_this)on the freedJSGlobalObject(bmalloc-backed so ASAN misses it)ResolveInfoRequest::on_cares_complete->request_completed()->remove_timer()->(*runtime_state()).timerwith the TLS already nulled (null deref)Fix
Add a
RuntimeHooks::close_dns_for_terminateslot that runsResolver::close_channel_for_terminate()fromWebWorker::shutdown()(and theBUN_DESTRUCT_VM_ON_EXITmain-thread path) right afterclose_all_socket_groups, while JSC,RareData.file_polls, the event loop, andruntime_stateare all still live. The method also removes the resolver's c-ares timeout timer, whichGetAddrInfoRequest's EDESTRUCTION path never unwinds.GlobalData::dropstill handles the channel if the early hook never ran (it seeschannel == Nonewhen it did).This matches Node's model:
Worker::Exit->CleanupHandles()closes every handle wrap (includingChannelWrap) before disposing the Isolate.Verification
New ASAN-gated test in
test/js/web/workers/worker-terminate-lifetime.test.tsspawns four workers that each start adns.lookup()+dns.resolve4()and terminates them mid-flight.git stash -- src/ && bun bd test ...): null-deref panic / ASAN heap-use-after-freeAlso verified
test/js/node/dns/andtest/js/bun/dns/pass/fail counts are unchanged vs. main, andbun run rust:check-allis clean on all targets.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts