dns: pass c-ares requests to their handlers as *mut instead of &mut - #37801
dns: pass c-ares requests to their handlers as *mut instead of &mut#37801robobun wants to merge 3 commits into
Conversation
Every c-ares completion in dns.rs ends in on_cares_complete(this: *mut Self),
which reclaims the request's allocation (heap::take) on every path. The
handler traits in c_ares.rs handed the request to those impls as &mut self,
and Channel::{get_addr_info, resolve, get_host_by_addr, get_name_info} took
it as ctx: &mut T; c-ares (and resolve/get_host_by_addr themselves) complete
rejected inputs synchronously, so on those paths the allocation was freed
while both reference arguments were still live. Deallocating behind a
protected reference argument is undefined behaviour under Stacked and Tree
Borrows whether or not the reference is used again.
The request now travels as a raw pointer the whole way: the Channel methods
take *mut T, the thunks pass c-ares's ctx pointer through instead of forming
&mut T with callback_ctx, the trait methods take this: *mut Self, and the
seven dns.rs impls forward it to on_cares_complete unchanged. No operation
is added or reordered; only the type the pointer travels as changes.
test/internal/source-lints/cares-request-raw-ptr.test.ts keeps it that way:
it reports the seven on_cares_complete(from_mut(self)) sites, the six &mut
self handler methods and the four ctx: &mut T parameters on the previous
tree, and passes on this one.
WalkthroughChangesThe c-ares callback API now uses unsafe raw request pointers. DNS submission paths pass heap pointers directly. Runtime handlers preserve reply ownership. Tests, a source lint, and Miri coverage validate the ownership model. c-ares raw-pointer callback ownership
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review at a563990; waiting on CI infrastructure, not on changes. Since the first push: the self-review's one surviving point (the c_ares.rs half is checkable as a property, not by regex) is addressed in 3e8c649, which adds the Miri-driven unit tests to c_ares.rs, puts CI: the diff is green everywhere CI has been able to run it. Build 93068 on the first head passed 179/181 (the two darwin 26 aarch64 shards never got an agent). On a563990 every GitHub Actions check is green, including Reproduced as a source-level finding: with dns.rs at PR: #37801 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes unsafe trait signatures and SAFETY contracts across the c-ares FFI boundary and the DNS request lifecycle, a human look would still be worthwhile.
What was reviewed:
- Confirmed
dns.rsis the only implementor of the six handler traits and the only caller of the fourChannelregistration methods, so no site is left on the old signature. - Each converted thunk performs the same operations in the same order; only
callback_ctx::<T>(ctx)→ctx.cast::<T>()and the dispatch form change. - The shape matches the
ReadBytesHandler::on_read_bytesprecedent insrc/runtime/webcore/Blob.rs; the new source-lint follows the siblingself-receiver-reclaim.test.tsstructure and self-tests its patterns.
Extended reasoning...
Overview
This PR converts the c-ares request-completion path from &mut self / ctx: &mut T to *mut Self / ctx: *mut T at every frame between registration and on_cares_complete, fixing a Tree Borrows / Stacked Borrows protector violation (deallocating the request while a reference argument to it is live). It touches: six handler-trait signatures and seven extern "C" thunks in src/cares_sys/c_ares.rs, four Channel::{get_addr_info, resolve, get_host_by_addr, get_name_info} signatures, seven trait impls and four registration sites in src/runtime/dns_jsc/dns.rs, a doc addition to bun_core::callback_ctx, and a new source-lint test.
Security risks
None identified. This is a soundness fix that removes a latent aliasing-model UB (rustc dereferenceable on a reference argument that gets freed mid-call). No new attack surface, no input handling changes, no allocation-size arithmetic changes.
Level of scrutiny
High. This is unsafe Rust at an FFI boundary in a production-critical subsystem (DNS resolution). The transformation itself is mechanical — every thunk and impl does exactly what it did before, only the pointer type the request travels as changes — but signature changes to unsafe fn trait methods, new # Safety contracts, and the interaction with concurrent open PRs (#36186 adds a ReplyHandler impl, #35776 edits adjacent lines) warrant a human maintainer's confirmation that this fits the ongoing aliasing-model cleanup (#37681/#37685/#37693/#37705/#37716).
Other factors
The PR is exceptionally well-documented: Miri-under-Tree-Borrows verification of both frames, before/after lint output showing all 17 offenders, clippy/fmt/cross-target checks, and an ASAN exercise script covering all 22 converted paths including the synchronous-completion cases. Grep confirms dns.rs is the sole implementor of the six traits and sole caller of the four Channel methods, so the compiler enforces completeness. The new lint test mirrors the existing self-receiver-reclaim.test.ts structure (same glob helper, same tracked-file filter, same self-test-the-patterns approach) and guards against regression. No bugs were found by the automated bug-hunting pass. Given the unsafe-code scope and cross-PR coordination noted in the description, deferring rather than auto-approving.
|
Updated 3:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit a563990 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37801That installs a local version of the PR into your bun-37801 --bun |
Adds a #[cfg(test)] module to c_ares.rs with a request that reclaims itself on completion, delivered through each of the nine thunks with an error status and through Channel::resolve (overlong name) and Channel::get_host_by_addr (unparsable address), the two registration frames that complete a request without reaching c-ares (Channel is a ZST, so no channel is needed). None of these paths call a foreign function, so the crate joins MIRI_CRATES; against the previous c_ares.rs all eight tests fail under Tree Borrows with "the strongly protected tag disallows deallocations", pointing at the &mut self handler methods, and with only the traits converted the two Channel tests still fail, pointing at ctx: &mut T. With the c_ares.rs frames checked as a property, the source lint shrinks to the half Miri cannot reach: the dns.rs impls must forward `this` to on_cares_complete rather than spell the receiver's address. Renamed to match the self-receiver-* lints it belongs with.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues. The &mut self → *mut Self conversion is mechanically consistent across all six handler traits, nine thunks, four Channel registration methods, and seven dns.rs impls, and the Miri tests plus source lint cover the invariant well. Because this changes the safety contract of every c-ares handler trait and touches memory-lifetime code in the DNS FFI path, a human look is still worthwhile — in particular to weigh the ~18 comment-cop flags, which land on required # Safety docs and SAFETY comments rather than workaround prose and read as false positives to me.
What was reviewed:
- All nine thunks and four
Channel::*methods passctxthrough as a raw pointer with no reference formed before the handler runs; each handler-call site is the last use ofctx. - Confirmed the four dns.rs registration sites are the only callers of the now-
unsafeChannelmethods, and the seven impls are the only implementors of the changed traits. - The two
Channeltests take only the pre-FFI early-return paths (1023-byte name → EBADNAME, empty addr → ENOTIMP), sobun_cares_sysinMIRI_CRATESwon't hit foreign-function errors.
Extended reasoning...
Overview
This PR fixes a Tree Borrows / Stacked Borrows protector violation in the c-ares DNS request lifecycle. Previously, six handler traits in src/cares_sys/c_ares.rs took &mut self and four Channel registration methods took ctx: &mut T; the completion path (on_cares_complete in src/runtime/dns_jsc/dns.rs) frees the request allocation via heap::take, which is UB when a reference argument to that allocation is still live in a caller frame — even if never used again. The fix threads the request as *mut Self / *mut T end-to-end so no protected reference exists at deallocation time. The PR also adds eight Miri unit tests to c_ares.rs (driving a self-freeing request through every thunk and the two Channel methods that complete synchronously without reaching c-ares), a source-lint test that bans on_cares_complete(self-as-pointer, ..) spellings tree-wide, wires bun_cares_sys into the Miri crate set and workflow paths, and adds a doc note to bun_core::callback_ctx steering consuming callbacks away from it.
Security risks
None identified. This is a soundness refinement with no observable behaviour change: every thunk and impl performs the same operations in the same order; only the type carrying the pointer changes. No new inputs are parsed and no validation is relaxed. The only surface widening is that four Channel methods and six trait methods become unsafe fn, which is the honest spelling of the contract they already had.
Level of scrutiny
High. This is native memory-lifetime code on the DNS resolution path, the traits are public within the workspace, and the change alters six trait signatures plus four method signatures from safe-with-&mut to unsafe-with-raw-pointer. The pattern is established (the description cites #37681, #37716, #37685, #37693, #37705 as siblings in the same family), the Miri tests demonstrate the failure on the old code and pass on the new, and CI is green on every lane that ran (the two darwin-26 shards expired on an empty agent queue, unrelated). Still, a human should confirm the soundness reasoning and decide whether the comment-cop flags on the SAFETY / # Safety blocks warrant trimming — they look like a length-heuristic firing on documentation the codebase's own rules require, but that's a maintainer call.
Other factors
I traced each of the nine thunks: after let this = ctx.cast::<T>(), no reference to *this is formed before T::on_*(this, ..), and this is not touched after that call on any branch, satisfying the stated handler contract. In the four Channel methods, ctx is only ever .cast::<c_void>()'d and passed on. The two new Channel tests use Channel::opaque_mut(NonNull::dangling()), which is sound because Channel is a ZST (asserted at its definition) and both test inputs (name.len() >= 1023, empty ip_addr) hit the early-return branches before any ares_* extern is reached — so adding bun_cares_sys to MIRI_CRATES won't trip Miri's foreign-function restriction. The source lint self-tests its regex against positive and negative spellings and guards against a vacuous pass with a scanned > 0 assertion. The 18 comment-cop inline comments target the extended callback-wrapper note, the per-method # Safety sections, per-unsafe-block SAFETY comments, and the test-module header; none of those read as workaround justifications to me, but they are unaddressed on the thread.
The rationale for passing the request as *mut stays once, on the callback-wrapper note; every # Safety section and SAFETY comment now states its own contract in a line instead of pointing at a longer block. The callback_ctx doc addition is dropped: its existing contract (the T must stay live for the whole callback) already excludes a callback that frees it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/source-lints/self-receiver-cares-complete.test.ts`:
- Around line 65-75: Update the Rust-source scan around the offenders loop to
first use git grep with the on_cares_complete token and src scope to build a
shortlist of tracked files, then scan only those candidates while preserving
canonical-path filtering and tracked-file semantics. Keep the scanned counter
and its scanned > 0 guard, and follow the existing git grep discovery pattern
from no-mangle-short-names.test.ts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9a2d815-817b-478c-860c-3f39dca99301
📒 Files selected for processing (5)
.github/workflows/miri.ymlscripts/rust-miri.tssrc/cares_sys/c_ares.rssrc/runtime/dns_jsc/dns.rstest/internal/source-lints/self-receiver-cares-complete.test.ts
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the unsafe FFI ownership contract across six handler traits, nine extern "C" thunks, and four Channel registration methods — memory-safety code in the most-scrutinized category — a human look would still be worthwhile.
What was reviewed:
- The
&mut self→*mut Selfconversion in all six traits and their thunks; each thunk still delivers exactly once on every branch, and the seven dns.rs impls forwardthisunchanged toon_cares_complete. - The four
Channel::{get_addr_info,resolve,get_host_by_addr,get_name_info}sites and their dns.rs callers:ctxreaches c-ares (or the synchronous EBADNAME/ENOTIMP thunk) exactly once, and therequestpointer at each call site was already a rawheap::allocpointer, so no reference is now silently coerced. - The Miri tests only take error-status paths and the two pre-c-ares synchronous completions, so no foreign call is reached;
Channelis a ZST so the danglingopaque_mutis sound. - The comment-cop flags on this PR land on the one-line
# Safetycontracts and the 4-line callback-wrapper rationale, which the last commit already reduced from the earlier multi-line versions.
Extended reasoning...
Overview
The PR converts the c-ares request-completion path from &mut self receivers to *mut Self raw pointers so that on_cares_complete (which heap::takes the request on every path) no longer deallocates behind a live reference argument — a Tree Borrows protector violation. It touches src/cares_sys/c_ares.rs (six handler traits, nine extern "C" thunks, four Channel methods, plus a new #[cfg(test)] Miri module), src/runtime/dns_jsc/dns.rs (seven trait impls and four registration call sites), scripts/rust-miri.ts and .github/workflows/miri.yml (adds bun_cares_sys to the Miri crate set), and a new source-lint test.
Security risks
None identified. This is an aliasing-model correctness fix; no user-controlled input handling, auth, or crypto is touched. The behaviour of every thunk and impl is preserved — only the type the pointer travels as changes.
Level of scrutiny
High. This is unsafe Rust at an FFI boundary, the category REVIEW.md calls out as most-blocked. The change is mechanical in shape (the same two-token conversion applied at every site) and mirrors already-landed siblings (#37681 ReadBytesHandler, #37716, #37685, #37693, #37705), but it changes public trait signatures and marks four Channel methods unsafe, which affects at least one open PR (#36186). The Miri tests and source lint give strong regression protection, and CI is green on every lane that ran (macOS 26 shards expired unclaimed on an empty agent queue, unrelated to this change).
Other factors
The comment-cop bot left a batch of flags at 16:54 UTC; commit a563990 (16:54:00) reduced every # Safety doc to a single line and the callback-wrapper note to four lines. The remaining flags land on required clippy # Safety documentation and a short rationale block, which read as appropriate rather than as workaround justification — but a human should confirm the bot's threshold is satisfied. Given the scope (unsafe-code trait-signature change across two crates) and the repo's stated bar for native memory-safety changes, deferring rather than auto-approving.
1 participant
Problem
&mut self(the handler traits) andctx: &mut T(the fourChannelregistration methods), so on every completion the request was freed while a reference to it was still a live argument two frames up.Channelframe is also live when c-ares completes synchronously inside the registering call: names it refuses (ARES_EBADNAME), addresses that do not parse (ARES_ENOTIMP), and literal addresses or.onionnames given toBun.dns.lookup(.., { backend: "c-ares" }).Undefined Behavior: deallocation through <tag> ... is forbidden(the strongly protected tag disallows deallocations) at its&mut self.Fix
*mutthe whole way: the six trait methods takethis: *mut Self, the thunks cast the c-ares ctx and callT::on_*(this, ..), the fourChannelmethods takectx: *mut T, and the seven impls in dns.rs forwardthistoon_cares_complete. This is the shapeReadBytesHandleralready uses.Channelpaths that complete without reaching c-ares; all eight fail under Miri at the base commit and pass here. A new source lint flagged the seven old call sites and passes now. Existing dns suites pass, apart from real-network lookups the released binary also fails in the same container.Background
dns.resolve*,reverse,lookupServiceand thec-aresbackend ofBun.dns.lookup. A query is registered with a callback plus an opaquevoid*ctx, and c-ares may invoke the callback before the registering call returns.src/cares_sys/c_ares.rswraps each callback kind as a trait (HostentHandler,AddrInfoHandler,ReplyHandler, ...) plus anextern "C"thunk that parses the reply and calls the trait method.src/runtime/dns_jsc/dns.rsimplements the traits; every impl ends inon_cares_complete, which settles the JS side and frees the request.dereferenceableattribute encodes and what Miri's Tree Borrows checks. Raw pointers make no such promise.bun run rust:miriruns a fixed list of FFI-free crates; the new tests never call c-ares, so its bindings crate can join the list. The runtime crate holding dns.rs links everything and cannot, which is why the second guard is a source lint.test/internal/source-lints/holds tests that grep the tree for banned spellings; each ships with positive and negative samples so the pattern itself is checked.Original description
Problem
Every c-ares completion in src/runtime/dns_jsc/dns.rs ends in an
on_cares_complete(this: *mut Self, ..)that reclaims the request's allocation on every path (heap::take(this)directly, ordrain_pending_*->heap::take(key.lookup)when the request is the pending-cache owner). Two frames above it still held the request as a reference. At626034fda0:HostentHandler,HostentWithTtlsHandler,NameinfoHandler,AddrInfoHandler,ReplyHandler,AnyHandler) took&mut self; theextern "C"thunks formed that reference withbun_core::callback_ctx, and the seven impls in dns.rs (GetHostByAddrInfoRequest,GetNameInfoRequest,GetAddrInfoRequest, and the fourResolveInfoRequest<T>impls produced by the record-type macros) calledSelf::on_cares_complete(ptr::from_mut(self), ..). The request is freed whileselfis still a live argument of the trait method, on every completion.Channel::{get_addr_info, resolve, get_host_by_addr, get_name_info}took the request asctx: &mut T.resolveandget_host_by_addrinvoke the thunk themselves for names they refuse (ARES_EBADNAME) and addresses that do not parse (ARES_ENOTIMP), and c-ares itself completes synchronously for e.g.resolve4("a..b"), a literal address or a.onionname given toBun.dns.lookup(.., { backend: "c-ares" })(the cases dns: arm the c-ares retransmit timer before dispatch so sync rejections balance #35776 measures). On those paths the request is also freed while thectxargument of theChannelmethod is live.A reference argument is protected for the whole call it is passed to, and deallocating protected memory is undefined behaviour under both aliasing models whether or not the reference is used again afterwards. This is checkable on the real code: the unit tests added to c_ares.rs (see Tests) drive a self-freeing request through the thunks and through the two
Channelmethods that complete without reaching c-ares, and against the626034fda0version of the file every one of them fails under Miri with Tree Borrows (the modelbun run rust:miriuses):(the other five traits report the same at their own
&mut self). With the traits and thunks converted butresolve/get_host_by_addrstill takingctx: &mut T, the thunk tests pass and the twoChanneltests still fail, with the protected tag now created at thectx: &mut Tparameter of theChannelmethod itself; with both converted all eight pass. The protector is the model behind thedereferenceableattribute rustc puts on reference arguments, so this is a latent miscompile hazard rather than a Miri-only concern; no crash is known from it, and the unoptimized debug/ASAN build cannot observe it. Same family as #37681 (ReadBytesHandler::on_read_bytes), #37716 (JsSinkType::finalize), #37685, #37693, #37705; the comments onGetAddrInfoRequest::on_cares_complete/thenin dns.rs already state the rule for the frame below this one.Fix
The request travels as a raw pointer the whole way, the shape
ReadBytesHandler/read_bytes_to_handler(ctx: *mut H)already use:unsafe fn on_*(this: *mut Self, ..); the nine thunks bindctx.cast::<T>()and callT::on_*(this, ..)instead of going throughcallback_ctx; the fourChannelmethods becomeunsafe fntakingctx: *mut Tand pass it straight to c-ares or to the thunk. The rationale is stated once, on the existing callback-wrapper note; each trait method andChannelmethod carries a one-line# Safetycontract.this: *mut Selfand forward it toon_cares_complete; the four registration sites passrequestinstead of&mut *request, with their SAFETY comments now stating what actually holds (the request stays allocated until its handler, possibly invoked before the call returns, consumes it) instead of "the borrow is not held past this call", which was the wrong property.Every thunk and impl performs the same operations in the same order as before; only the type the pointer travels as changes. Nothing else implements these traits or calls these
Channelmethods (#36186, open, adds a TLSAReplyHandlerimpl in the old shape and will need the two-token update when it rebases; #35776 edits the lines next to the four registration sites).callback_ctx'sdebug_assert!(!ctx.is_null())is not replaced: everyon_cares_completedereferencesthison its first line, so a null ctx would fault at the same place in any build.Tests
#[cfg(test)]module: aRequestthat reclaims itself in its completion (as every dns_jsc request type does) implements all seven traits and is delivered with an error status through each of the nine thunks, plus throughChannel::resolvewith a 1023-byte name andChannel::get_host_by_addrwith an empty address, the two registration frames that complete a request without calling c-ares (Channelis an asserted ZST, soChannel::opaque_muton a dangling pointer is a valid channel for those paths). None of this reaches a foreign function, sobun_cares_sysis added toMIRI_CRATESin scripts/rust-miri.ts andsrc/cares_sys/**to the Miri workflow's paths;bun run rust:miri -p bun_cares_sysruns the eight tests in about a second, and the Miri workflow runs on this PR because it touches those two files. The failing output against626034fda0is quoted above.get_addr_infoandget_name_infocall into c-ares unconditionally and are covered by the same signature change plus the existing dns suites.on_cares_complete(is never passed the receiver (self,ptr::from_mut(self),self as *mut _,&raw mut *self,addr_of_mut!(*self),NonNull::from(self), including rustfmt-wrapped calls), with its pattern checked against positive and negative spellings. With dns.rs at626034fda0it reports:and passes on this branch. It is the
on_cares_completeentry of the teardown-name lint that #37685 / #37693 / #37705 are adding (their headers name it as the one consuming helper they do not cover yet); once one of those lands, the name goes into its list and this file can go.Verification
Debug (ASAN) build on Linux.
bun run rust:miri -p bun_cares_syspasses (8 tests, leak check clean);cargo clippyonbun_cares_sys(also with--tests) andbun_runtime,cargo fmt --check, andcargo check -p bun_cares_sys --target x86_64-pc-windows-msvcare clean. test/internal/source-lints/ (all 19 files) passes. test/js/bun/dns/dns-interleave, dns-prefetch, test/js/node/dns/dns-lookup-keepalive, dns-resolver-concurrent-timeout and dns-tcp-bidirectional-poll pass; in test/js/node/dns/node-dns.test.js and test/js/bun/dns/resolve-dns.test.ts the failing set (real-network lookups, ESERVFAIL from this container's resolver, identical on the system and libc backends this change does not touch) is a subset of what the released binary fails here, and the rest pass. A script driving every converted path against an unreachable resolver (the EBADNAME / ENOTIMP / literal-address /.onionsynchronous completions, one query per handler trait,reverse,lookupService, c-areslookup, and a coalesced pending-cache pair) completes all 22 with the expected codes under ASAN.