http: fix socket re-use races

Whether and when a socket is destroyed or not after a timeout is up to
the user. This leaves an edge case where a socket that has emitted
'timeout' might be re-used from the free pool. Even if destroy is called
on the socket, it won't be removed from the freelist until 'close' which
can happen several ticks later.
Sockets are removed from the free list on the 'close' event.
However, there is a delay between calling destroy() and 'close'
being emitted. This means that it possible for a socket that has
been destroyed to be re-used from the free list, causing unexpected
failures.
PR-URL: #32000
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Author: ronag

doc/api/http.md

@@ -239,6 +239,9 @@ added: v0.11.4
 An object which contains arrays of sockets currently awaiting use by
 the agent when `keepAlive` is enabled. Do not modify.
 
+Sockets in the `freeSockets` list will be automatically destroyed and
+removed from the array on `'timeout'`.
+
 ### `agent.getName(options)`
 <!-- YAML
 added: v0.11.4

lib/_http_agent.js

@@ -120,6 +120,12 @@ function Agent(options) {
 socket[async_id_symbol]=-1;
 socket._httpMessage=null;
 this.removeSocket(socket,options);
+
+constagentTimeout=this.options.timeout||0;
+if(socket.timeout!==agentTimeout){
+socket.setTimeout(agentTimeout);
+}
+
 freeSockets.push(socket);
 }else{
 // Implementation doesn't want to keep socket alive
@@ -202,12 +208,21 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
 this.sockets[name]=[];
 }
 
-constfreeLen=this.freeSockets[name] ? this.freeSockets[name].length : 0;
+constfreeSockets=this.freeSockets[name];
+letsocket;
+if(freeSockets){
+while(freeSockets.length&&freeSockets[0].destroyed){
+freeSockets.shift();
+}
+socket=freeSockets.shift();
+if(!freeSockets.length)
+deletethis.freeSockets[name];
+}
+
+constfreeLen=freeSockets ? freeSockets.length : 0;
 constsockLen=freeLen+this.sockets[name].length;
 
-if(freeLen){
-// We have a free socket, so use that.
-constsocket=this.freeSockets[name].shift();
+if(socket){
 // Guard against an uninitialized or user supplied Socket.
 consthandle=socket._handle;
 if(handle&&typeofhandle.asyncReset==='function'){
@@ -216,10 +231,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
 socket[async_id_symbol]=handle.getAsyncId();
 }
 
-// don't leak
-if(!this.freeSockets[name].length)
-deletethis.freeSockets[name];
-
 this.reuseSocket(socket,req);
 setRequestSocket(this,req,socket);
 this.sockets[name].push(socket);
@@ -319,6 +330,20 @@ function installListeners(agent, s, options) {
 }
 s.on('close',onClose);
 
+functiononTimeout(){
+debug('CLIENT socket onTimeout');
+
+// Destroy if in free list.
+// TODO(ronag): Always destroy, even if not in free list.
+constsockets=agent.freeSockets;
+for(constnameofObjectKeys(sockets)){
+if(sockets[name].includes(s)){
+returns.destroy();
+}
+}
+}
+s.on('timeout',onTimeout);
+
 functiononRemove(){
 // We need this function for cases like HTTP 'upgrade'
 // (defined by WebSockets) where we need to remove a socket from the
@@ -327,6 +352,7 @@ function installListeners(agent, s, options) {
 agent.removeSocket(s,options);
 s.removeListener('close',onClose);
 s.removeListener('free',onFree);
+s.removeListener('timeout',onTimeout);
 s.removeListener('agentRemove',onRemove);
 }
 s.on('agentRemove',onRemove);
@@ -409,14 +435,6 @@ function setRequestSocket(agent, req, socket) {
 return;
 }
 socket.setTimeout(req.timeout);
-// Reset timeout after response end
-req.once('response',(res)=>{
-res.once('end',()=>{
-if(socket.timeout!==agentTimeout){
-socket.setTimeout(agentTimeout);
-}
-});
-});
 }
 
 functionemitErrorNT(emitter,err){

test/parallel/test-http-agent-timeout-option.js

@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
 
 constlisteners=socket.listeners('timeout');
 
-strictEqual(listeners.length,1);
-strictEqual(listeners[0],request.timeoutCb);
+strictEqual(listeners.length,2);
+strictEqual(listeners[1],request.timeoutCb);
 }));

test/parallel/test-http-agent-timeout.js

@@ -0,0 +1,94 @@
+'use strict';
+
+constcommon=require('../common');
+constassert=require('assert');
+consthttp=require('http');
+
+{
+// Ensure reuse of successful sockets.
+
+constagent=newhttp.Agent({keepAlive: true});
+
+constserver=http.createServer((req,res)=>{
+res.end();
+});
+
+server.listen(0,common.mustCall(()=>{
+letsocket;
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+socket=res.socket;
+assert(socket);
+res.resume();
+socket.on('free',common.mustCall(()=>{
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+assert.strictEqual(socket,res.socket);
+assert(socket);
+agent.destroy();
+server.close();
+}));
+}));
+}));
+}));
+}
+
+{
+// Ensure that timeouted sockets are not reused.
+
+constagent=newhttp.Agent({keepAlive: true,timeout: 50});
+
+constserver=http.createServer((req,res)=>{
+res.end();
+});
+
+server.listen(0,common.mustCall(()=>{
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+constsocket=res.socket;
+assert(socket);
+res.resume();
+socket.on('free',common.mustCall(()=>{
+socket.on('timeout',common.mustCall(()=>{
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+assert.notStrictEqual(socket,res.socket);
+assert.strictEqual(socket.destroyed,true);
+agent.destroy();
+server.close();
+}));
+}));
+}));
+}));
+}));
+}
+
+{
+// Ensure that destroyed sockets are not reused.
+
+constagent=newhttp.Agent({keepAlive: true});
+
+constserver=http.createServer((req,res)=>{
+res.end();
+});
+
+server.listen(0,common.mustCall(()=>{
+letsocket;
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+socket=res.socket;
+assert(socket);
+res.resume();
+socket.on('free',common.mustCall(()=>{
+socket.destroy();
+http.get({port: server.address().port, agent })
+.on('response',common.mustCall((res)=>{
+assert.notStrictEqual(socket,res.socket);
+assert(socket);
+agent.destroy();
+server.close();
+}));
+}));
+}));
+}));
+}

test/parallel/test-http-client-set-timeout-after-end.js

@@ -20,7 +20,7 @@ server.listen(0, () => {
 constreq=get({ agent, port },(res)=>{
 res.on('end',()=>{
 strictEqual(req.setTimeout(0),req);
-strictEqual(socket.listenerCount('timeout'),0);
+strictEqual(socket.listenerCount('timeout'),1);
 agent.destroy();
 server.close();
 });

test/parallel/test-http-client-set-timeout.js

@@ -42,7 +42,7 @@ server.listen(0, mustCall(() => {
 }));
 
 req.on('timeout',mustCall(()=>{
-strictEqual(req.socket.listenerCount('timeout'),0);
+strictEqual(req.socket.listenerCount('timeout'),1);
 req.destroy();
 }));
 }));

test/parallel/test-http-client-timeout-option-listeners.js

@@ -24,9 +24,9 @@ const options = {
 server.listen(0,options.host,common.mustCall(()=>{
 options.port=server.address().port;
 doRequest(common.mustCall((numListeners)=>{
-assert.strictEqual(numListeners,1);
+assert.strictEqual(numListeners,2);
 doRequest(common.mustCall((numListeners)=>{
-assert.strictEqual(numListeners,1);
+assert.strictEqual(numListeners,2);
 server.close();
 agent.destroy();
 }));

test/parallel/test-http-client-timeout-option-with-agent.js

@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
 
 constlisteners=socket.listeners('timeout');
 
-strictEqual(listeners.length,1);
-strictEqual(listeners[0],request.timeoutCb);
+strictEqual(listeners.length,2);
+strictEqual(listeners[1],request.timeoutCb);
 }));