node:dns: leave errno undefined on c-ares resolver errors - #37323
node:dns: leave errno undefined on c-ares resolver errors#37323robobun wants to merge 7 commits into
Conversation
Node's dnsException only assigns a numeric errno when libuv reported a numeric error (getaddrinfo/getnameinfo); c-ares resolver failures carry a string code and leave errno undefined. Bun was exposing the raw c-ares status enum (e.g. 4 for ENOTFOUND) as errno on query*/getHostByAddr errors, a value with no meaning in Node's error model. Fixes #37320
|
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 (5)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe DNS error conversion now supports an undefined ChangesDNS resolver error compatibility
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/js/node/dns/node-dns.test.js`:
- Around line 181-190: Update the Promise wrappers for the queryErr and
reverseErr lookups to reject when their callbacks receive no error, while
resolving with the error object otherwise. Preserve the existing ENOTFOUND
assertions so unexpected successful lookups are reported as explicit promise
failures rather than causing a property-access TypeError.
- Around line 164-175: Update the DNS test’s UDP fixture around socket.send and
socket lifecycle handling: route send failures and socket error events to the
test rejection path, create/register the close promise before invoking
socket.close(), and await that promise in finally to ensure cleanup completes.
- Around line 178-190: Extend the DNS error tests around the existing resolver
cases to assert the preserved numeric errno for dns.lookup/getaddrinfo failures,
and add a getnameinfo failure case with its expected error fields. If
synchronous constructor channel-initialization failures are reachable, cover
those separately rather than combining them with deferred c-ares query and
reverse tests.
🪄 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: 904c0286-a4c6-49cc-93b7-74533897a6ce
📒 Files selected for processing (2)
src/runtime/dns_jsc/cares_jsc.rstest/js/node/dns/node-dns.test.js
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix that mirrors Node's own dnsException behavior (setting errno to undefined for c-ares resolver errors while keeping it numeric for getaddrinfo/getnameinfo).
What was reviewed:
- Traced all call sites of the three error constructors: predicate matches only
query*andgetHostByAddr;getaddrinfo,getnameinfo, and the UDPconnectpath are correctly left with numeric errno. - Confirmed
.put(..., UNDEFINED)sets an own property (matching Node, which explicitly assignsex.errno = undefined), rather than deleting it. - Test is hermetic (local UDP NXDOMAIN server, resolver timeout bounds it), follows the existing compressed-SRV fixture pattern in the same file, and rejects on unexpected success per the addressed CodeRabbit feedback.
Extended reasoning...
Overview
This PR fixes #37320: Bun's node:dns resolver errors carried a numeric errno set to the raw c-ares status enum (e.g. 4 for ARES_ENOTFOUND), whereas Node.js leaves errno as an own property with value undefined for c-ares resolver failures and only assigns a numeric errno for libuv-backed lookups (getaddrinfo/getnameinfo). The fix adds a small helper clear_errno_for_resolver_syscall in src/runtime/dns_jsc/cares_jsc.rs that overwrites errno with undefined when the syscall name starts with query or equals getHostByAddr, and applies it at all three DNSException construction sites (ErrorDeferred::reject, error_to_js_with_syscall, error_to_js_with_syscall_and_hostname). A hermetic test spins up a local UDP DNS server that replies NXDOMAIN and asserts resolve4 and reverse errors have code: 'ENOTFOUND', correct syscall/hostname, and errno === undefined.
Security risks
None. This changes only the shape of an error object property (numeric → undefined) on an already-thrown exception. No new input parsing, no privilege boundaries, no allocation changes.
Level of scrutiny
Low-to-medium. It's a 17-line source change plus a 42-line test in the Node compat layer. I enumerated every caller of the three error constructors across src/runtime/dns_jsc/ and src/runtime/socket/ to confirm the syscall-name predicate is exhaustive and exclusive: getnameinfo (dns.rs:713/727), getHostByAddr (dns.rs:1506/1517, 5077), T::SYSCALL = query* (dns.rs:1656/1667, 5395), getaddrinfo (dns.rs:1794/1825/1833, 5193), and connect (udp_socket.rs:761). Only the resolver syscalls match the predicate; the lookup family and connect path retain numeric errno, matching Node's split.
Other factors
- The post-construction
.put(b"errno", UNDEFINED)mirrors Node's owndnsExceptionimplementation, which explicitly assignsex.errno = undefined— so this is the semantically faithful fix, not a workaround. The comment-cop bot flagged the doc comment length; it was tightened in 54c3fca and both threads are resolved. - CodeRabbit's suggestion to reject on unexpected successful lookups was addressed in aa40ae2 (both promise wrappers now
reject(new Error(...))whenerris falsy). The fire-and-forgetsocket.sendconcern was withdrawn — the resolver's 1s timeout bounds any lost reply, matching the existing compressed-SRV fixture directly above. - No CODEOWNERS entry for these paths. No prior claude[bot] review on this PR.
|
Status: ready for review. Repro: Latest push: the errno decision now happens when the native code fills the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes native error-construction semantics in the DNS resolver (post-hoc overwrite of errno gated on a syscall-name prefix match), a human look at the approach would still be worthwhile.
Checked that the query*/getHostByAddr filter covers every resolver SYSCALL constant and excludes the getaddrinfo/getnameinfo and fetch/Bun.connect paths, which keep numeric errno.
Verified .put(errno, UNDEFINED) matches Node's dnsException, which sets errno as an own undefined property rather than omitting it.
The new test is hermetic (local NXDOMAIN UDP server, bounded by the resolver timeout) and mirrors the compressed-SRV fixture directly above it; the earlier CodeRabbit and comment-cop threads are all resolved.
Extended reasoning...
Overview
The PR fixes #37320: c-ares resolver errors (resolve4, reverse, etc.) were surfacing the raw c-ares status enum as errno (e.g. 4 for ARES_ENOTFOUND), whereas Node leaves errno as an own property with value undefined on these errors and only assigns a numeric errno for libuv-backed getaddrinfo/getnameinfo failures. The fix adds a small helper clear_errno_for_resolver_syscall in src/runtime/dns_jsc/cares_jsc.rs and calls it from all three DNSException construction sites (ErrorDeferred::reject, error_to_js_with_syscall, error_to_js_with_syscall_and_hostname). A hermetic regression test in test/js/node/dns/node-dns.test.js spins up a local UDP server that answers NXDOMAIN and asserts the error shape for both resolve4 (queryA) and reverse (getHostByAddr).
Security risks
None. This only adjusts which value a property on an already-thrown error object holds; no parsing, allocation, or trust boundary is touched.
Level of scrutiny
Moderate. It is native Rust code in the resolver error path, but the change is ~20 lines and purely additive — every existing field on the error remains, and only errno is overwritten to undefined for a bounded set of syscall names. I traced every call site: all resolver SYSCALL constants are query* (Srv/Soa/Txt/Naptr/Mx/Caa/Any/Ns/Ptr/Cname/A/Aaaa) plus the two getHostByAddr sites, and the getaddrinfo/getnameinfo deferred paths plus the system_error_with_syscall_and_hostname callers in socket_body.rs and FetchTasklet.rs (both getaddrinfo) are correctly left alone.
Other factors
All prior review threads (CodeRabbit's reject-on-unexpected-success and UDP-fixture notes, and the comment-cop doc-comment length flag) are resolved in commits aa40ae2 and 54c3fca. The remaining reason to defer is architectural taste: the fix builds SystemError with errno: this as i32 and then overwrites the JS property afterward, gated on a string-prefix match of the syscall name. That is semantically correct (Node's dnsException literally assigns ex.errno = undefined), and it is the minimal change, but a maintainer may prefer threading a flag through or making SystemError support omitting errno rather than set-then-clear. That is a judgment call for a human, not a bug.
…rrno test Co-authored-by: deepshekhardas <deepshekharadas1234@gmail.com>
|
Updated 5:36 PM PT - Aug 19th, 2026
✅ @robobun, your commit 4a1482b2344c19e35bda2bdb1197f1a1c11a54d1 passed in 🧪 To try this PR locally: bunx bun-pr 37323That installs a local version of the PR into your bun-37323 --bun |
There was a problem hiding this comment.
LGTM — small, well-scoped Node.js compat fix.
What was reviewed:
- Traced every caller of the three error builders:
query*/getHostByAddrsyscalls get errno cleared;getaddrinfo/getnameinfo/connect(fetch, Bun.connect, udp_socket, lookup, lookupService) are left untouched. - Test is hermetic (local UDP NXDOMAIN responder,
port: 0, bounded by resolvertimeout: 1000), covers callback+promise × resolve4/resolveAny/reverse, and assertserrnoremains an own property. - The added
err.errno).toBeNumber()guard in the pre-existingdns.lookup badtest prevents the getaddrinfo path from regressing.
Extended reasoning...
Overview
This PR fixes #37320: Bun's node:dns resolver errors (resolve*, reverse) carried a numeric errno holding the raw c-ares status enum value, whereas Node leaves errno as an own property with value undefined on c-ares-backed errors (only getaddrinfo/getnameinfo errors get a numeric libuv errno). The fix adds a single helper clear_errno_for_resolver_syscall in src/runtime/dns_jsc/cares_jsc.rs that overwrites errno with JSValue::UNDEFINED when the syscall string is query* or getHostByAddr, and calls it from the three error-instance builders (ErrorDeferred::reject, error_to_js_with_syscall, error_to_js_with_syscall_and_hostname).
Security risks
None. This only adjusts one property value on an already-constructed JS error object. No parsing of untrusted input, no new allocations, no changes to control flow.
Level of scrutiny
Low-to-medium. This is a Node.js compatibility fix touching error-object shape only, not resolver behavior. I traced every call site of the three affected builders in dns.rs, udp_socket.rs, FetchTasklet.rs, and socket_body.rs: the syscall filter correctly matches only the c-ares resolver paths (getHostByAddr and the T::SYSCALL values which are all "query" + ucfirst(TYPE_NAME)), and leaves getaddrinfo, getnameinfo, and connect untouched. The system_error_with_syscall_and_hostname helper (used by fetch/Bun.connect to build a raw SystemError struct) is not affected — only the JS-instance builders call the new helper, and their external callers pass non-matching syscalls anyway.
Other factors
- The new test uses the same local-UDP-server pattern as the existing compressed-SRV and CAA tests in the file (skipped on Windows,
port: 0,timeout: 1000, tries: 1), so a lost reply fails assertions rather than hanging. It covers both callback and promise APIs across three resolver methods and asserts exactcode/syscall/hostname/errno/hasOwnErrnoshapes. - All prior CodeRabbit and comment-cop feedback is marked resolved; the callback wrappers now reject explicitly on unexpected success, and the doc comment on the helper was trimmed.
- The bug-hunting system found no issues. CI on the merge commit was reported green on all lanes that ran (the only failure was an Azure agent-provisioning error on a Windows lane, and the new test is Windows-skipped).
Add SystemError::NO_ERRNO. The C++ emitter turns it into an own errno property with the value undefined. The three c-ares error builders set it for resolver syscalls instead of overwriting errno on the JS error afterwards. The test also covers a getaddrinfo failure that needs no network, which has to keep its numeric errno.
4a1482b to
3b97647
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a new cross-FFI sentinel convention (NO_ERRNO = INT_MIN synced between Rust and bindings.cpp) and there are three unresolved comment-cop flags on the doc comments, a human look would still be worthwhile.
What was reviewed:
errno_for_syscallgates onquery*/getHostByAddr; confirmed the external callers insocket_body.rs,udp_socket.rs, andFetchTasklet.rsall passgetaddrinfoand keep their numeric errno.INT_MINsentinel cannot collide with real errno/libuv values, andSystemError::default().errno = 0still emits0, so existing callers are unaffected.- The local NXDOMAIN fixture matches the compressed-SRV pattern already in the file and is bounded by the resolver timeout.
Extended reasoning...
Overview
This PR fixes #37320: dns.resolve*/reverse errors in Bun carry a numeric errno (the raw c-ares status enum), while Node leaves errno as an own property with value undefined for c-ares resolver failures. The fix spans five files: a new SystemError::NO_ERRNO sentinel constant in src/jsc/SystemError.rs (= c_int::MIN), a mirrored comment in headers-handwritten.h, a systemErrorErrnoValue helper in bindings.cpp that maps the sentinel to jsUndefined() at the two emission sites, an errno_for_syscall helper in cares_jsc.rs that returns NO_ERRNO for query*/getHostByAddr syscalls (used by all three error builders), and a ~95-line test in node-dns.test.js using a local UDP NXDOMAIN server plus a network-free dns.lookup NUL-byte failure to verify both families.
Security risks
None identified. The change only affects the value of the errno property on already-thrown DNS error objects. No new input parsing, no allocation sizing, no auth/crypto paths.
Level of scrutiny
Medium-to-high. The change is small in line count but introduces a new sentinel convention that must stay in sync across the Rust/C++ FFI boundary — std::numeric_limits<int>::min() in C++ must match c_int::MIN in Rust (they do; both are INT_MIN for a 32-bit int). The fd field already uses the same convention, which the PR description cites, but adding a second sentinel field to a shared #[repr(C)] struct is a design decision a maintainer should sign off on. The change also touches bindings.cpp, which REVIEW.md calls out as a monolithic file where new code needs care, and it's a Node-compat behavioral change.
Other factors
- All three CodeRabbit inline findings from the first revision are resolved. The remaining unresolved comments are three automated comment-cop flags ("paragraph-long comment to justify a workaround") on the new doc comments in
SystemError.rs:87,bindings.cpp:2573, andcares_jsc.rs:646. These look like false positives — the comments are 2-4 line doc comments explaining Node's behavior, not workaround justifications — but they're unaddressed and the author may want to trim or dismiss them. - I verified the callers of the three modified error builders:
dns.rspassesquery*syscall names andgetHostByAddr;socket_body.rs,udp_socket.rs, andFetchTasklet.rsall passb"getaddrinfo", which falls through to the numeric branch. So non-resolver DNS errors keep their number, as the PR description claims. bun_sys::SystemError::errnoisc_intand is copied verbatim by theFromimpl; no realistic errno source producesINT_MIN, so no accidentalundefinedfor other error paths.- The test is thorough (callback + promise API, three record types, own-property check, and a positive control that
lookupstill gets a number) and follows the existing local-UDP-fixture pattern in the same file. It's Windows-skipped like its neighbors. - The bug hunter found no issues.
1 participant
Fixes #37320
Problem
node:dns(resolve*,reverse) expose the raw c-ares status enum as a numericerrno, for exampleerrno: 4(ARES_ENOTFOUND). Node leaveserrnoas an own property with the valueundefinedon these errors.src/runtime/dns_jsc/cares_jsc.rsstored the c-ares enum inSystemError.errno, and the C++ emitter always wrote it as a number.Fix
SystemError::NO_ERRNO(c_int::MIN, the same sentinel thefdfield already uses). The C++ emitter (systemErrorToErrorInstance) writeserrno: undefinedfor it.NO_ERRNOfor resolver syscalls (query*,getHostByAddr). Lookup-family errors (getaddrinfo,getnameinfo) and the fetch/Bun.connectpaths keep their numericerrno, matching Node's split: Node only assigns a numericerrnowhen libuv reported a numeric code.test/js/node/dns/node-dns.test.jsruns a local UDP DNS server that answers NXDOMAIN and checks the full error shape (callback and promise APIs,resolve4,resolveAny,reverse, plus a hermeticlookupfailure that keeps a numericerrno). It fails on the released bun and passes with this change.Background
dnsException(lib/internal/errors.js) receives either a numeric libuv code (getaddrinfo/getnameinfo) or a string code (c-ares query callbacks). It assignsex.errno = errnowhereerrnois only set in the numeric case, so resolver errors carry an ownerrno: undefined.SystemErroris bun's#[repr(C)]error descriptor shared with C++. The Rust side fills the fields,SystemError__toErrorInstancebuilds the JS error.dns.NOTFOUNDis the string'ENOTFOUND', and libuv errnos are negative.Notes
errnowithundefinedon the JS object after construction. Review preferred deciding the value where theSystemErroris built, which is the current shape.dns.promises.lookup("invalid.invalid\0"): a NUL byte is rejected before any resolver backend runs, so the getaddrinfo error needs no network. The pre-existing network-dependentdns.lookup badtest also assertserrnois a number now.test/js/node/dns/local-fixture files, andtest/js/node/dns/node-dns.test.js(remaining failures are the pre-existing network-dependent tests, identical with and without the fix).