From 0d62f61c827062070b88e5ffa50f441026b0d1a0 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 19 Jun 2026 16:46:17 +0200 Subject: [PATCH] http: avoid stream listeners on idle agent sockets Signed-off-by: Matteo Collina --- lib/_http_agent.js | 66 +++++++++++++++++-- .../test-http-agent-free-socket-data-guard.js | 9 +-- test/parallel/test-http-agent-keepalive.js | 5 +- 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 1a8ef14d4d40a7..eb58650f885105 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -43,12 +43,22 @@ const { getGlobalAgent, } = require('internal/http'); const { AsyncResource } = require('async_hooks'); -const { async_id_symbol } = require('internal/async_hooks').symbols; +const { + async_id_symbol, + owner_symbol, +} = require('internal/async_hooks').symbols; const { getLazy, kEmptyObject, once, } = require('internal/util'); +const { + onStreamRead, +} = require('internal/stream_base_commons'); +const { + kReadBytesOrError, + streamBaseState, +} = internalBinding('stream_wrap'); const { validateNumber, validateOneOf, @@ -60,6 +70,7 @@ const { getOptionValue } = require('internal/options'); const kOnKeylog = Symbol('onkeylog'); const kRequestOptions = Symbol('requestOptions'); const kRequestAsyncResource = Symbol('requestAsyncResource'); +const kFreeSocketDataGuard = Symbol('freeSocketDataGuard'); // New Agent code. @@ -92,9 +103,51 @@ function freeSocketErrorListener(err) { // in the TCP buffer and be silently consumed as the response for the // *next* request that reuses the socket (response-queue poisoning). // See: https://hackerone.com/reports/3582376 -function freeSocketDataGuard() { - debug('DATA on FREE socket - destroying poisoned socket'); - this.destroy(); +function freeSocketOnReadGuard() { + const nread = streamBaseState[kReadBytesOrError]; + if (nread === 0) return; + + debug('READ on FREE socket - destroying poisoned socket'); + this[owner_symbol].destroy(); +} + +function installFreeSocketDataGuard(socket) { + if (socket.readableLength > 0) { + debug('BUFFERED DATA on FREE socket - destroying poisoned socket'); + socket.destroy(); + return; + } + + if (socket.connecting) { + socket[kFreeSocketDataGuard] = function onConnect() { + socket[kFreeSocketDataGuard] = null; + installFreeSocketDataGuard(socket); + }; + socket.once('connect', socket[kFreeSocketDataGuard]); + return; + } + + const handle = socket._handle; + if (handle) { + handle.onread = freeSocketOnReadGuard; + if (!handle.reading) { + handle.reading = true; + const err = handle.readStart(); + if (err) socket.destroy(); + } + } +} + +function removeFreeSocketDataGuard(socket) { + if (socket[kFreeSocketDataGuard]) { + socket.removeListener('connect', socket[kFreeSocketDataGuard]); + socket[kFreeSocketDataGuard] = null; + } + + const handle = socket._handle; + if (handle?.onread === freeSocketOnReadGuard) { + handle.onread = onStreamRead; + } } function Agent(options) { @@ -207,8 +260,7 @@ function Agent(options) { this.removeSocket(socket, options); socket.once('error', freeSocketErrorListener); - socket.on('data', freeSocketDataGuard); - socket.resume(); + installFreeSocketDataGuard(socket); freeSockets.push(socket); }); @@ -600,7 +652,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { Agent.prototype.reuseSocket = function reuseSocket(socket, req) { debug('have free socket'); socket.removeListener('error', freeSocketErrorListener); - socket.removeListener('data', freeSocketDataGuard); + removeFreeSocketDataGuard(socket); req.reusedSocket = true; socket.ref(); }; diff --git a/test/parallel/test-http-agent-free-socket-data-guard.js b/test/parallel/test-http-agent-free-socket-data-guard.js index 9c1a526aaebb38..2cded0838ce1a4 100644 --- a/test/parallel/test-http-agent-free-socket-data-guard.js +++ b/test/parallel/test-http-agent-free-socket-data-guard.js @@ -8,8 +8,8 @@ // writes a full HTTP response during this window, it is consumed as the // response for the *next* request — poisoning the response queue. // -// The fix attaches a data guard listener + resume() on idle sockets so -// that unsolicited data causes the socket to be destroyed. +// The fix installs a read guard on idle sockets so that unsolicited data +// causes the socket to be destroyed without adding public stream listeners. const common = require('../common'); const assert = require('assert'); @@ -48,8 +48,9 @@ server.listen(0, common.mustCall(() => { assert.strictEqual(agent.freeSockets[name]?.length, 1); const freeSocket = agent.freeSockets[name][0]; assert.strictEqual(freeSocket.parser, null); - // With the fix, a data guard listener is attached - assert.strictEqual(freeSocket.listenerCount('data'), 1); + // With the fix, no public stream listeners are added. + assert.strictEqual(freeSocket.listenerCount('data'), 0); + assert.strictEqual(freeSocket.listenerCount('readable'), 0); // Step 2: Server injects a poisoned response while socket is idle serverSocket.write( diff --git a/test/parallel/test-http-agent-keepalive.js b/test/parallel/test-http-agent-keepalive.js index e4f5c09de2dbde..1cf5a597d0af0b 100644 --- a/test/parallel/test-http-agent-keepalive.js +++ b/test/parallel/test-http-agent-keepalive.js @@ -149,8 +149,9 @@ server.listen(0, common.mustCall(() => { function checkListeners(socket) { const callback = common.mustCall(() => { if (!socket.destroyed) { - // Sockets have freeSocketDataGuard while in the free pool. - assert.strictEqual(socket.listenerCount('data'), 1); + // Sockets have no public stream guard listeners while in the free pool. + assert.strictEqual(socket.listenerCount('readable'), 0); + assert.strictEqual(socket.listenerCount('data'), 0); assert.strictEqual(socket.listenerCount('drain'), 0); // Sockets have freeSocketErrorListener. assert.strictEqual(socket.listenerCount('error'), 1);