http: avoid stream listeners on idle agent sockets

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #64004Fixes: #63989
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.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,
@@ -60,6 +70,7 @@ const { getOptionValue } = require('internal/options');
 constkOnKeylog=Symbol('onkeylog');
 constkRequestOptions=Symbol('requestOptions');
 constkRequestAsyncResource=Symbol('requestAsyncResource');
+constkFreeSocketDataGuard=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
-functionfreeSocketDataGuard(){
-debug('DATA on FREE socket - destroying poisoned socket');
-this.destroy();
+functionfreeSocketOnReadGuard(){
+constnread=streamBaseState[kReadBytesOrError];
+if(nread===0)return;
+
+debug('READ on FREE socket - destroying poisoned socket');
+this[owner_symbol].destroy();
+}
+
+functioninstallFreeSocketDataGuard(socket){
+if(socket.readableLength>0){
+debug('BUFFERED DATA on FREE socket - destroying poisoned socket');
+socket.destroy();
+return;
+}
+
+if(socket.connecting){
+socket[kFreeSocketDataGuard]=functiononConnect(){
+socket[kFreeSocketDataGuard]=null;
+installFreeSocketDataGuard(socket);
+};
+socket.once('connect',socket[kFreeSocketDataGuard]);
+return;
+}
+
+consthandle=socket._handle;
+if(handle){
+handle.onread=freeSocketOnReadGuard;
+if(!handle.reading){
+handle.reading=true;
+consterr=handle.readStart();
+if(err)socket.destroy();
+}
+}
+}
+
+functionremoveFreeSocketDataGuard(socket){
+if(socket[kFreeSocketDataGuard]){
+socket.removeListener('connect',socket[kFreeSocketDataGuard]);
+socket[kFreeSocketDataGuard]=null;
+}
+
+consthandle=socket._handle;
+if(handle?.onread===freeSocketOnReadGuard){
+handle.onread=onStreamRead;
+}
 }
 
 functionAgent(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=functionreuseSocket(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.
 
 constcommon=require('../common');
 constassert=require('assert');
@@ -48,8 +48,9 @@ server.listen(0, common.mustCall(() => {
 assert.strictEqual(agent.freeSockets[name]?.length,1);
 constfreeSocket=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(() => {
 functioncheckListeners(socket){
 constcallback=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);