node:http: the server socket grants the connection to one response and takes it back
Problem
- A replaced node:http response keeps its socket pointer. After the close, a call on it reads freed memory:
heap-use-after-freeinuws_res_state(NodeHTTPResponse::is_socket_closed_or_closing), orSegmentation fault at address 0x0. Without one, the process never exits. JSNodeHTTPServerSockettells only its current and queued responses about a close.startPipelinedResponse()and the dispatch (NodeHTTP.cpp) replace the current response silently.res.detachSocket()plus a pipelined request reach that.- After a WebSocket upgrade, a queued response reads the old socket:
heap-buffer-overflowinuWS::HttpContextData::isParsing.
Fix
setCurrentResponse()alone stores the current response and takes the connection back from the one that leaves.onUpgraded()does that for a WebSocket upgrade.Flags::CURRENTmarks the response that has the connection.writer()answers only for it,reader()also for a queued response.- Verified:
test/js/node/http/node-http-displaced-response.test.ts(20 pass, main fails all 20), thenode-http*.test.tsfiles, node'stest-http-*files.
Background
- uWS keeps one response state per socket. The current response writes through it. A pipelined response waits in a queue.
- A take-back flushes the tail of a
write()into the socket buffer, clears the response's socket handlers, and releases its refs. JS sees a closed response. - Considered: telling replaced responses about a close. They could still write into the next response.
Downsides
- A replaced response drops its writes. main sent some. Node.js buffers them.
- Each keep-alive request after the first runs 14 more instructions.
.textgrows by 1,792 bytes. - A replaced request gets no
'aborted'event at the close.
Notes
Repro (from the report; bun repro.mjs printed a segfault on 1.4.0 to 1.4.3, returned on 1.3.14 and Node.js v26.3.0):
import http from "node:http";
import net from "node:net";
let n = 0;
const server = http.createServer((req, res) => {
if (n++ > 0) return;
res.detachSocket(req.socket);
setTimeout(() => { res.end("late"); console.log("returned"); server.close(); server.closeAllConnections(); }, 100);
});
server.listen(0, "127.0.0.1", () => {
const c = net.connect(server.address().port, "127.0.0.1", () => {
c.write("GET /a HTTP/1.1\r\nHost: t\r\n\r\n");
c.write("GET /b HTTP/1.1\r\nHost: t\r\n\r\n");
setTimeout(() => c.destroy(), 30);
});
c.on("error", () => {}); c.resume();
});
Commits. The first commit changes no behaviour: raw_response moves into a private module behind writer() / reader(), and currentResponseObject becomes private behind currentResponse() / setCurrentResponse(). The release build of that commit alone, on its earlier base 1313ca61d4, had the .text size of that base (58,143,989 bytes) and the same instruction count in every NodeHTTPResponse function. The second commit is the role and the take-back. The third is the WebSocket case.
States of a response that is replaced.
| Flags | How | Take-back |
|---|---|---|
| ended and complete | Each keep-alive request replaces the one before. Its native handle (resume, pauseReads, notifyWhenReadParsed, flushHeaders, end) read the freed socket after a close. | Releases the handle. 14 instructions, inlined into the dispatch. |
| in flight | The four JS triggers, then a pipelined request. | Out of line: tail, handlers, handle, SOCKET_CLOSED, refs. |
| complete, not ended, still pending | The dispatch promise settled before the response ended, then the next request came. The process never exited. | The same. |
A taken-back response gets SOCKET_CLOSED, so is_done(), the Drop assertion and the JS checks of handle.flags need no change. JS gets no event for it.
Label of each site (NodeHTTPResponse.rs unless a file is named; 25 writer, 19 reader, 4 release, 1 take_for_upgrade):
| Site | Label | Reason |
|---|---|---|
get_this_value, get_server_socket_value, notify_when_read_parsed | reader | Looks up the socket, writes nothing. |
capture_request_trailers | reader | Takes the trailers of the request. |
pause_socket, pause_socket_reads, resume_socket, do_pause, do_resume | reader | Flow control of the request body. A queued request uses it. |
release_body_slot, set_on_data | reader | The request body handler. |
set_on_aborted_handler, dispatch tail (mod.rs) | reader | The timeout handler shares its user data slot with the request body handler. Both belong to the exchange that was dispatched last, which can be a queued one. |
is_socket_closed_or_closing | reader | Reads a connection-scoped state bit. |
get_buffered_amount, has_unflushed_write | reader | Reads the byte count of the socket buffer. res.writableLength of a queued response includes it (node-http-backpressure.test.ts). |
abort (first site) | reader | Asks uWS to close the connection after the read that it parses. |
abort (second and third site) | writer | Ends the response. A queued request that is destroyed no longer writes an empty response into the place of the response in flight. |
write_head_impl, write_head_and_end, write_or_end (7), write_continue, write_informational, flush_headers, cork, on_auto_flush, spill_pending_pinned_write | writer | Sends bytes of the response. |
set_on_writable | writer | Arms the drain handler of the response. |
set_timeout | writer | Sets the timeout of the response in flight. |
node_http_request_on_resolve, node_http_request_on_reject, thrown dispatch (mod.rs) | writer | Ends the response for a settled or thrown dispatch. |
on_upgrade (server_body.rs) | writer | Writes the 101 status and headers. |
upgrade | take_for_upgrade | Only the current response can hand the socket to a WebSocket. |
handle_abort_or_timeout (2) | release | The socket closed. |
upgrade (first check) | writer | Only the current response can upgrade. |
grant_connection | reader | Checks that the response still has the handle. |
take_back_connection (finished response) | release | The server socket took the connection back. |
take_back_unfinished_connection | writer, reader, release | Flushes the tail of its own write(), clears its own handlers, releases the handle. |
Measurements. Release builds of main (9f70da0741) and of this PR, linux x64. perf is not permitted in the container (perf_event_paranoid is 4) and valgrind cannot be installed, so there is no dynamic instruction count.
| What | main | PR | Tool |
|---|---|---|---|
.text | 58,148,853 | 58,150,645 (+1,792) | size -A |
| stripped binary | 80,832,072 | 80,836,168 (+4,096) | ls -l |
NodeHTTPServer__onRequest_http | 1,500 instructions | 1,525 | llvm-objdump -d |
| of them at the slot store, previous response finished | 0 | 14, no call (the take-back is inlined) | llvm-objdump -d |
NodeHTTPResponsePrototype__getFlags | 28 | 28 | |
NodeHTTPResponsePrototype__writeHeadAndEnd | 221 | 226 | |
write_or_end::<true> | 713 | 723 | |
write_head_impl | 456 | 453 | |
NodeHTTPResponse__createForJS | 192 | 195 | |
pipelined activation (startPipelinedResponse) | 278, 5 calls, 3 lock-prefixed | 333 (262 + 71 out of line), 7 calls, 3 lock-prefixed | |
| WebSocket upgrade callback | 6 | 142, 7 calls, 2 lock-prefixed (onUpgraded) | |
| extern "C" entries | +3 (Bun__NodeHTTPResponse_takeBackConnection, Bun__NodeHTTPResponse_grantConnection, uws_res_clear_handlers_of), -0 | nm | |
| JS to native calls per request | unchanged in 4 of 4 shapes (sequential or pipelined, sync or async handler) | wrapped prototypes, debug builds | |
NodeHTTPResponse | 544 bytes | 544 | size_of |
Flags | 2 | 2 | compile-time assertion |
JSNodeHTTPServerSocket | 208 | 208 | llvm-dwarfdump |
NodeHTTPResponse cells after 100 replaced responses, connections closed, full GC | 101 | 1 | heapStats() |
| user CPU per request, 200,000 sequential keep-alive requests, 8 interleaved runs | median 12.5 us (10.2 to 15.6) | median 13.5 us (11.5 to 17.1) | process.cpuUsage(); the ranges overlap, the host was loaded |
user CPU per request, 200,000 pipelined requests (4 per write, the handler answers from setImmediate), 8 interleaved runs | median 8.4 us (7.3 to 8.8) | median 9.7 us (7.5 to 14.3) | the same. This row was missing from the first version of the table |
| both benchmarks again after the merge, 16 interleaved runs each | sequential: median 16.6 us (11.7 to 21.9). pipelined: median 10.3 us (7.8 to 15.7) | sequential: median 14.3 us (12.5 to 21.9). pipelined: median 9.9 us (8.6 to 16.0) | the same. The median is lower for the PR and the minimum is lower for main. One build spreads by a factor of 2 (host load average 400 to 800), so these runs cannot show a difference of a few percent |
| the new test file | 2.7 s release, 76 s debug+ASAN |
Tests. Each test spawns one fixture process that runs the scenarios of one suite. A fixture that finds a response with its refs still held exits with code 1, so an unfixed build fails in seconds.
- 4 triggers x 11 later uses x 2 read orders (second request in the same read, or in a later one), on TCP and TLS.
- A finished response that the next keep-alive request replaced: 11 calls on its native handle after the close.
- The client stays: response 2 and 3 arrive in order, response 1 cannot write, and the socket does not take response 1 again.
- A queued response calls its native handle: nothing reaches the wire before its turn. Without the
CURRENTcheck this test fails. - A replaced response with an 8 MB write in the socket buffer: the bytes go out before response 2, and the process exits after a GC.
- A response that native code completed is replaced: the process exits.
- A WebSocket adopts the connection with a response queued, on TCP and TLS. The scenarios run side by side. In that form main shows no sanitizer report on TCP or on TLS, and the test fails on main only through the check of the held refs. When the TCP scenarios run one after the other, main reports
heap-buffer-overflowinuWS::HttpContextData::isParsing. - main (9f70da0741), debug+ASAN: 0 pass, 20 fail. Release 1.4.3: 0 pass, 20 fail.
- The tests are in a new file.
node-http-uaf.test.tsruns two fixtures that loop thousands of requests and are close to their time limits on debug and ASAN builds. The 20 fixture processes of this file would compete with them for the CPU. - The time limit in the test file applies only to a local debug or ASAN build, which needs about 4 s to start one fixture. CI uses the time of its runner.
Still different from Node.js after a replacement. Not changed here.
- Node.js buffers the writes of a detached response until
assignSocket(). Bun drops them.write()returns false,end()emits'prefinish'. - Node.js ignores a
'close'event that user code emits on a response. Bun's dispatcher takes it as the end of the response. - Node.js aborts the request of a replaced response at the close (
abortIncoming). Bun emits nothing for it. - Node.js does not dispatch a request that follows an Upgrade request in the same read. Bun queues it. When the second request arrives in a later read, Node.js queues it too.
Found, not fixed here.
res.writeContinue()andres.addTrailers()of a response that lost the connection go through the socket's current response and reach the wire in its place.res._send()on a queued response has no queue check.- After
res.emit('close')on an open response whose handler returned, native code ends the response with a bare\r\n. - A
'request'listener that throws makes the process segfault when its response waits behind a rawsocket.write()from a'connection'listener.mark_dispatch_threw_if_queued()compares against the current-response slot of the socket, and that slot is empty there. main has this since #43557. It is not in a release. - A response that upgrades while a large
write()still holds a tail by reference drops that tail. main does the same.
Same as Node.js. A request that is queued behind a request that takes the socket for a WebSocket gets no event at the takeover. It gets 'aborted', 'error' and 'close' when the socket closes. Node.js v26.3.0 does the same when the second request arrives in a later read (an Upgrade request that a 'request' listener holds and then answers with a raw 101). An earlier version of this description listed this as a defect, and a review reply said that Node.js does not reach this state. Both were wrong.
Self-review. Two reviews ran. The first covered the first two commits. Its verdict asked for three changes, and all three are here: one PR that has the tests, the WebSocket case, and no statement about Node.js in the tests that was not checked. The second covered the merged diff and finished after the merge. It raised 18 concerns (11 to fix, 7 to consider). This PR addresses none of them. The ones that change code, tests or this text:
- The crash and the
writeContinue()/addTrailers()item in the list above. - No test scenario has a request body, and
req.destroy()on a queued request has no test. - The
drainingscenario still passes when the call that clears the handlers is removed. I confirmed that with a copy of the debug binary that has a no-op in the place of that call. - The WebSocket scenarios fail on main only through a private flag bit (see Tests).
- Wrong text in this description: the statement about Node.js, the label counts, and the note about the WebSocket test. They are corrected above.
Question for a maintainer. The response keeps its pointer and a CURRENT bit, and the socket pushes a take-back. The other form removes the pointer and asks the socket on each use: about 15 more loads per request, and the wrapper grows from 64 to 80 bytes. This PR builds the first.
Platforms. I ran the tests on Linux x64. CI ran the new test file on macOS and Windows, x64 and aarch64: 20 pass on each.