uws: a split request head leaves the parser before its dispatch - #44210
Conversation
A request head that took several reads is parsed out of the parser's fallback buffer. consumePostPadded emptied that buffer only after the dispatch, and only when the handler answered with the same socket. The parse frame now owns the buffer for the dispatch. The parser gets the block back when the handler answers with the same socket, and the bytes back when the head is not complete yet. For node:http the head of such a request is the whole rest of the read, as for a head that came in one read. The Bun.serve cases and their two fixtures come from #42789.
StatusReproduced on main
With this change the three cases give Node's output, and the fixtures are clean. Pull request: #44210 |
|
Navigate logical layers of code changes, visualize relationships, and explore their 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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe parser now reassembles partial request heads and handles bytes following a completed head for node:http. New Bun and Node HTTP tests cover split-head header access, CONNECT and Upgrade tunnel data, and connection shutdown. ChangesHTTP request handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The split-head adjustment keeps the CONNECT/Upgrade head span within the current read after parsing consumes the request head. No actionable merge-blocking risk is established. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:00 AM PT - Sep 29th, 2026
✅ @robobun, your commit fe5cd144f89d8ac5fa537bd44d249c3d7497a885 passed in 🧪 To try this PR locally: bunx bun-pr 44210That installs a local version of the PR into your bun-44210 --bun |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the new pointer arithmetic at packages/bun-uws/src/HttpParser.h:1437: the head can only end past the had bytes from earlier reads, so the remaining length is at most maxCopyDistance and the widened span stays inside the current read, including when the buffer hit maxFallbackSize. The three exits of the reassembly frame (handler answered another socket, head consumed, head still incomplete) each leave fallback in a defined state and touch no member after a dispatch that may have freed the parser, and the node:http-only skip of dataHandler in the CONNECT arm is covered by the widened head, so no bytes are dropped or doubled on that path.
Extended reasoning...
The change reworks ownership of the split-head reassembly buffer in the vendored uWebSockets parser (packages/bun-uws/src/HttpParser.h, consumePostPadded and fenceAndConsumePostPadded) so the parse frame holds the bytes across the request dispatch, widens node:http req.head for a split head to the whole rest of the read, and skips the CONNECT-arm dataHandler for node:http; the Rust change is a comment only. It touches a security-sensitive surface (HTTP request parsing of peer-controlled bytes, memory safety under handler-driven socket teardown) but adds no new trust boundary. Tests add two spawned ASAN fixtures for Bun.serve and node:http tunnel/abort/close-drain matrices. Not approved because it is a memory-safety fix in a core parser hot path with one inline finding on the widened head copy; a human maintainer should weigh the head-copy cost for plain node:http requests.
The second request can reach a connection that the server has closed. On Windows and macOS the client then gets ECONNRESET ahead of 'close', and events.once(socket, 'close') rejects on that 'error'.
There was a problem hiding this comment.
I reviewed the current revision and found no bugs; because this rewrites the ownership of the parser's reassembly buffer during the dispatch and adds backward pointer arithmetic on a span into the read buffer, a human look at packages/bun-uws/src/HttpParser.h is still worthwhile.
What was reviewed:
fallbackstate on every exit of the fallback arm: moved back after a complete head (cleared), restored intact on an incomplete head (431 check still runs on the restored length), left empty whenreturnedData != user(error, upgrade, or a closed socket), and no member of the parser is touched between the dispatch and that return.- The
req->head.data() - lengthextension at HttpParser.h:1435: it is in bounds only if the head ends past thehadbytes from earlier reads, which the pre-existingerrorStatusCodeOrConsumedBytes -= hadadjustment already relies on, and ConsumeMinimally breaks after the head soconsumedBytes >= hadholds for thedata += consumedBytes - hadstep. - The skipped
dataHandlerfor a node:http CONNECT with remaining bytes: those bytes are exactly the head span the dispatch just delivered, and the Bun.serve arm keeps the call; the next tunnel read reaches theisConnectRequestearly-dispatch at line 1227 with an empty fallback, so the repeat cannot recur. - The rewritten NodeHTTPResponse.rs comment still matches
UpgradeCTX::reset()nullingrequestsopreserve_web_socket_headers_if_neededcannot deref a dead request.
Extended reasoning...
The PR changes the fallback-buffer arm of consumePostPadded in packages/bun-uws/src/HttpParser.h so a stack-local string owns the reassembled head during the dispatch, sets req.head for node:http to the uncopied tail of the read and extends it backwards inside fenceAndConsumePostPadded, and skips the CONNECT dataHandler for node:http; the Rust change is a comment only, and the rest is tests (two spawned ASan fixtures for Bun.serve, node:http CONNECT/upgrade split-head matrices, closeIdleConnections and https FIN cases). It touches the HTTP request parser that handles untrusted network bytes, a memory-safety-critical surface, though no new parsing of adversarial lengths is introduced. The hunt ran dry with no findings and my reading confirmed the fallback bookkeeping and the head-span invariant, but the change is a use-after-free fix in shared parser core with pointer arithmetic that depends on a non-local invariant, so it is not simple enough to approve without a human. The only prior inline note from this bot was a performance nit that the author resolved; no third-party objections are recorded.
Supersedes #42789.
Problem
node:http, request head in two reads, no body: a'connect'or'upgrade'listener that callssocket.end()gets the head again with the next read,FIRST-connect example.test:443 HTTP/1.1\r\nHost: example.test:443\r\n\r\nFIRST-SECOND(new since node:http: the server follows Node (request body, framing, response finish, lifecycle) #43557). Bytes past 17 KB behind the head are lost (1.3.6 to 1.4.2).consumePostPadded(packages/bun-uws/src/HttpParser.h:1671) empties its reassembly buffer after the dispatch, and for one handler answer only.Bun.serve: a handler that closes the socket frees that buffer under the request,heap-use-after-free(1.3.6 to 1.4.2).Fix
node:http,headof such a request is the whole rest of the read, as for a head in one read.test/js/node/http/node-http-connect.test.ts(18 of 20 new cases fail on main). Suites: Notes.Background
fallbackis the parser'sstd::stringfor a request head that needs several reads.headis the third argument of'connect'and'upgrade'.HttpContext.h:593): every other answer keeps the defect and the use-after-free.Downsides
headof such a tunnel grows (20,000 bytes against 17,187) and the first'data'chunk goes. The stream is the same.closeIdleConnections()in the handler of such a request closes the connection, like a head in one read.headBuffer (before 17,180, whole head 127,905).Notes
Supersedes #42789: its parser hunk (the buffer that the parse frame owns) is the first part of this change, and its three
Bun.servecases and two fixtures come along unchanged. The tunnel fuzzer reported the twonode:httpfaces as ledger entries 79844 (the repeat) and 79846 (the loss).Repro, no timers. Node v26.3.0 prints
"FIRST-SECOND".How it goes wrong on main. The second read completes the head in
fallback.socket.end()in the listener shuts the socket down, so the request handler inHttpContext.hanswersnullptr.consumePostPaddedreturns at once. It does not emptyfallback, and it does not deliver the part of the read that did not fit into it (the buffer takesmaxHeaderSizeplus 864 bytes). Since #43557 anode:httptunnel reads aftersocket.end(). The next read is appended to the old head, and the tunnel gets the whole buffer. Before #43557 nothing was read aftersocket.end().Which line is the fix.
std::string reassembled = std::move(fallback)ahead of the dispatch removes the repeat and the use-after-free. Theheadspan at the dispatch site removes the loss. The tunnel arm then has nothing left to deliver fornode:http.A third symptom goes with them. Over https, a plain request with a split head whose listener ends the socket raised
'clientError'HPE_INVALID_EOF_STATEat the client's FIN (1.4.0 to 1.4.2 and main, not 1.3.14).node-http-server-abort-events.test.tshas the case.Not fixed here. An Upgrade request that declares a body still gets no body and no tunnel bytes when its listener calls
socket.end()(#44206).What changes on a path that is right today
headfor a request head that took several reads is the whole rest of the read, the value that Node gives. A listener that ends its side later got the rest as its first'data'chunk.fallbacksees such a head like a head from one read once it is dispatched: the idle test ofcloseIdleConnections(), the request timer, theclientErrorat the peer's FIN.Measurements. Linux x64. main is
9f70da074, and the other build is this change on it. The rows marked (h) come from a build ofHttpParser.halone with the release flags (clang -O3), outside the repo.connect,upgradex head in 1, 2, 3 reads x four ways to end in the listener x 6 bytes, 64 KiB, and the listener that ends last)Bun.servefixtures under ASAN, clean of 3operator new+deletefor each later split head on one connection (2 and 3 reads, 1000 heads)sizeof(uWS::HttpRequest)textconsumePostPadded<false>,<true>fenceAndConsumePostPadded<true, true>,<false, true>Bun.serve,node:http)onDatacalls for each split-head tunnel (100 tunnels, lldb)headBuffer of a plain POST, 256 KiB of body with the head (lldb): split head, whole headcloseIdleConnections()in the handler, 6 orders: a split head answers like a whole headfenceAndConsumePostPadded<false, true>, the parse of a head from one read, is the same in the two release builds (717 lines, addresses left out).consumePostPaddedgets a larger frame and other registers. That is the 5 instructions more and the 3 less for a head in one read. No operation is added there.res.end()thencloseIdleConnections()in the same tick moves away from Node's answer for a split head. A whole head answers that way today (node:http: closeIdleConnections() in the same tick as res.end() closes the connection of that request #44207).req.urlin theBun.servefixture. 1.3.5 has nohead.What no test in the repo pins.
fallback.clear()behind the move, and the block that the parser gets back: they change allocations only. A test needs a hook into the parser. Each other clause fails a new case when it is deleted (the restore of a head that is not complete: the cases with three reads).Suites on the debug build:
node-http-connect,node-http-server-close-drain,node-http-server-abort-events,node-http-transfer-encoding,node-http-maxHeaderSize,node-http-maxHeadersCount,node-http-parser,node-http-req-complete,node-http-req-socket-pause,node-http-server-timeouts,node-http-with-ws,node-http,ws,request-smuggling,websocket-server-upgrade-early-frames,websocket-server-upgrade-reentrant,serve-pending-promise-abort-leak,serve.The machine was under load from other work. Tests that start a process or move 64 MB reached the 5 s limit there: 2 in
node-http-connect, 1 inserve-pending-promise-abort-leak, 2 innode-http-with-ws, 1 innode-http. A debug build of main does the same in those four files (5, 1, 1 and 1, not the same tests).serve.test.tsroot range port and/bun:infofail on this machine with every build. No other test failed.Follow-ups
socket.end()in the listener.closeIdleConnections()in the same tick asres.end().headBuffer that every request with bytes behind its head copies.Content-Length: 0and a split head gets a 408 on the idle connection. It is in the same branch ofconsumePostPadded, and this change leaves it as it is.Bun.servehas no tunnel API. The bytes behind a CONNECT head reach no handler (req.bodyisnull), with this change as before.Not run: macOS, Windows.
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-server-close-drain.test.ts, test/js/node/http/node-http-connect.test.ts