Bun.serve: close listen socket before destroying app in deinit()#32543
Bun.serve: close listen socket before destroying app in deinit()#32543robobun wants to merge 2 commits into
Conversation
When Bun.serve({ tls, http3: true }) binds TCP successfully but the
UDP bind for QUIC fails on the last retry attempt, listen() throws
and tears the server down via NewServer::deinit(). The TCP listen
socket is still linked in the HttpContext socket group at that point,
so ~TemplatedApp -> httpContext->free() -> us_socket_group_deinit()
aborts on the head_listen_sockets != NULL assertion (debug/ASAN) or
leaks the bound listen socket and its fd (release).
Close any remaining TCP/H3 listen socket in deinit() before destroying
the uws app. This matches what stop_listening() would have done and
covers every synchronous deinit() caller in the listen() error path.
|
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 (2)
Walkthrough
ChangesH3 Listener Leak Fix and Regression Test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 11:53 AM PT - Jun 20th, 2026
✅ @robobun, your commit c77d8ae09684eeb442597ae474e525d46dd5cad8 passed in 🧪 To try this PR locally: bunx bun-pr 32543That installs a local version of the PR into your bun-32543 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Same crash as #32539 / #32542 but fixed at a different layer: those close the TCP listener inside the H3 retry loop (at the one call site that leaves it orphaned today); this one closes it inside The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fuzzer-found crash fix that mirrors stop_listening()'s cleanup, with a deterministic regression test.
Extended reasoning...
Overview
This PR adds listen-socket cleanup to NewServer::deinit() in src/runtime/server/mod.rs (~11 lines) so that any still-linked TCP/H3 listen socket is closed before the uws app is destroyed. It also adds a regression test in test/js/bun/http/serve-http3.test.ts that occupies a UDP port, calls Bun.serve({ tls, http3: true, port }), asserts a clean throw, and verifies the TCP port is released by re-binding it.
Correctness
The new .take() + opaque_deref_mut(...).close() calls are byte-for-byte the same idiom stop_listening() already uses for these handles (lines ~1533 and ~1571). In the normal lifecycle (stop_listening → schedule_deinit → deinit), both listener and h3_listener are already None, so the added blocks are no-ops; they only fire on the synchronous Self::deinit(this) listen-failure paths in listen(), which is exactly the bug surface. .take() makes the close idempotent and ordering (close listener before NewApp::destroy) matches the PR description's invariant about us_socket_group_deinit's head_listen_sockets == NULL assert.
Security risks
None. This is teardown ordering on an error path — no input parsing, auth, crypto, or trust-boundary changes. If anything it removes a release-build hazard (leaked fd with a dangling accept_group pointer).
Level of scrutiny
Low-to-moderate. Small, mechanical, follows established patterns in the same file, and is covered by a precise subprocess test that checks both the debug-assert path (no SIGABRT, signalCode: null) and the release-leak path (TCP rebind succeeds). No CODEOWNERS entry covers these files.
Other factors
No prior reviewer comments to address; bug-hunting system found nothing. The fuzzer fingerprint and before/after repro in the description are concrete.
There was a problem hiding this comment.
I didn't find any issues with the change itself — the take() + close() pattern is idempotent with stop_listening() and the ordering before NewApp::destroy() is correct — but with #32539 / #32542 fixing the same crash at a different layer, a human should pick which approach lands.
Extended reasoning...
Overview
This PR adds defensive cleanup in NewServer::deinit() (src/runtime/server/mod.rs:1855-1871): before destroying the uws h3_app and app, it now take()s and close()s any remaining h3_listener / listener. This fixes a fuzzer-found crash where TCP listen succeeds but the HTTP/3 UDP bind fails on the last retry, leaving the TCP listen socket linked in the HttpContext socket group when ~TemplatedApp runs (debug assert / release leak). A regression test in test/js/bun/http/serve-http3.test.ts deterministically reproduces this by occupying a UDP port and verifying the TCP port is released afterward.
Security risks
None identified. This is teardown ordering on an error path; no auth, crypto, or untrusted-input parsing is touched. The added close() calls use the same bun_opaque::opaque_deref_mut(...).close() pattern already used in stop_listening() (lines 1533, 1571), and Option::take() guarantees no double-close when stop_listening() already ran.
Level of scrutiny
Medium-high. The diff is small and mechanically mirrors existing code, but it sits in the server lifecycle/FFI destruction path where ordering bugs manifest as UAF or fd leaks. I traced the call sites: in the normal path, stop_listening() has already take()'d both listeners so deinit()'s new blocks are no-ops; in the synchronous Self::deinit(this) listen-failure callers (e.g. lines 2491, 2520, 2563, 2845), stop_listening() never ran, which is exactly the case this covers.
Other factors
The main reason I'm not approving is that two sibling PRs (#32539, #32542) fix the same crash by closing the listener inside the H3 retry loop instead of in deinit(), and the author has explicitly asked which approach is preferred. That's a maintainer decision (centralized-in-deinit vs. close-at-the-call-site), not something a bot should resolve by approving one of the three.
1 participant
Found by fuzzer (fingerprint
777577b59557fdfd).What
When
Bun.serve({ tls, http3: true })binds TCP successfully but the UDP bind for QUIC fails on the last retry attempt,listen()throws and tears the server down viaNewServer::deinit(). At that point the TCP listen socket is still linked in the HttpContext socket group, so~TemplatedApp -> httpContext->free() -> us_socket_group_deinit():head_listen_sockets != NULLassertion in debug/ASAN buildsls->accept_grouppointing into freed memory)The fuzzer hit this via
{ port: 0, http3: <truthy>, serverName: "..." }(serverName alone promotes to an HTTPS server, so HAS_H3 is true); under REPRL load enough UDP ports get occupied that the 3-attempt retry loop eventually exhausts.Fix
Close any remaining TCP/H3 listen socket in
deinit()before destroying the uws app. This matches whatstop_listening()would have done and covers every synchronousSelf::deinit(this)caller in thelisten()error path.Repro
Deterministic: occupy a UDP port, then
Bun.serve({ tls, http3: true, port: <that port> }). Since the port is non-zero,max_attempts = 1; TCP listen succeeds, UDP bind fails, exception is thrown,deinit()runs with the listener still linked.Before:
After: clean
Failed to listen on UDP port N for HTTP/3error, and the TCP port is released (verified by re-binding it in the test).