http: avoid stream listeners on idle agent sockets

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

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,
@@ -92,9 +102,37 @@ 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;
+  }
+
+  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) {
+  const handle = socket._handle;
+  if (handle?.onread === freeSocketOnReadGuard) {
+    handle.onread = onStreamRead;
+  }
 }
 
 function Agent(options) {
@@ -207,8 +245,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 +637,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();
 };

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(

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);