Conversation
… error
us_udp_socket_connect returns -1 when connect(2) fails and a getaddrinfo
status when the name does not resolve. The connect option handled the -1
through a helper that returned nothing on Windows, so the value fell
through to the getaddrinfo mapping and every connect(2) failure there was
reported as ENOTFOUND. The connect() method node:dgram uses threw a bare
"Failed to connect socket" on every platform and ignored getaddrinfo
failures altogether.
Both paths now go through one check: -1 reads errno (WSAGetLastError on
Windows), anything else non-zero is a getaddrinfo code.
bsd_connect_udp_socket captures the connect() error before freeaddrinfo()
so that is what the caller reads. node:dgram decorates the error like
Node's ExceptionWithHostPort ("connect EAFNOSUPPORT ::1:1", .address,
.port), sharing the helper the send path already used.
|
Warning Review limit reached
Next review available in: 23 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 (5)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on Windows x64 with the 1.4.0 release and the canary at eabb96d: Tests: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a well-scoped fix, but it reworks cross-platform errno/WSA handling across the C↔Rust boundary and changes user-visible error behavior for node:dgram connect(), so a human look would still be worthwhile — especially given the noted overlap with #38588 on bsd_connect_udp_socket.
What was reviewed:
bsd_connect_udp_socket:LIBUS_ERRcaptures errno/WSAGetLastError immediately afterconnect()and is re-installed afterfreeaddrinfo; theresult == NULLcase now yieldsEAFNOSUPPORTvia the same path.check_connect's-1vs getaddrinfo-code split:EAI_BADFLAGS == -1on glibc, buthints.ai_flagsis zeroed so it isn't reachable; macOS/Windows EAI codes don't collide with -1.get_us_errorrefactor preserves the existing POSIX== -1/ Windows< 0semantics; the removederrno_syswas file-local (no other callers).address_z.as_bytes()excludes the trailing NUL (ZBox::as_bytesvsas_bytes_with_nul), so the DNS-error hostname isn't NUL-padded.
Extended reasoning...
Overview
The PR fixes UDP connect() error reporting so a failed connect(2) surfaces its actual errno instead of being misclassified as a DNS ENOTFOUND (Windows) or a codeless Error: Failed to connect socket (node:dgram on all platforms). It touches:
packages/bun-usockets/src/bsd.c—bsd_connect_udp_socketnow captures the lastconnect()error beforefreeaddrinfo()and re-installs it viaerrno/WSASetLastError; an empty getaddrinfo result reportsEAFNOSUPPORTrather than a stale value.src/runtime/socket/udp_socket.rs— removes the file-localerrno_sysshim, splitsget_us_errorinto failure detection + a sharedlast_socket_errorreader, and addscheck_connectthat both connect callers (Bun.udpSocket({connect})andnode:dgram'sjs_connect) use to interpret the two-namespace return (0 / -1 / EAI code).src/js/node/dgram.ts— generalizesdecorateSendError→decorateHostPortError(err, syscall, address, port)and applies it to the connect error path so.syscall/.address/.port/.messagematch Node'sExceptionWithHostPort.- New tests in
udp_socket.test.tsanddgram.test.tscovering the IPv4→::1family mismatch (deterministic offline), the Windows-only0.0.0.0→EADDRNOTAVAILcase, and thenode:dgramcallback +'error'-event shapes.
Security risks
None identified. This is error-classification plumbing on already-failed syscalls; no new inputs are parsed, no privilege boundaries crossed, no resource allocation added.
Level of scrutiny
Moderate. The diff is small and the mechanism is very clearly explained, but it sits squarely in the "Cross-platform" and "Error handling" review categories: platform-gated C (#ifdef _WIN32), platform-gated Rust (#[cfg(windows)]), errno preservation across a libc call, and Winsock's separate error slot. It also changes user-visible behavior (node:dgram's socket.connect() now fails on an unresolvable address where it previously reported success). None of that is mechanical enough to auto-approve.
Other factors
- Verified
LIBUS_ERRexpands toWSAGetLastError()/errno(internal.h:80,84) and(int)LIBUS_SOCKET_ERRORis-1on both platforms, so the capture-then-reinstall in bsd.c is sound andcheck_connect's-1arm matches on Windows too. - Checked the glibc
EAI_BADFLAGS == -1collision:hintsis zeroed (noai_flags), so getaddrinfo cannot return it here; macOS/BSD EAI codes are positive and Windows uses WSA-range values, so-1is unambiguously the connect-failure sentinel. - Confirmed the removed
errno_syswas local to this file (its only remaining caller wasget_us_error's POSIX arm, now inlined asres == -1); othererrno_syshits in the tree are unrelated definitions. ZBox::as_bytes()excludes the trailing NUL (util.rs:157 vs the separateas_bytes_with_nulat :174), so the hostname passed intoerror_to_js_with_syscall_and_hostnameis clean — and this call site already existed pre-PR for theBun.udpSocket({connect})path.- The PR flags an upcoming conflict with #38588 on the same C function; that coordination is a reason on its own for a maintainer to look before merging.
|
Not a duplicate of #38588, though the two touch the same C function.
|
#40986) ### Problem - `Bun.serve`, `Bun.listen` and `Bun.udpSocket` (bind and connect) resolve their hostname with a synchronous `getaddrinfo` in uSockets (`packages/bun-usockets/src/bsd.c:1340`, `:1733`, `:1818`), even for a name that can never be a hostname. Some resolvers never answer such a query, and the call blocks the JS thread (#40970 saw this on the connect side). - Its error is also wrong: `Bun.serve({ hostname: "this is not a hostname" })` throws `ENOENT: no such file or directory, listen`, `Bun.listen` a bare `Failed to listen at <host>`, `Bun.udpSocket` `bind ENOENT <host>`, and `dgram`'s `connect()` reports success. ### Fix - Each path checks the hostname with `bun_dns::is_valid_hostname` before it calls into uSockets. A rejected name throws `getaddrinfo ENOTFOUND <host>` with `code`, `syscall` and `hostname`: what `Bun.connect` reports for it and what Node emits from `server.listen()`. - `Bun.serve` now strips brackets only around an IPv6 literal: `"[::1]"` still binds, `"[foo]"` no longer resolves as `foo`. - A well-formed name that does not resolve (`nxdomain.invalid`) still gets the wrong errno. #37690 (serve/listen) and #37707 (udp) fix that. This PR does not touch `bsd.c` or fix #25765. - Verified: 18 new cases in `test/js/bun/http/serve-listen.test.ts`, `test/js/bun/net/socket-dns-error.test.ts`, `test/js/bun/udp/udp_socket.test.ts` and `test/js/bun/udp/dgram.test.ts` fail on 1.4.1 and pass here. Also ran the serve, socket, net and dgram suites. ### Background - `is_valid_hostname` (`src/dns/lib.rs`) accepts an IP literal or an RFC 1035 name (labels of 1 to 63 bytes from the c-ares set, 253 total). - Node resolves with `dns.lookup` before it binds, so no name reaches a blocking call. - Self-reviewed: 2 concerns, both addressed (an unchecked udp `connect` path; the framing is now the resolver skip). <details><summary>Notes</summary> What is and is not shown here: - The blocking resolver is not observed in this PR. It is the same libc `getaddrinfo` call, on the same names, that #40970 measured on the connect side (`dns.lookup` waiting out the macOS resolver timeout for a label with a space). Linux glibc rejects these names locally, so on Linux the visible change is only the error. - No user report exists for a malformed bind hostname. #25765 (`something.localhost` on macOS) is a well-formed name and is #37690's case. Repro on 1.4.1 (Linux, no resolver needed for the first four names): ``` serve "this is not a hostname" => ENOENT listen "ENOENT: no such file or directory, listen" listen "this is not a hostname" => (no code) "Failed to listen at this is not a hostname" udpSocket "this is not a hostname" => ENOENT bind "bind ENOENT this is not a hostname" udpSocket connect { hostname: "this is not a hostname" } => DNSException "connect ENOTFOUND this is not a hostname" dgram socket.connect(1234, "this is not a hostname") via a custom lookup => no error, socket marked connected serve "aaaa...(64).com" => EMSGSIZE listen serve "nxdomain.invalid" => EAGAIN listen (left to #37690) udpSocket "nxdomain.invalid" => ESRCH bind (left to #37707) ``` After this change the first six report `getaddrinfo ENOTFOUND <host>` (`name: "Error"`, `code: "ENOTFOUND"`, `syscall: "getaddrinfo"`, `hostname`). `node:net`, `node:http` and `node:dgram` (custom `lookup`) servers emit the same error object from `listen()` / `bind()`. Where the check sits: - `Bun.serve`: at the top of `NewServer::listen` (`src/runtime/server/mod.rs`), before the uws app is created. The existing `deinit` path frees the server and `Bun.serve` throws synchronously, as it does for `EADDRINUSE`. The bracket strip now lives in one function, `strip_ipv6_brackets`, used by the check and by the existing `Addr` code. It used to drop the first and last byte of any hostname that starts with `[`, so `[foo]` resolved as `foo` (`[localhost]`, `[127.0.0.1]` and `[example.com` were already rejected by the base URL parse). It now strips only when the inside is an IPv6 literal (`to_ip_address`, so a `%zone` still passes); anything else keeps its brackets and fails `is_valid_hostname`. - `Bun.listen` (`src/runtime/socket/Listener.rs`): right after `SocketConfig::from_js`, only when a port is set (a unix path or an adopted fd has no hostname). `SocketConfig::from_js` itself is shared with `Bun.connect`, whose error is delivered asynchronously, so the check is not placed there. - `Bun.udpSocket` (`src/runtime/socket/udp_socket.rs`): bind and `connect.hostname` are both checked right after the config parse, before the socket is created. Bind failures there throw synchronously today, and so does this. The `connect` option used to report these names as `connect ENOTFOUND <host>` (`DNSException`) from the `EAI_*` code; a well-formed unresolvable `connect.hostname` still does, that block is #38622's. - The internal `UDPSocket.jsConnect` that `node:dgram`'s `socket.connect()` calls gets the same check. `dgram` resolves with `dns.lookup` first, so only a custom `lookup` can hand it a raw name. glibc rejected such a name with a code `jsConnect` never read (it checks for `-1` only), so the socket was marked connected with no error. #38622 reworks that return-code handling; the check here sits before the call and does not overlap it. The error is built by `not_a_hostname_error` in `src/runtime/dns_jsc/cares_jsc.rs` from `system_error_with_syscall_and_hostname`, the helper `Bun.connect` uses. For #37690 and #37707: their tests use a 64-byte label, which `is_valid_hostname` rejects, so once this lands that input is answered in-process and no longer exercises the `dns_error` plumbing. glibc rejects `a*b.com`, `a/b.com`, `-a.com` and `ünicode.com` locally (`EAI_NONAME`, no query sent) while `is_valid_hostname` accepts all four. Other platforms need checking in CI. `Bun.listen({ hostname: "[::1]" })` and `Bun.udpSocket({ hostname: "[::1]" })` never stripped brackets and already fail on 1.4.1 (`Failed to listen at [::1]`, `bind ENOENT [::1]`). They now fail with `getaddrinfo ENOTFOUND [::1]`. Suites run on the debug build: `serve-listen.test.ts` (37 pass), `socket-dns-error.test.ts` (11 pass), `udp_socket.test.ts` (214 pass), `dgram.test.ts` + `udp_socket_recv_flags.test.ts` (64 pass), `serve.test.ts` + `serve-http3.test.ts` + `server-url-invalid.test.ts` (349 pass, 2 environment failures: root can bind port 1003, egress proxy), `node-net-server.test.ts`, `serve-epoll-add-fail.test.ts`, `test/internal/source-lints/` (171 pass), node `test-dgram-bind*`, `test-dgram-connect*`, `test-dgram-custom-lookup`, `test-dgram-error-message-address`, `test-net-listen-error`, `test-http-listening`. `socket.test.ts` and `node-net.test.ts` have the same `localhost` dual-stack failures on the released bun in this container. </details> <!-- robobun:evidence:begin --> --- **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/bun/udp/udp_socket.test.ts, test/js/bun/udp/dgram.test.ts, test/js/bun/http/serve-listen.test.ts <!-- robobun:evidence:end -->
Problem
connect()failure behindBun.udpSocket({ connect })is thrown as a DNS error:Bun.udpSocket({ connect: { hostname: "::1", port: 1 } })rejects withDNSException: connect ENOTFOUND ::1(code: "ENOTFOUND"). The name resolved fine; it wasconnect()that failed. Linux reports the real reason,EAFNOSUPPORT(the default socket is IPv4). Same misreport forWSAEADDRNOTAVAIL(connectto0.0.0.0),WSAEFAULT,WSAENETUNREACH, and so on. Reproduces with the 1.4.0 release and a canary at eabb96d.bsd_connect_udp_socket(packages/bun-usockets/src/bsd.c:1831) returns-1whenconnect()fails and a getaddrinfo code when resolution fails.udp_socket.rsran the-1througherrno_sys()first (src/runtime/socket/udp_socket.rs:50), whose Windows branch returnsNonefor every non-zero value, so the-1went on toc_ares::Error::init_eai(), whose Windows fallback arm (src/cares_sys/c_ares.rs:1797) isENOTFOUND.connect()method thatnode:dgram'ssocket.connect()calls (js_connect,udp_socket.rs:1919) has the same two-namespace return value and handled neither:-1became a bareError: Failed to connect socketwith nocodeon every platform (Node givesconnect ENETUNREACH 224.0.0.1:1withcode,syscall,address,port), and a getaddrinfo status was not checked at all, so the socket was marked connected.Fix
check_connect()inudp_socket.rsinterprets the status for both callers:-1reads the socket error (WSAGetLastError()on Windows,errnoelsewhere); any other non-zero value is a getaddrinfo code and keeps going throughinit_eai. The Winsock read is the oneget_us_error()(the setsockopt and send wrappers) already did, split out aslast_socket_error()so both share it;errno_sys()is removed, its only other caller wasget_us_error's POSIX branch.bsd_connect_udp_socketcaptures theconnect()error beforefreeaddrinfo()and re-installs it, so the value the caller reads is the oneconnect()produced (and an empty result list reportsEAFNOSUPPORTinstead of a stale value).node:dgramdecorates the error like Node'sExceptionWithHostPort(connect EAFNOSUPPORT ::1:1,.address,.port,.syscall), reusing the helper the send path already had for this (decorateSendError, nowdecorateHostPortError).-1/ getaddrinfo split is the contract of the function being called; reading-1as a getaddrinfo code was never valid, it just happened to be masked on POSIX becauseerrno_sysclaimed the-1first. Fornode:dgram, the resulting shape is Node's (thecodeis the kernel's: Node reports the family mismatch asEINVALbecause it parses the literal itself; kernel-level failures such asENETUNREACHmatch Node exactly).test/js/bun/udp/udp_socket.test.ts(connect option failures) andtest/js/bun/udp/dgram.test.ts(connect() failure ...). Against the unfixed canary: on Windows x64, 2udp_sockettests fail (DNSExceptioninstead ofError) and bothdgramtests fail (codeundefined); on Linux, bothdgramtests fail and theudp_sockettests pass, since that path was already correct there (the Windows-only0.0.0.0->EADDRNOTAVAILcase is the second distinct Winsock code, showing the code is read from the failed call). With the fix:udp_socket.test.ts208 pass on Linux, 209 on Windows;dgram.test.ts63 pass on Linux, 50 pass / 13 pre-existing skips on Windows.test-dgram-connect*,test-dgram-send-*error*,test-dgram-error-message-address,test-dgram-custom-lookup,test-dgram-blocklistnode tests, andudp_socket_recv_flags.test.ts.socket.connect()innode:dgramnow fails (with aDNSException) when the address it is handed does not resolve, where it previously reported success.node:dgramresolves names before calling it, so this is only reachable through a customlookupreturning something unparsable.bsd_connect_udp_socketand establishes the same errno contract, so whichever lands second takes that version of the function).Background
getaddrinfo()reports failures through its return value (EAI_*codes, positive on macOS and Windows, negative on glibc), not througherrno.connect()returns-1and reports througherrno, orWSAGetLastError()on Windows, where the CRTerrnois not touched by Winsock calls.bsd_connect_udp_socketresolves and then connects, so its singleintreturn carries both kinds of result and-1is the only value that means "look at the socket error".init_eaimaps anEAI_*code to the c-ares error Bun uses for DNS exceptions; on Windows any value it does not recognise becomesENOTFOUND, which is how a-1turned into a DNS error.ExceptionWithHostPortis the error Node'sdgrambuilds for bind/send/connect failures: message<syscall> <code> <address>:<port>pluscode,errno,syscall,address,portproperties. Bun'sdgram.tskeeps the native error (itscodeis already platform-correct) and sets those fields on it.::1is a deterministic, offline failure usable as a fixture (Linux and Windows sayEAFNOSUPPORT; the BSDs may check the sockaddr length first and sayEINVAL, so the tests accept either).