"f10bc786ef74aa4a4ee740b0ce01eb42d7a23399": "node:http: destroy an idle pooled keep-alive socket that receives unsolicited data (#42128)\n\n### Problem\n\n- A socket parked in `agent.freeSockets` (`keepAlive: true`) keeps\nreading while idle, and unsolicited bytes do not retire it. A whole\nresponse written there is attributed to the next request.\n- The cause is the Agent's `'free'` handler\n(`src/js/node/_http_agent.ts:123`). It pools the socket with nothing\nwatching the read side, where the parser is already detached. Node fixed\nthe same hole as CVE-2026-48931 (https://hackerone.com/reports/3582376).\nThis ports that one fix only.\n\n### Fix\n\n- The Agent marks a pooled socket with `kDestroyOnRead`, and\n`reuseSocket` clears it. A freed socket that already holds buffered\nreadable data is destroyed before it can be pooled or handed to a queued\nrequest.\n- In `node:net`, `pushDataToSocket` (the one function through which all\nthree handler tables feed the stream, from #35347) destroys a marked\nsocket instead of pushing the bytes.\n- Like node's guard, it adds no public stream listener (Notes).\n- Verified: `test/js/node/http/node-http-agent-free-socket.test.ts`, 8\n`node:test` cases that Bun runs in-process, plus one that runs the same\nfile under Node.js. 6 fail on released bun. On Node they pass from\nv26.4.0, where the upstream fix shipped (Notes). Also\n`test/js/node/http/`, `test/js/node/net/`, and the vendored http, https,\nnet, tls suites. Self-reviewed: 4 concerns, 3 addressed, 1 declined\n(Notes, \"Scope\").\n\n### Background\n\n- `Agent.freeSockets` holds idle keep-alive sockets per origin.\n`addRequest` takes one out and calls `reuseSocket`.\n- A pooled socket has no parser and no `'data'` listener: what it\nreceives goes nowhere, or reaches the next response's parser.\n- A bun `net.Socket` reads through a native handler table passed at dial\ntime. One table serves every socket that used it, so a per-socket hook\nmust live on the socket.\n\n<details><summary>Notes</summary>\n\n**Reproduction** (bun 1.4.3, linux x64). A raw origin answers each\nrequest with its own path, then writes a complete unsolicited response\non the idle pooled connection.\n\n| when the stray response arrives | stock bun | this branch |\n| --- | --- | --- |\n| the event loop polls before the next request | socket stays pooled,\nbytes discarded | socket destroyed, next request dials a fresh\nconnection |\n| the next request is issued in the same tick | next request reads\n`poison` | next request reads `poison` |\n\nNode v26.3.0 (before the guard shipped) behaves like stock bun in both\nrows.\n\n**Node versions.** The upstream fix first shipped in Node v26.4.0\n(`lib/_http_agent.js` at the v26.3.0 tag has no\n`installFreeSocketDataGuard`). Forced to run everywhere: Node v26.3.0\npasses 2 of 8 (the reuse cases) and fails the 6 guard cases, like stock\nBun; Node v26.4.0 passes 6 and fails the 2 queued-request cases (Node\nchecks buffered bytes only on the pool path); this branch passes 8. In\nthe file the guard cases skip on Node < 26.4.0 and the queued-request\ncase skips on Node, each with the reason.\n\n**No public listener.** Node's first version of this guard used a\n`'data'` listener plus `resume()`. node-fetch@2 reads\n`socket.listenerCount('data')` while a response closes and started\nreporting false `ERR_STREAM_PREMATURE_CLOSE` errors (nodejs/node#63989),\nso node reworked the guard onto the stream handle's internal `onread`\nhook. Bun has no per-socket equivalent of that hook: the handler table\ngiven to `Bun.connect` is one shared cell, and `socket.reload()` mutates\nit for every socket that shares it. Hence the per-socket flag, read in\n`pushDataToSocket`. The test asserts `listenerCount('data')` and\n`listenerCount('readable')` are still 0 on a free socket, as node's\ndoes. The handler table built for the `onread` socket option keeps its\nown `data` callback: it bypasses the stream, and `node:http` cannot use\nsuch a socket.\n\n**The same-tick row is the residual race, and node has it too.** The\nstray bytes are still unread in the kernel when `addRequest` hands the\nsocket out, so no check in JS can see them. Node's own test says as much\n(\"in a real attack, there is always time between the poison arriving and\nthe next client request\"). Closing it needs a peek of the read side at\ncheckout, which is what #41987 does for `fetch()`'s pool in Rust.\nUnrelated to this change: when bytes that win that race are not a valid\nresponse, the client's parse error surfaces as an uncaught exception\ninstead of an `'error'` event on the request, on stock bun and on this\nbranch alike.\n\n**The upstream test is not vendored yet.**\n`test/parallel/test-http-agent-free-socket-data-guard.js` injects the\nstray bytes with `req.socket.write()` on a `node:http` server. Bun's\nserver leaves that socket corked after the request\n(`socket.writableCorked === 1`, the bytes sit in the writable buffer),\nwhich is #35664. Once that lands, the upstream file can be vendored as\nis and the bespoke cases trimmed. The new cases drive a raw `net`/`tls`\nserver instead, which is also how they cover `https.Agent`.\n\n**Scope.** CVE-2026-48931 shipped in a Node security release together\nwith other advisories. This PR does not examine or claim anything about\nthe others. The self-review asked for a public tracking issue that lists\nthem against bun; I left that to the maintainers, since it amounts to\npublishing an unverified vulnerability list.\n\n**Already-buffered bytes.** Node's `installFreeSocketDataGuard` destroys\na socket whose `readableLength > 0`, but the caller still pushes it into\n`freeSockets`, where `'close'` prunes it a tick later (and `addRequest`\ncan pop it first under `lifo`), and the check does not run at all when\nthe freed socket goes straight to a request queued in `agent.requests`.\nHere the check runs right after the `writable` check in the `'free'`\nhandler, so both hand-off paths share it and a destroyed socket is never\npooled. For the queued path, the destroyed socket's `'close'` reaches\n`removeSocket`, which dials a new connection for the waiting request.\nThe \"holds unsolicited data\" cases cover both paths: they `push()` the\nstray bytes in the response's `'end'` handler, which is after the parser\ndetached and one tick before `'free'`.\n\n**Disarm point.** `reuseSocket` clears the flag, as in Node, where\n`Agent.prototype.reuseSocket` is the only caller of\n`removeFreeSocketDataGuard` and nothing else restores `_handle.onread`\n(`initSocketHandle` runs only for a new or reconnected socket). A\nsubclass that replaces `reuseSocket` without calling the parent loses\nreused sockets on both runtimes.\n\n**TLS.** The guard sees decrypted application data only, so a\npost-handshake `NewSessionTicket` on an idle pooled socket does not trip\nit. The `https` cases cover a parked TLS socket that is poisoned, one\nfreed with buffered bytes (pooled and queued paths), and one that is\nreused.\n\n**Cost.** One symbol-property load per received chunk. The `src/` diff\nis 28 lines. `kDestroyOnRead` is initialized in the `Socket` constructor\nso the read path stays monomorphic.\n\n**Pre-existing failures in this container, with and without this diff**\n(each one rechecked against main's `src/js` on the same build):\n`test-http-agent-keepalive.js` (`agent.sockets[name]` is not cleaned up\nafter the server closes the socket),\n`test-http-client-timeout-option.js`, the `test-http(s)-proxy-request*`\nfamily, one subprocess case of `node-http-syscall-fault.test.ts`, 10\n`node-net.test.ts` cases that fail here with `ECONNREFUSED`,\n`test-net-server-async-dispose.mjs`,\n`test-net-connect-custom-lookup-non-string-address.mjs`,\n`test-tls-client-allow-partial-trust-chain.js`.\n\n</details>\n\n<!-- robobun:evidence:begin -->\n\n---\n\n**[human-review]** gate passed \u00b7 iteration 2 \u00b7 4 files touched\n\n<details><summary>fails on main (without fix)</summary>\n\n```console\nASAN without fix: 6 FAILED\n$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test \"--reporter=junit\" \"--reporter-outfile=/tmp/pr_gate.xml\" test/js/node/http/node-http-agent-free-socket.test.ts\nbun test v1.4.3 (4ff919377)\n\ntest/js/node/http/node-http-agent-free-socket.test.ts:\n157 | assert.strictEqual(freeSocket.listenerCount(\"readable\"), 0);\n158 | \n159 | serverSockets[0].write(poisonedResponse);\n160 | \n161 | await pollUntil(() => freeSocket.destroyed && agent.freeSockets[name] === undefined);\n162 | assert.strictEqual(freeSocket.destroyed, true);\n ^\nAssertionError: Expected values to be strictly equal:\n\nfalse !== true\n\n generatedMessage: true,\n actual: false,\n expected: true,\n operator: \"strictEqual\",\n diff: \"simple\",\n code: \"ERR_ASSERTION\"\n\n at /workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:162:16\n at withAgent (/workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:118:11)\n at /workspace/bun/test/js/node/http/node-http-agent-free-socket.test.ts:150:13\n at node:test:1781:26\n at executeTestNode (node:test:1785:63)\n at processTicksAndRejec\n... (truncated)\n\nrelease without fix: all passed\nbun test v1.4.3-canary.1 (9f655b715)\n\ntest/js/node/http/node-http-agent-free-socket.test.ts:\n(pass) http.Agent free keep-alive socket over http > destroys a free socket that receives unsolicited data [22.64ms]\n(pass) http.Agent free keep-alive socket over http > does not pool a socket that holds unsolicited data when it is freed [3.12ms]\n(pass) http.Agent free keep-alive socket over http > does not hand a freed socket that holds unsolicited data to a queued request [2.17ms]\n(pass) http.Agent free keep-alive socket over http > reuses a free socket that received nothing [1.64ms]\n(pass) http.Agent free keep-alive socket over https > destroys a free socket that receives unsolicited data [41.32ms]\n(pass) http.Agent free keep-alive socket over https > does not pool a socket that holds unsolicited data when it is freed [5.10ms]\n(pass) http.Agent free keep-alive socket over https > does not hand a freed socket that holds unsolicited data to a queued request [4.67ms]\n(pass) http.Agent free keep-alive socket over https > reuses a free socket that received nothing [3.29ms]\n(pass) Node.js compatibility > all tests pass in Node.js [156.92ms]\n\n 9 pass\n 0 fail\nRan 9 tests across 1\n... (truncated)\n```\n\n</details>\n\n<details><summary>passes on PR (with fix)</summary>\n\n```console\nASAN with fix: all passed\n$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test \"--reporter=junit\" \"--reporter-outfile=/tmp/pr_gate.xml\" test/js/node/http/node-http-agent-free-socket.test.ts\nbun test v1.4.3 (4ff919377)\n\ntest/js/node/http/node-http-agent-free-socket.test.ts:\n(pass) http.Agent free keep-alive socket over http > destroys a free socket that receives unsolicited data [1158.89ms]\n(pass) http.Agent free keep-alive socket over http > does not pool a socket that holds unsolicited data when it is freed [172.89ms]\n(pass) http.Agent free keep-alive socket over http > does not hand a freed socket that holds unsolicited data to a queued request [115.53ms]\n(pass) http.Agent free keep-alive socket over http > reuses a free socket that received nothing [100.60ms]\n(pass) http.Agent free keep-alive socket over https > destroys a free socket that receives unsolicited data [474.62ms]\n(pass) http.Agent free keep-alive socket over https > does not pool a socket that holds unsolicited data when it is freed [200.09ms]\n(pass) http.Agent free keep-alive socket over https > does not hand a freed socket that holds unsolicited data to a queued request [147.77ms]\n(pass) http.Agent fre\n... (truncated)\n\nrelease with fix: all passed\n$ bun scripts/build.ts --profile=release\n[configured] bun-profile \u2192 bun (stripped)\n target linux-x64-gnu\n build type Release\n build dir ./build/release\n revision 400b9a922c\n features baseline\n\n23 deps, 131 codegen, 1172 objects in 697ms\n\nninja: Entering directory `/workspace/bun/build/release'\n[1/146] fetch picohttpparser\n[picohttpparser] up to date\n[2/146] fetch WebKit (prebuilt)\n[WebKit] up to date\n[3/146] gen ZigGeneratedClasses.{cpp,h,rs}\nFound 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts\n - ResolveMessage (15 fields)\n - BuildMessage (10 fields)\nFound 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts\n - Archive (4 fields, 1 class fields)\nFound 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts\n - ResourceUsage (8 fields)\n - Subprocess (20 fields)\nFound 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts\n - CronJob (5 fields)\nFound 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts\n - FileSystemRouter (5 fields)\n - FrameworkFileSystemRouter (2 fields)\n - MatchedRoute (8 fields)\nFound 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts\n\n... (truncated)\n```\n\n</details>\n\n<details><summary>diff hotspot</summary>\n\n```\nsrc/js/internal/net/symbols.ts | 3 +\n src/js/node/_http_agent.ts | 12 +\n src/js/node/net.ts | 14 +-\n .../node/http/node-http-agent-free-socket.test.ts | 251 +++++++++++++++++++++\n 4 files changed, 279 insertions(+), 1 deletion(-)\n```\n\n</details>\n\n**gate history** \u00b7 3 passed \u00b7 2 rejected \u00b7 iteration 2\n\n<details><summary>evidence per changed file</summary>\n\n```\nfile reads edits tests\nsrc/js/internal/net/symbols.ts 3 4 6\nsrc/js/node/_http_agent.ts 5 10 8\nsrc/js/node/net.ts 13 15 7\ntest/js/node/http/node-http-agent-free-socket.test.ts 0 0 2\n```\n\n</details>\n\n<!-- robobun:evidence:end -->"
0 commit comments