Conversation
…s socket A request head that arrives split over two reads is parsed out of the HTTP parser's per-socket fallback buffer, and the uWS HttpRequest the dispatch holds views into that buffer. server.stop(true) inside the handler closes the request's own socket right there, and HttpContext::onClose destructs the parser with its buffer. Everything that materialises the headers afterwards read freed memory: req.headers inside the handler, and the url/header snapshot the server takes when an async handler ends the dispatch. onClose now hands the buffer to the parse frame in onData, which owns it until it returns, after the last dispatch that can view it.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesFallback buffer lifetime
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
StatusReproduced on main (7e56b40) and on bun 1.4.3. The request head arrives split over two reads, so it is parsed in place out of the parser's fallback buffer, and the buffer is freed while the request still views it. Three cases in
With this branch all three return the intact url and headers, and ASAN is clean. Run: The fix moved after a self-review. The first version hooked |
![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's a memory-safety fix in the uWS HTTP hot path whose correctness rests on std::string move semantics keeping the heap block address stable across libc++/libstdc++/MSVC, a human look would still be worthwhile.
What was reviewed:
- Traced
onDatabetween theparsedFallbackHolder = &parsedFallbackset and its restore — no outer-frame early returns exist in that span (thereturns are inside the parser lambdas), so the context pointer can't dangle. - Checked the SSO argument:
consumePostPaddedreservesfallback.length() + maxCopyDistance + max(MINIMUM_HTTP_POST_PADDING=32, sizeof(std::string)), which exceeds every stdlib's SSO threshold, so the moved buffer is always heap-backed when views point into it. - Confirmed the handover in
onCloseruns before~HttpResponseData(), is gated byparsingSocket == s, and move-assigns into an empty destination so no prior block is freed. - Test follows harness conventions (subprocess with
-e,port: 0, concurrent pipe drain, awaits the/barrierresponse as the observable park signal rather than sleeping,Buffer.allocover.repeat, stdout asserted before exit code).
Extended reasoning...
Overview
This PR fixes a heap-use-after-free in packages/bun-uws/src/HttpContext.h / HttpParser.h: when an HTTP request head arrives split across two reads it is parsed out of the parser's per-socket fallback std::string, and the dispatched HttpRequest holds string_views into that buffer. If the handler calls server.stop(true) (or otherwise closes its own socket) during dispatch, onClose destructs the parser and frees the buffer while those views are still live. The fix adds HttpParser::takeFallbackBuffer() and a parsedFallbackHolder pointer on HttpContextData that, during a parse, points at a stack-local std::string in the onData frame; onClose moves the buffer into that holder before destructing the parser, so the bytes survive until the parse frame unwinds. A new two-variant subprocess test in serve-pending-promise-abort-leak.test.ts reproduces the split-head + stop(true) scenario for both the lazy-req.headers path and the async-handler snapshot path.
Security risks
This is remotely reachable HTTP parsing, and the pre-fix behaviour is a UAF read of freed memory that surfaces as garbage header names/values (or an ASan abort). The fix does not introduce new parsing of untrusted input; it only extends the lifetime of an existing buffer to match the frame that already borrows from it. I checked that the new context-level pointer cannot dangle: it is set after parsingSocket and restored alongside it, and there are no early returns at the outer onData scope between the set and the restore (all intervening returns are inside the request/data lambdas). The onClose handover is guarded by parsingSocket == s, so a close of an unrelated socket during a nested parse does not touch the outer frame's holder. The move-assignment target is a freshly default-constructed std::string, so no previously-held block is freed by the assignment.
Level of scrutiny
High. This is C++ memory management in the server's per-read hot path, templated over both SSL and IsNodeHttp, and correctness depends on a non-obvious property: that moving a heap-backed std::string transfers the heap block by pointer (so existing string_views into it stay valid) rather than copying into a new allocation. That holds on every stdlib Bun ships against, and the reserve in consumePostPadded (at least 32 bytes past current length) guarantees the buffer is heap-backed whenever views point into it — but it is exactly the kind of reasoning REVIEW.md flags for a second pair of eyes. The PR description also explicitly scopes out two adjacent hazards (nested onData on the same socket, and loop-receive-buffer aliasing) as separate issues, which a maintainer should confirm they're comfortable leaving for follow-up.
Other factors
The implementation cleanly mirrors the existing parsingSocket / prevParsingSocket save-and-restore pattern in the same function, so re-entrancy handling is consistent with established code. Per-read cost is two pointer stores plus one empty std::string on the stack; the actual move only happens on the rare in-dispatch close. The test is well-constructed: it deterministically parks a partial head in the fallback buffer by piggy-backing it on a /barrier request and awaiting that response (an observable condition, not a sleep), then churns native allocations so a non-sanitizer build observably corrupts the header values, and asserts exact header content and an empty stderr before the exit code. No findings were reported by the bug hunt and I found nothing to add, but the change is neither simple nor mechanical, so deferring rather than approving.
…h limit The helper that churns native allocations passed names of up to 1192 bytes to Bun.resolveSync. macOS caps a path at 1024 bytes, and a resolve whose path plus the probed extension passes that cap panics in the resolver, so the child process crashed on both darwin lanes. Names now stop at 760 bytes, which still covers the size of the freed block. The two cases also run concurrently.
A request head that arrives split over two reads is parsed in place out of HttpParser::fallback, and the dispatched uWS HttpRequest holds views into it. consumePostPadded now moves that buffer into a frame-local string for the dispatch and puts it back on the short-read path, so the frame that creates the views owns the bytes. This covers every site that destructs the parser mid-dispatch (onClose, HttpResponse::upgrade, the HTTP/2 handoff), not only the close, and it adds no work to a read that carries a whole head. The onClose hook, the HttpContextData holder and takeFallbackBuffer() are gone. The new test case needs no server API call: a split head plus a declared body that never arrives plus Connection: close closes the socket from the response completion inside the dispatch.
|
Updated 3:15 PM PT - Sep 21st, 2026
✅ @robobun, your commit e09f98388e5c4eebea1cb56daeef66a94c33f420 passed in 🧪 To try this PR locally: bunx bun-pr 42789That installs a local version of the PR into your bun-42789 --bun |
There was a problem hiding this comment.
No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
… upgrade request (#43153) ### Problem - A client sends frames in the same TCP read as the upgrade request. `Bun.serve` answers 101 and never sees them: `server saw: ["later"] ping events: 0 pongs on the wire: 0`. A frame cut by the read closes the connection later. - After `server.upgrade()`, `HttpContext::onData` (`packages/bun-uws/src/HttpContext.h:560`) stops the HTTP parser, sends the 101 and returns the WebSocket (line 746). Nothing parses the rest of the read. ### Fix - `consumePostPadded` reports how much of the read it used. `onData` sends the 101, then gives the rest to the WebSocket with `us_dispatch_data`, in the same call. It stores no bytes. A declared request body is never handed over. - This matches `ws` on Node, whose upstream test now passes. A `server.upgrade()` in a later event-loop turn still gets `400 Bad Request` (#43149 tracks the `ws` shim). - Only for the socket of this read: `upgradedWebSocket` can name another connection's WebSocket. Without the check, a test shows one connection's bytes in another's `message` handler. - Verified: `test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts` (8 of 13 fail before), three tests in `test/js/first_party/ws/ws.test.ts`, plus the websocket, `serve.test.ts` and node:http upgrade suites. Self-reviewed (Notes). ### Background - `HttpResponse::upgrade` destroys the socket's HTTP state, moves the socket into the WebSocket context in place, and sets `upgradedWebSocket` so that `onData` sees the change. - A request head that spans two reads is parsed from the parser's `fallback` buffer, which the upgrade frees. So the parser reports an offset, not a pointer. - `us_dispatch_data` is how the event loop delivers read bytes to a socket. Its first C++ caller needs `extern "C"`. <details><summary>Notes</summary> **Repro** (bun only). Before: `server saw: ["later"] ping events: 0 pongs on the wire: 0`. After: `server saw: ["early","later"] ping events: 1 pongs on the wire: 1`. ```js const net = require("node:net"); const crypto = require("node:crypto"); function frame(opcode, payload) { const p = Buffer.from(payload), mask = crypto.randomBytes(4); const out = Buffer.alloc(6 + p.length); out[0] = 0x80 | opcode; out[1] = 0x80 | p.length; mask.copy(out, 2); for (let i = 0; i < p.length; i++) out[6 + i] = p[i] ^ mask[i % 4]; return out; } const seen = []; let pings = 0; const server = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch(req, server) { if (server.upgrade(req)) return; return new Response("no", { status: 400 }); }, websocket: { message(ws, msg) { seen.push(String(msg)); if (String(msg) === "later") ws.close(1000); }, ping() { pings++; }, }, }); const key = crypto.randomBytes(16).toString("base64"); const upgrade = `GET / HTTP/1.1\r\nHost: 127.0.0.1:${server.port}\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: ${key}\r\nSec-WebSocket-Version: 13\r\n\r\n`; const c = net.connect(server.port, "127.0.0.1", () => { c.write(Buffer.concat([Buffer.from(upgrade), frame(0x1, "early"), frame(0x9, "p")])); }); let got = Buffer.alloc(0), sentLater = false; c.on("data", d => { got = Buffer.concat([got, d]); if (!sentLater && got.includes("\r\n\r\n")) { sentLater = true; c.write(frame(0x1, "later")); } }); c.on("close", () => { const body = got.subarray(got.indexOf("\r\n\r\n") + 4); let pongs = 0; for (let i = 0; i < body.length; ) { if ((body[i] & 0x0f) === 0xa) pongs++; i += 2 + (body[i + 1] & 0x7f); } console.log("server saw:", JSON.stringify(seen), "ping events:", pings, "pongs on the wire:", pongs); server.stop(true); }); ``` **Who sees this.** No user reported it. RFC 6455 section 4.1 tells a client to wait for the 101, and browsers do. A client that writes the request and the first frame back to back sees it only when TCP puts both in one read, so it loses frames some of the time and gets no error. gorilla/websocket rejects such a client on purpose. The `ws` package on Node parses the frames. This PR takes the `ws` behavior, because an open connection that lost data with no signal is the worst of the three. **Where the boundary is.** The fix applies when `server.upgrade()` runs before the dispatch of the request returns to the parser. That includes an `async` handler whose awaits need no new turn of the event loop (`await Promise.resolve()`, `await req.text()` on a GET). Two tests cover that. After a timer or I/O, the read is over. The parser has then read the frame bytes as a pipelined request, `getHeaders` fails, and uWS writes `HTTP/1.1 400 Bad Request` with `Connection: close`. `server.upgrade()` then returns `false`. A test pins this. One exception is left as it is: early bytes that can still begin a request line (for example the single byte `0x41`) wait in the parser's buffer, and the upgrade frees that buffer. **`ws` shim.** #43149 has the full table. A `handleUpgrade()` inside the 'upgrade' event takes the fixed path. A `handleUpgrade()` in a later task, a `verifyClient` that answers later, and frames that arrive in a read of their own before a deferred `handleUpgrade()` still lose the frames: they are in `head` or in the socket's stream, and `src/js/thirdparty/ws.js` has no way to give bytes to the native WebSocket. That needs a design decision, so it has three `it.todo` tests and the issue. The new test `handles data passed along with the upgrade request` is a port of the test of the same name in websockets/ws `test/websocket-server.test.js`. **The check on the socket.** `upgradedWebSocket` is one field per HTTP context. `HttpResponse::upgrade` sets it when any socket of the context is in `onData`. Two ways lead to a value that belongs to another connection. (1) A handler of connection A resolves a promise of connection B, and B's `server.upgrade()` runs in the microtask checkpoint of A's dispatch. The test `never reach the WebSocket of another connection` covers this: with the check removed in a local build, the `message` handler of B received the frame that A sent. (2) A `server.upgrade()` from a request body handler (node:http with a body on the upgrade request, `handleUpgrade()` from `req.on("end")`) leaves the field set. I instrumented a build: the next request on another connection saw the stale field in its request handler (`consumed=32 length=44`). #43163 tracks that bug. Both happen on main today, and this PR does not change what main does there. #37463 fixes (1) at its source. The two PRs are independent and work in either order. With #37463, a connection whose synchronous upgrade is followed by another connection's upgrade in the same dispatch also gets its frames. The check has to stay with #37463 too, because of (2). **`upgrade()` adopts in place.** On linux x64, `sizeof(WebSocketData)` is 160 and `sizeof(HttpResponseData<SSL>)` is 224, and `us_socket_adopt` keeps the block when the new ext is not larger. If a future layout makes the adopt move the socket, the check fails, the frames are dropped as before, and the new tests fail. **Request bodies.** An upgrade request can declare a body. Node reads it as the body of the request. `server.upgrade()` never looked at it. On main, the bytes of such a body in the same read are dropped and the WebSocket works. In a later read they go to the WebSocket parser, which closes the connection. This PR keeps both. When the request declared a body (a `Content-Length` above 0, or chunked), the parser returns the count `HttpParserResult::WHOLE_READ` and `onData` hands nothing over. Every other count is the exact end of the request head, also for a head that fills the parser's 16 KiB buffer for split heads (a test covers that size). The first push of this PR handed the body over too, and that closed connections that main keeps open. Three tests in the new file and one in `ws.test.ts` pin it: they pass on main, fail on the first push, and pass now. The count is a named value and not a new field, because `HttpParserResult` is 16 bytes and comes back in two registers. **Cost on the normal path.** One pointer copy at the top of `consumePostPadded`. The other new code runs only after a handler took the socket. The request handler lambda has no new captures: it lives in a `MoveOnlyFunction` with a 16-byte inline buffer, and a larger closure would allocate on every `onData` call. **Earlier work.** #33692 fixed the same bug in July and was closed as stale, with no judgment on the fix. This PR also covers a request head that spans two reads and a read that starts with the rest of another request's body, adds no lambda captures, and has the check on the socket. **Self-review.** Addressed: the description of the failure (a parser that starts in the middle of a frame, not only a drop), the boundary (a turn of the event loop, not an `await`), the scope of "clear failure" (Bun.serve only), the port of the upstream `ws` test, tests for the microtask upgrade and for the body-tail offset, and the `ws` shim gaps (todo tests and #43149). Rejected: to fold #37463 in and drop the check on the socket. #37463 is open and green on its own, and case (2) above needs the check with or without it. **Reentrancy.** A handler that runs the event loop inside itself after `server.upgrade()` (for example `Bun.build` with an async plugin `setup()`) can let a later read reach the WebSocket before these bytes. HTTP reads have the same property (#42794). **Related open PRs in the same files.** #37463 (`HttpResponse.h`, the `isParsingHttp` lines of `HttpContext.h`), #42789 (the fallback return in `HttpParser.h`, a textual conflict only: `had` stays a local there), #39843 (the uncork lines above the new block), #38128, #39802. **Why a new test file.** `websocket-server.test.ts` does not pass as a whole under a debug ASAN build on my machine: its subprocess-client tests time out on main without this change. A run of that file before and after the fix proves nothing. The directory already has one file for each raw-frame topic (`websocket-server-rsv-frames`, `-unmasked-frames`, `-upgrade-reentrant`). **Suites run with the debug build:** the new file (36 runs, all pass), `test/js/first_party/ws/ws.test.ts`, `test/js/bun/websocket/`, `test/js/bun/http/serve.test.ts`, `request-smuggling.test.ts`, `http-server-chunking.test.ts`, `bun-server.test.ts`, `node-http-with-ws.test.ts`, `node-http-req-socket-pause.test.ts`, `node-http-connect.test.ts`. The subprocess-client tests in `websocket-server.test.ts` time out on my machine with and without this change, and pass when run alone. Two `serve.test.ts` tests fail on my machine for reasons of the machine (it runs as root, and its network blocks the external address). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 10 failed, 3 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts test/js/first_party/ws/ws.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: 146 | // One write, so the request and the frames reach the server in one read. 147 | client.socket.write(Buffer.concat([Buffer.from(upgradeRequest), text("early"), ping("p")])); 148 | expect(await client.status()).toBe("HTTP/1.1 101 Switching Protocols"); 149 | client.socket.write(text("later")); 150 | 151 | expect(await client.framesUntil("text:echo:later")).toEqual(["text:echo:early", "pong:p", "text:echo:later"]); ^ error: expect(received).toEqual(expected) [ - "text:echo:early", - "pong:p", "text:echo:later", ] - Expected - 2 + Received + 0 at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:151:57) 169 | 170 | client.socket.write(Buffer.concat([Buffer.from(upgradeRequest), text ... (truncated) release without fix: 4 failed, 3 skipped bun test v1.4.3-canary.1 (becf408) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: 78 | // Only observed through the races below. 79 | failed.promise.catch(() => {}); 80 | socket.on("error", error => failed.reject(error)); 81 | socket.on("close", () => { 82 | closed.resolve(); 83 | failed.reject(new Error("the server closed the socket")); ^ error: the server closed the socket at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:83:25) at emit (node:events:100:22) at <anonymous> (node:net:2350:20) 78 | // Only observed through the races below. 79 | failed.promise.catch(() => {}); 80 | socket.on("error", error => failed.reject(error)); 81 | socket.on("close", () => { 82 | closed.resolve(); 83 | failed.reject(new Error("the server closed the socket")); ^ error: the server closed the socket at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts:83:25) at emit (node:events:100:22) at <anonymous> (node:net:2350:2 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 3 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts test/js/first_party/ws/ws.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-upgrade-early-frames.test.ts: (pass) frames in the same read as the upgrade request > are delivered in order, and a ping gets its pong (tls: false) [738.45ms] (pass) frames in the same read as the upgrade request > are delivered when server.upgrade() runs after `await Promise.resolve()` [381.05ms] (pass) frames in the same read as the upgrade request > are delivered when server.upgrade() runs after `await req.text()` [380.86ms] (pass) frames in the same read as the upgrade request > a frame that the read cuts short is completed by the next read [397.03ms] (pass) frames in the same read as the upgrade request > are delivered in order, and a ping gets its pong (tls: true) [680.67ms] (pass) frames in the same read as the upgrade request > a Content-Length body in the same read as the upgrade request is not parsed as frames [131.43ms] (pass) frames in the same read as the u ... (truncated) release with fix: 3 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 666ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/27] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[0m �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default �[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name` �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41:8 �[1m�[94m|�[0m �[1m�[94m41�[0m �[91m- �[0mname = �[91m"bun_shim_impl"�[0m �[1m�[94m41�[0m �[92m+ �[0mname = �[92m"bun-shim-impl"�[0m �[1m�[94m|�[0m �[1m�[33mwarning�[0m: `bun_shim_impl` (manifest) generated 1 warning �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-usockets/src/internal/internal.h | 6 + packages/bun-uws/src/HttpContext.h | 16 +- packages/bun-uws/src/HttpParser.h | 26 +- .../websocket-server-upgrade-early-frames.test.ts | 344 +++++++++++++++++++++ test/js/first_party/ws/ws.test.ts | 133 +++++++- 5 files changed, 518 insertions(+), 7 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-usockets/src/internal/internal.h 1 2 45 packages/bun-uws/src/HttpContext.h 6 5 46 packages/bun-uws/src/HttpParser.h 3 4 45 …websocket/websocket-server-upgrade-early-frames.test.ts 2 5 30 test/js/first_party/ws/ws.test.ts 2 3 31 ``` </details> <!-- robobun:evidence:end -->
…e-stop-true-split-head-uaf
The three cases spawned bun with an inline -e script. They now live in serve-split-head-stop-fixture.ts and serve-split-head-close-fixture.ts, and the test runs them with bunRun and asserts with toSpawn.
robobun
changed the title
|
Closing: #44210 carries this fix on current main. It has the parser hunk of this pull request (the parse frame owns the reassembly buffer), its three test cases and its two fixtures, and it fixes the |
3 participants
Problem
HttpParser::fallback, and the dispatcheduWS::HttpRequestholdsstring_views into it. Anything that destructs the parser during the dispatch frees that buffer under every lazy reader of the head, including the server's own snapshot into_async(RequestContext.rs:2381). ASAN reportsheap-use-after-free. A release build reads garbage bytes.HttpContext::onClose(HttpContext.h:288): a split head, a declared body it never sends, andConnection: closeclose the socket inside the dispatch.server.stop(true)in a handler is the same free.Fix
consumePostPaddedmoves the buffer into a frame-localstd::stringfor the dispatch, and puts it back on a short read.returnafter a dispatch that destructed the parser touches no parser member.onClose,HttpResponse::upgrade, the HTTP/2 handoff. Self-reviewed: 6 concerns raised, 6 addressed (see Notes).test/js/bun/http/serve-pending-promise-abort-leak.test.ts, three cases, all fail on a released bun. Other suites: see Notes.Background
HttpParser::fallbackis astd::stringin the socket's uWS ext. A read that ends mid-head is appended to it. The next read parses the head out of it.onCloseruns.Notes
The connection close gate runs when a response completes.
Connection: closeplus a request body that never finished closes the socket from insideinternalEnd, and a response larger than the cork buffer completes while the dispatch is still on the stack. The pendingreq.text()then rejects, and its handler reads two garbage bytes asreq.urlon a release build.Suites run on the debug build after the merge of main:
serve-pending-promise-abort-leak,serve,bun-server,bun-serve-routes,node-http,node-http-parser,node-http-maxHeaderSize,node-http-req-socket-pause,node-http-backpressure,node-http-server-timeouts,websocket-server-upgrade-reentrant,websocket-server-upgrade-early-frames,request-smuggling.This replaces an earlier shape of the same fix, which hooked
HttpContext::onCloseand parked the buffer through a raw pointer on the sharedHttpContextData. A self-review rejected it: the buffer belongs to the frame that creates the views, the hook covered only one of the three destruct sites, and it put two pointer stores and astd::stringon every read. The other concerns: the hook moved the string while views into it were live, which needed a pointer-stability argument (the move now happens before any view exists, and the reserve runs on the local), the body leaned on work for nested event loop runs that a maintainer rejected, and it did not state the client-side precondition (now the first test case).The cases run from
serve-split-head-stop-fixture.tsandserve-split-head-close-fixture.tswithbunRun.How the tests park a partial head without a sleep: the client sends one write that holds a whole
/barrierrequest plus the first 22 bytes of the next head. The server answers/barrierand parks the rest in the fallback buffer, in that same read. The client waits for the/barrierresponse, which proves the park happened, then writes the rest of the head.The
stop(true)cases churn native allocations of many sizes around the size of the freed block (about 430 bytes) so a build with no sanitizer observably reads the new owner's bytes. The client-only case needs no churn: the freed block's first bytes are already clobbered when the url is read.Checked against this fix, from a fuzz census of this axis: every one of the 293 cut positions of a 294 byte head, the other lazy readers (
req.url,req.clone(),new Request(req),Bun.inspect(req), a GC before the read, andreq.paramsandreq.cookiesonrouteshandlers), the synchronous part of the handler and the part after oneawaitof a resolved value, TLS and unix sockets, andstop(true)followed byupgrade(req)or a throw.stop(),reload(), a plainupgrade(req), keep-alive and pipelining, a client RST or FIN, the idle timeout andnode:httpnever hit this free.A head that arrived in one read can still be clobbered by a nested read of the same connection, because those views alias the loop receive buffer. That is a different buffer and a different trigger. It belongs to the work on not re-entering the event loop from a handler (#33261), not here.
Pre-existing failures in this container, identical on a released bun:
serve.test.tsroot range port and/bun:info,bun-server.test.tslisten on IPv6 (no IPv6 here).[human-review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file