node:http: the server socket grants the connection to one response and takes it back

Merged
RO
1mo ago
+977-98

Problem

  • A replaced node:http response keeps its socket pointer. After the close, a call on it reads freed memory: heap-use-after-free in uws_res_state (NodeHTTPResponse::is_socket_closed_or_closing), or Segmentation fault at address 0x0. Without one, the process never exits.
  • JSNodeHTTPServerSocket tells 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-overflow in uWS::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::CURRENT marks 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), the node-http*.test.ts files, node's test-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. .text grows 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.

FlagsHowTake-back
ended and completeEach 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 flightThe four JS triggers, then a pipelined request.Out of line: tail, handlers, handle, SOCKET_CLOSED, refs.
complete, not ended, still pendingThe 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):

SiteLabelReason
get_this_value, get_server_socket_value, notify_when_read_parsedreaderLooks up the socket, writes nothing.
capture_request_trailersreaderTakes the trailers of the request.
pause_socket, pause_socket_reads, resume_socket, do_pause, do_resumereaderFlow control of the request body. A queued request uses it.
release_body_slot, set_on_datareaderThe request body handler.
set_on_aborted_handler, dispatch tail (mod.rs)readerThe 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_closingreaderReads a connection-scoped state bit.
get_buffered_amount, has_unflushed_writereaderReads the byte count of the socket buffer. res.writableLength of a queued response includes it (node-http-backpressure.test.ts).
abort (first site)readerAsks uWS to close the connection after the read that it parses.
abort (second and third site)writerEnds 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_writewriterSends bytes of the response.
set_on_writablewriterArms the drain handler of the response.
set_timeoutwriterSets the timeout of the response in flight.
node_http_request_on_resolve, node_http_request_on_reject, thrown dispatch (mod.rs)writerEnds the response for a settled or thrown dispatch.
on_upgrade (server_body.rs)writerWrites the 101 status and headers.
upgradetake_for_upgradeOnly the current response can hand the socket to a WebSocket.
handle_abort_or_timeout (2)releaseThe socket closed.
upgrade (first check)writerOnly the current response can upgrade.
grant_connectionreaderChecks that the response still has the handle.
take_back_connection (finished response)releaseThe server socket took the connection back.
take_back_unfinished_connectionwriter, reader, releaseFlushes 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.

WhatmainPRTool
.text58,148,85358,150,645 (+1,792)size -A
stripped binary80,832,07280,836,168 (+4,096)ls -l
NodeHTTPServer__onRequest_http1,500 instructions1,525llvm-objdump -d
of them at the slot store, previous response finished014, no call (the take-back is inlined)llvm-objdump -d
NodeHTTPResponsePrototype__getFlags2828
NodeHTTPResponsePrototype__writeHeadAndEnd221226
write_or_end::<true>713723
write_head_impl456453
NodeHTTPResponse__createForJS192195
pipelined activation (startPipelinedResponse)278, 5 calls, 3 lock-prefixed333 (262 + 71 out of line), 7 calls, 3 lock-prefixed
WebSocket upgrade callback6142, 7 calls, 2 lock-prefixed (onUpgraded)
extern "C" entries+3 (Bun__NodeHTTPResponse_takeBackConnection, Bun__NodeHTTPResponse_grantConnection, uws_res_clear_handlers_of), -0nm
JS to native calls per requestunchanged in 4 of 4 shapes (sequential or pipelined, sync or async handler)wrapped prototypes, debug builds
NodeHTTPResponse544 bytes544size_of
Flags22compile-time assertion
JSNodeHTTPServerSocket208208llvm-dwarfdump
NodeHTTPResponse cells after 100 replaced responses, connections closed, full GC1011heapStats()
user CPU per request, 200,000 sequential keep-alive requests, 8 interleaved runsmedian 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 runsmedian 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 eachsequential: 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 file2.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 CURRENT check 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-overflow in uWS::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.ts runs 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() and res.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 raw socket.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 draining scenario 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.