Conversation
|
Updated 3:51 AM PT - Aug 12th, 2026
✅ @robobun, your commit 6e2cc0ceb2211a4e8941650bd51d9517c39e84e9 passed in 🧪 To try this PR locally: bunx bun-pr 37690That installs a local version of the PR into your bun-37690 --bun |
|
Warning Review limit reached
Next review available in: 9 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 (15)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it threads an FFI ABI change (uws_listen_handler gains a parameter) through four layers (usockets C → uWS C++ → C shim → Rust) and refactors the TemplatedApp::listen overloads, a human look would still be worthwhile.
What was reviewed:
- All call sites of
bsd_create_listen_socket,us_socket_group_listen,SocketGroup::listen, anduws_listen_handlerare updated in lockstep; the h3 listen path uses a separate closure signature and is unaffected. c_ares::Error::init_eai(0)returnsNoneon all platforms, so the existing errno paths (EACCES/ENOSPC/EADDRINUSE, unix sockets) inon_listen_failedandListener::listenstill fire; on Windows the raw WSA getaddrinfo code hitsinit_eai's catch-allENOTFOUNDarm.- The four collapsed
TemplatedApp::listenoverloads preserve the empty-host →nullptrmapping and thehttpContext == nullptrguard (dnsError stays 0).
Extended reasoning...
Overview
This PR fixes error reporting when Bun.serve / Bun.listen (and therefore node net/tls/http/https servers) are given a hostname that fails to resolve. Previously the error was whatever stale errno the resolver happened to leave behind (EAGAIN/ENOENT/EMSGSIZE on Linux) or a generic EADDRINUSE / code-less "Failed to listen" elsewhere. The fix threads a new dns_error out-parameter carrying the raw getaddrinfo(3) return code from bsd_create_listen_socket up through us_socket_group_listen, HttpContext::listen, TemplatedApp::listen, the uws_listen_handler C shim, and into the Rust on_listen / on_listen_failed callbacks, where it is mapped through the same c_ares::Error::init_eai + system_error_with_syscall_and_hostname helpers the connect/fetch/dns paths already use.
The App.h change also collapses four near-identical listen overloads into a shared listenTcp helper. Nine new tests cover Bun.serve (http/https), Bun.listen (tcp/tls), and node net/tls/http/https servers, all using an over-length DNS label so no network is contacted.
Security risks
None identified. This is error-reporting plumbing on a failure path; no new user input reaches allocation, no bounds arithmetic, no auth/crypto/permission logic.
Level of scrutiny
High. The change is well-contained conceptually but spans an FFI callback signature change coordinated across C, C++, a C-ABI shim, and Rust extern declarations — a mismatch anywhere is silent stack corruption. It also touches the Bun.serve and Bun.listen error-handling paths, which are production-critical, and refactors uWS TemplatedApp::listen overloads. I traced every declaration/definition/caller of the affected symbols and found them consistent, and verified the App.h refactor preserves the empty-host and null-httpContext behaviors, but a maintainer familiar with the usockets/uWS layering should confirm.
Other factors
init_eai(0)returnsNoneon both the Windows and POSIX branches, so the new DNS branch is only taken whengetaddrinfoactually failed; the existing Linux errno special-cases (EACCES, epoll ENOSPC) and the fallback EADDRINUSE message are untouched.- On Windows,
bsd_create_listen_socketcalls nativegetaddrinfo(returning WSA codes, notUV_EAI_*); the Windows arm ofinit_eaihas a catch-all_ => Some(Error::ENOTFOUND)for non-zero codes, so the tests' expectedENOTFOUNDholds there too. - The
Listener.rsDNS branch is inside the existingcleanupscopeguard, so the half-built listener is torn down on the new early return. - The unix-socket listen paths (
us_socket_group_listen_unix,uws_listen_domain_handler,on_listen_unixtrampoline) correctly do not carrydns_error; the trampoline forwards a hardcoded 0. - Test coverage is thorough (both native paths × both TLS variants × node compat surface), hermetic (RFC 1035 over-length label), and asserts exact error shape rather than just "throws".
|
Status: ready for review. CI is green on the current revision (build 93041, all lanes; build 92679 on the first revision was green too), all review threads are resolved, and the automated reviews found nothing to change. Reproduced on Linux with the release build ( The new tests in |
f59135f to
228bb23
Compare
228bb23 to
8101485
Compare
bsd_create_listen_socket() returned LIBUS_SOCKET_ERROR when getaddrinfo() failed without recording why: getaddrinfo reports failures through its return value, so errno was left holding whatever the resolver last set. Bun.serve on Linux then built its error from that stale errno (EAGAIN, ENOENT, EMSGSIZE, depending on the input) attributed to listen(2), Bun.serve elsewhere claimed the port was in use, and Bun.listen (so also net/tls servers) threw a code-less "Failed to listen at <host>". Carry the getaddrinfo return code out of usockets in its own out-param, separate from the errno one because the two number spaces overlap, and pass it to the uws listen callback. Bun.serve and Bun.listen map it the same way Bun.connect and fetch already do, so the TCP listen APIs (and the node servers built on them) report `getaddrinfo ENOTFOUND <host>` with code, syscall and hostname set, which is also the error Node emits for server.listen() on a host that does not resolve. The UDP bind path (Bun.udpSocket, HTTP/3-only servers) has its own encoding of the same failure and is left for a separate change.
8101485 to
6e2cc0c
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it threads an FFI ABI change (the uws_listen_handler callback signature and us_socket_group_listen out-param) through C, C++, and Rust and refactors the TemplatedApp::listen overloads, a human look at the plumbing would still be worthwhile.
Checked: the new dns_error param is zero-initialized at every caller (App.h listenTcp, Listener::listen) and only written on getaddrinfo failure, so init_eai(0) → None keeps the existing errno paths (EACCES/ENOSPC/EADDRINUSE/unix) untouched. The Rust on_listen trampoline signature matches the updated C typedef, and on_listen_unix correctly forwards 0. The App.h refactor preserves the empty-host → nullptr fallthrough. Tests are hermetic (64-byte DNS label rejected locally) and cover both native paths (Bun.serve and Bun.listen).
Extended reasoning...
Overview
This PR fixes error reporting for Bun.serve/Bun.listen (and the net/tls/http/https servers built on them) when the listen hostname fails to resolve. Previously the failure surfaced as a stale errno (EAGAIN/ENOENT/EMSGSIZE on Linux) or a generic EADDRINUSE/"Failed to listen" on other platforms. The fix threads the raw getaddrinfo return code through a new dns_error out-parameter from bsd_create_listen_socket (bsd.c) → us_socket_group_listen (context.c/libusockets.h) → HttpContext::listen/TemplatedApp::listen (App.h) → the uws_listen_handler C shim (libuwsockets.cpp/_libusockets.h) → Rust's SocketGroup::listen/App::listen and the on_listen trampoline → on_listen_failed (server/mod.rs) and Listener::listen (Listener.rs), which map it via the existing c_ares::Error::init_eai + system_error_with_syscall_and_hostname helpers to a Node-compatible getaddrinfo ENOTFOUND <host> error. Nine new tests cover http/https (Bun.serve), tcp/tls (Bun.listen), and net/tls/http/https (node compat) with a hermetic 64-byte DNS label.
Security risks
None identified. The change only affects error-reporting on the listen-failure path; no new user input reaches parsing or allocation, and the getaddrinfo code is passed through a fixed mapping table already used by the connect path.
Level of scrutiny
High. This is a cross-language FFI change: it alters a C-ABI callback signature (uws_listen_handler gains an int parameter) and a public C function signature (us_socket_group_listen gains a nonnull out-param), refactors four TemplatedApp::listen C++ overloads into a shared listenTcp helper, and touches the core listen path for both Bun.serve and Bun.listen. An ABI mismatch between the Rust trampoline and the C typedef would only surface at runtime. While I verified the signatures line up and the refactor preserves behavior (empty host → nullptr, options default 0), this is exactly the kind of change where a second pair of eyes on the FFI plumbing is warranted.
Other factors
CI was green on the first revision (build 92679, all lanes); subsequent pushes only trimmed comments per the comment-cop bot, and all those threads are resolved. The tests are well-constructed: hermetic (over-long DNS label rejected locally, no network), exercise both native paths and both TLS variants, verify a subsequent listen still works after a failure, and destructure the error object to assert exact code/syscall/hostname/message rather than just toThrow(). I confirmed init_eai(0) returns None on every platform (including the Windows fallthrough), so the new branch cannot fire when getaddrinfo succeeded. The _hostname → hostname rename in on_listen_failed is because the field is now read outside the Linux-only cfg block.
|
Another report of the same bug came in from the work on #32628. The bug is still present on main at db059d8. This branch now conflicts with main in |
|
Heads-up from #40986, which adds a If #40986 lands first, the 64-byte label these tests use is answered in-process with The two changes are independent otherwise: #40986 does not touch |
#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
Bun.serveon a hostname that does not resolve throws the wrong error. On Linux it is whatever errno the resolver left behind, blamed onlisten(EAGAIN,ENOENTorEMSGSIZE, varying with the input). On macOS and Windows it isFailed to start server. Is port 0 in use?withcode: "EADDRINUSE"(thesomething.localhostreport in Bun.serve failed to start on localhost subdomain #25765).Bun.listen, and sonetandtlsservers, throw a bareError: Failed to listen at <host>with nocode.httpandhttpsservers emit the stale errno error.getaddrinfowithout recording why.getaddrinforeturns its error instead of setting errno, soBun.servereads a stale errno andBun.listenreads an out-param that was never written.Fix
dns_error, carrying the rawgetaddrinfocode up to the listen callback.Bun.serveandBun.listenmap it with the helpersBun.connectandfetchalready use, so both throwgetaddrinfo ENOTFOUND <host>withcode,syscallandhostname, the shape Node'sserver.listen()and Bun's connect side already produce.errorbecause the two number spaces overlap (glibcEAI_NONAMEis -2, macOS's is 8, Windows returns WSA codes), the same reason the connect path tagserror_is_dns.dns_erroris written only whengetaddrinfoitself fails, before any socket call, and zero maps to no error.Bun.udpSocket(HTTP/3-only servers) encodes the same failure differently and is left for a separate change.Bun.servehttp/https,Bun.listentcp/tls,net/tls/http/httpsserver.listen()) fail on the unpatched build and pass with this change. They use a 64-byte DNS label, rejected locally, so no network is involved.Background
getaddrinfo(3)is the libc call that turns a hostname into socket addresses. It reports failure through itsEAI_*return value and does not set errno, so errno after a failed call is leftover resolver state.EAI_*numbering differs per platform.packages/bun-usockets) is Bun's C socket layer; uWS (packages/bun-uws) is the HTTP layer on it.Bun.servereaches usockets through uWS and a C shim,Bun.listencalls usockets directly, hence two Rust call sites plus the plumbing between.init_eaiconverts a platformEAI_*code into Bun's DNS error (ENOTFOUNDforEAI_NONAME/EAI_NODATA, and for any failure on Windows) and returnsNonefor 0.system_error_with_syscall_and_hostnamebuilds the Node-shaped error withcode,syscallandhostname.net,tls,httpandhttpsservers resolve the host withdns.lookupbefore binding, so on an unresolvable host they emit the lookup error itself.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/http/serve-listen.test.ts test/js/node/net/node-net-server.test.ts
Original description
Repro
bun -e 'try { Bun.serve({ hostname: "not.a.real.host.invalid", port: 0, fetch() {} }) } catch (e) { console.log(e.code, e.message) }'On Linux this prints a different error depending on what the resolver happened to do internally:
On macOS and Windows the same call throws
Failed to start server. Is port 0 in use?withcode: "EADDRINUSE"(thesomething.localhostreport in #25765 is this).Bun.listenwith the same hostname, and thereforenet/tlsservers, throw a bareError: Failed to listen at <host>with nocodeat all;http/httpsservers emit the stale errno error above.Cause
bsd_create_listen_socket()inpackages/bun-usockets/src/bsd.creturnsLIBUS_SOCKET_ERRORwhengetaddrinfo()fails without recording why.getaddrinforeports failure through its return value, not errno, so at that point errno is whatever glibc's resolver last left behind.Bun.serve'son_listen_failed(src/runtime/server/mod.rs) reads errno on Linux and turns it into alistenerror; the other platforms andBun.listen(src/runtime/socket/Listener.rs, which reads the*errorout-param that was never written) fall through to their generic messages.Fix
bsd_create_listen_socket/us_socket_group_listengain a second out-param,dns_error, which receives the rawgetaddrinforeturn code. It is a separate parameter rather than a value in*errorbecause the two number spaces overlap (glibc'sEAI_NONAMEis -2, macOS's is 8, Windows returns WSA codes), the same reason the connect path tagsus_connecting_socket_t::error_is_dns.uWS::TemplatedApp::listenand theuws_listen_handlerC shim pass it along to the listen callback, soBun.servegets it directly instead of inferring anything from errno.Bun.serveandBun.listenmap it withc_ares::Error::init_eaiandsystem_error_with_syscall_and_hostname, the same helpersBun.connectandfetchalready use for a failed lookup.Bun.serve(http and https, including the TCP side ofhttp3: trueservers),Bun.listen, and everything built on them (net,tls,http,https,http2servers,bun ./index.html --host,--inspect) now produce:This is the shape Node produces for
server.listen()on a host that does not resolve (net,tls,httpandhttpsall go throughdns.lookupthere), and it is what Bun already produces for the connect side of the same failure, so the listen side is now consistent with both. The code mapping (ENOTFOUNDforEAI_NONAME/EAI_NODATA, and on Windows for any failure; c-ares numbering inerrno) isinit_eai's and is shared withBun.connect/fetch, not introduced here.The existing errno paths (EACCES, the epoll
ENOSPCcase, EADDRINUSE, unix sockets) are untouched:dns_erroris only written whengetaddrinfoitself fails, before any socket call, andinit_eai(0)isNoneon every platform, soon_listen_failedandListener::listenonly take the new branch in that case.Out of scope, same family:
Bun.udpSocket(and so HTTP/3-only servers withhttp1: false) go throughbsd_create_udp_socket, which encodes thegetaddrinfocode differently (bind ENOENT <host>today); that is a separate consumer and is left for its own change. A bracketed hostname longer than 1024 bytes crashesBun.servebefore reaching any of this; that is #37631, which is why the tests below exercise brackets and an over-long name separately.Verification
New tests use a 64-byte DNS label, which every resolver rejects locally, so they do not touch the network:
test/js/bun/http/serve-listen.test.ts:Bun.servehttp and https, plus the three kinds of input above producing the same error, plus a laterBun.servestill working.test/js/bun/net/socket-dns-error.test.ts:Bun.listentcp and tls, repeated three times.test/js/node/net/node-net-server.test.ts:net,tls,httpandhttpsserver.listen()emit the resolver error (net/tls useBun.listen, http/https useBun.serve).With
USE_SYSTEM_BUN=1the nine new tests fail (stale errno or missingcode); with this change they pass.serve.test.ts,serve-epoll-add-fail.test.ts(which pins the errno channels this change leaves alone),serve-http3.test.ts,socket.test.ts,node-net.test.tsand the Nodetest-net-*listen*/bind-twicetests show no new failures compared to the unpatched build. Full CI was green on the first revision; later revisions only changed comments.Noticed while working on #37631.