dns: carry c-ares reply ownership in OwnedReply<T> - #37602
Conversation
The parsed reply of a generic record query (ResolveInfoRequest<T: CAresRecordType>, 12 record types) travelled through on_cares_complete, CAresLookup::process_resolve and Resolver::drain_pending_cares as Option<*mut T>, and the two consumers re-established ownership by hand with scopeguard closures calling T::destroy, plus a comment explaining which caller passes null. It now travels as Option<OwnedReply<T>>, a repr(transparent) NonNull<T> wrapper whose Drop is the only caller of T::destroy; the four producers adopt the reply where the c-ares layer hands it over, and the consumers simply let it drop at the same scope exit where the guards fired. The reverse-lookup hostent, which c-ares only lends, keeps its raw pointer, so the owned and lent cases now differ in type rather than by convention. Option<OwnedReply<T>> is a single nullable pointer instead of a two-word Option<*mut T>, the null checks in the producers are the same NonNull::new checks as before, and the drop glue is the same T::destroy call the guards made.
|
Updated 2:04 PM PT - Aug 11th, 2026
✅ @robobun, your commit 72b9aaa30b3f4c972f1b6f006b00d87bb7745dea passed in 🧪 To try this PR locally: bunx bun-pr 37602That installs a local version of the PR into your bun-37602 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe DNS c-ares integration now uses DNS reply ownership
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.
LGTM — clean RAII wrapper that moves c-ares reply ownership from scopeguard-by-convention into the type system.
Checked that all exit paths in process_resolve and drain_pending_cares still free the reply: the early-return in the Some(err) branch drops the unmoved result parameter, and node/addr drop at the same scope end the old guards did. Verified T::destroy now has exactly one caller (the Drop impl), the four producers adopt at the c-ares handover point with correct SAFETY comments, and the borrowed GetHostByAddrInfoRequest hostent path is intentionally left as Option<*mut struct_hostent>. Confirmed heap::into_raw_nn exists and to_js_response(&mut self) resolves through DerefMut.
Extended reasoning...
Overview
Single-file refactor in src/runtime/dns_jsc/dns.rs (+71/-55). Introduces OwnedReply<T: CAresRecordType>, a #[repr(transparent)] NonNull<T> wrapper whose Drop calls T::destroy, and threads it through ResolveInfoRequest::on_cares_complete, CAresLookup::process_resolve, and Resolver::drain_pending_cares in place of Option<*mut T>. The four producer sites (impl_cares_record_type!, hostent_newtype!, hostent_ttls_newtype!, and the any handler) now adopt the reply where c-ares hands it over; the two consumer-side scopeguard calls are deleted.
Security risks
None. This is internal memory-ownership plumbing for DNS reply structs; no user-facing input handling, parsing, or trust boundary is touched.
Level of scrutiny
Moderate — it's memory-lifetime code, so I traced every exit path in both consumers. In process_resolve, the early return when err_.is_some() no longer has an explicit guard, but result: Option<OwnedReply<T>> is a by-value parameter and drops on return, so a (theoretical) Some(err) + Some(reply) case still frees. In drain_pending_cares, addr drops after the waiter loop, matching the old _free_addr guard's scope end. The None branch forwards None to process_resolve, so nothing to free there. T::destroy is now called from exactly one place (dns.rs:427), and the borrowed hostent path (GetHostByAddrInfoRequest, line 656) correctly retains its raw-pointer signature since c-ares owns that reply.
Other factors
The change is exactly what REVIEW.md's memory-safety guidance asks for: replace ad-hoc release-at-scope-exit with a Drop/RAII guard armed at the acquisition site. Option<NonNull<T>> niche-optimizes to a single nullable pointer, and panic = "abort" means no new unwind edges. Verified heap::into_raw_nn exists in bun_core::heap and that to_js_response takes &mut self so DerefMut dispatch works. PR description accurately notes the textual conflict with #36123. No behavior change, no test changes needed.
3 participants
What
For the 12 record types resolved through
ResolveInfoRequest<T: CAresRecordType>(srv, soa, txt, naptr, mx, caa, any, ns, ptr, cname, a, aaaa), the parsed reply travelled throughResolveInfoRequest::on_cares_complete,CAresLookup::process_resolveandResolver::drain_pending_caresasOption<*mut T>, and the two consumers re-established ownership by hand:process_resolvearmed ascopeguardcallingT::destroy, with a comment explaining that the cached path frees the reply itself and therefore "always passes null", anddrain_pending_caresarmed a second guard (_free_addr) after the firstto_js_responsecall.src/runtime/dns_jsc/dns.rsnow hasand the three signatures take
result: Option<OwnedReply<T>>. The 4 producers adopt the reply where the c-ares layer hands it over: theimpl_cares_record_type!andhostent_newtype!handlers replace theiris_null()checks withNonNull::new(..).map(adopt), and theanyandhostent_ttls_newtype!handlers, which receive aBoxfrom the parser, release it withheap::into_raw_nninto the same wrapper instead ofheap::into_raw. Both consumers lose their guards:process_resolvebindslet Some(mut node) = resultandnodedrops at the end of the function,drain_pending_caresbindslet Some(mut addr) = result, callsaddr.to_js_response(..)instead of(*addr).to_js_response(..)at its 2 sites, andaddrdrops after the waiter loop; theNoneit forwards toprocess_resolveon the no-reply path now needs no comment.CAresRecordType::destroyis unchanged per type and has exactly one caller, theDropimpl. The two ad hocT::destroysites are gone; the wrapper adds three one-lineunsafeblocks (deref,deref_mut,drop) and each of the 4 producers gains a one-lineunsafe { OwnedReply::adopt(..) }whose SAFETY comment states why that reply is ours to free. One file, +71/-55 lines.Why
Who frees a reply, and on which path, is now answered by the parameter type:
Option<OwnedReply<T>>is freed by whoever holds it, while the reverse-lookup hostent that c-ares only lends toGetHostByAddrInfoRequestkeeps itsOption<*mut struct_hostent>, so the owned and lent cases differ in type instead of by convention. A consumer can no longer forget to free a reply on one of its exit paths, and the free itself is written in one place instead of two. It is zero-cost:Option<OwnedReply<T>>is one nullable pointer (theNonNullniche) whereOption<*mut T>was two words, the producers'NonNull::newis the same null check they already performed,Box::into_rawandheap::into_raw_nncompile to the same thing, and the drop glue is the sameT::destroycall the guards made at the same scope exits (the workspace builds withpanic = "abort", so no unwind paths or drop flags are introduced).Part of a series of small type-system hardening changes; each PR stands alone.
Related
#36123 also removes these two
scopeguardsites, by wrapping theOption<*mut T>in aCAresReply<T>at the two consumer sites and leaving the signatures unchanged. This PR instead moves the ownership into the signatures and adopts the reply at the producers, so the two conflict textually inprocess_resolveanddrain_pending_cares; whichever lands second needs a rebase of that hunk.Verification
cargo checkandcargo clippyare clean for the touched crates. Debug build succeeds. test/js/node/dns/dns-resolver-concurrent-timeout.test.ts, dns-tcp-bidirectional-poll.test.ts, node-dns.test.js: 97 pass, 29 fail; the same 29 node-dns.test.js tests (resolveSrv/Txt/Soa/Naptr/Caa/Mx/Ns/Ptr/Cname against bun.sh/socketify.dev, lookup example.com, reverse/lookupService on 1.1.1.1/8.8.8.8) fail identically on main with ESERVFAIL/ETIMEOUT/ENOTFOUND because they need public internet DNS, so they are pre-existing and unrelated to this change.