Skip to content

Commit

Permalink
http: fix socket re-use races
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
ronag authored and targos committed Apr 20, 2020
1 parent 7d66cea commit f9123eb
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 24 deletions.
3 changes: 3 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 34 additions & 16 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,12 @@ function Agent(options) {
socket[async_id_symbol] = -1;
socket._httpMessage = null;
this.removeSocket(socket, options);

const agentTimeout = this.options.timeout || 0;
if (socket.timeout !== agentTimeout) {
socket.setTimeout(agentTimeout);
}

freeSockets.push(socket);
} else {
// Implementation doesn't want to keep socket alive
Expand Down Expand Up @@ -197,12 +203,21 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
this.sockets[name] = [];
}

const freeLen = this.freeSockets[name] ? this.freeSockets[name].length : 0;
const freeSockets = this.freeSockets[name];
let socket;
if (freeSockets) {
while (freeSockets.length && freeSockets[0].destroyed) {
freeSockets.shift();
}
socket = freeSockets.shift();
if (!freeSockets.length)
delete this.freeSockets[name];
}

const freeLen = freeSockets ? freeSockets.length : 0;
const sockLen = freeLen + this.sockets[name].length;

if (freeLen) {
// We have a free socket, so use that.
const socket = this.freeSockets[name].shift();
if (socket) {
// Guard against an uninitialized or user supplied Socket.
const handle = socket._handle;
if (handle && typeof handle.asyncReset === 'function') {
Expand All @@ -211,10 +226,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
socket[async_id_symbol] = handle.getAsyncId();
}

// don't leak
if (!this.freeSockets[name].length)
delete this.freeSockets[name];

this.reuseSocket(socket, req);
setRequestSocket(this, req, socket);
this.sockets[name].push(socket);
Expand Down Expand Up @@ -309,6 +320,20 @@ function installListeners(agent, s, options) {
}
s.on('close', onClose);

function onTimeout() {
debug('CLIENT socket onTimeout');

// Destroy if in free list.
// TODO(ronag): Always destroy, even if not in free list.
const sockets = agent.freeSockets;
for (const name of ObjectKeys(sockets)) {
if (sockets[name].includes(s)) {
return s.destroy();
}
}
}
s.on('timeout', onTimeout);

function onRemove() {
// We need this function for cases like HTTP 'upgrade'
// (defined by WebSockets) where we need to remove a socket from the
Expand All @@ -317,6 +342,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);
Expand Down Expand Up @@ -399,14 +425,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);
}
});
});
}

function emitErrorNT(emitter, err) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-agent-timeout-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {

const listeners = socket.listeners('timeout');

strictEqual(listeners.length, 1);
strictEqual(listeners[0], request.timeoutCb);
strictEqual(listeners.length, 2);
strictEqual(listeners[1], request.timeoutCb);
}));
94 changes: 94 additions & 0 deletions test/parallel/test-http-agent-timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');

{
// Ensure reuse of successful sockets.

const agent = new http.Agent({ keepAlive: true });

const server = http.createServer((req, res) => {
res.end();
});

server.listen(0, common.mustCall(() => {
let socket;
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.

const agent = new http.Agent({ keepAlive: true, timeout: 50 });

const server = http.createServer((req, res) => {
res.end();
});

server.listen(0, common.mustCall(() => {
http.get({ port: server.address().port, agent })
.on('response', common.mustCall((res) => {
const socket = 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.

const agent = new http.Agent({ keepAlive: true });

const server = http.createServer((req, res) => {
res.end();
});

server.listen(0, common.mustCall(() => {
let socket;
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();
}));
}));
}));
}));
}
2 changes: 1 addition & 1 deletion test/parallel/test-http-client-set-timeout-after-end.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ server.listen(0, () => {
const req = 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();
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http-client-set-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
}));
4 changes: 2 additions & 2 deletions test/parallel/test-http-client-timeout-option-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-client-timeout-option-with-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {

const listeners = socket.listeners('timeout');

strictEqual(listeners.length, 1);
strictEqual(listeners[0], request.timeoutCb);
strictEqual(listeners.length, 2);
strictEqual(listeners[1], request.timeoutCb);
}));

0 comments on commit f9123eb

Please sign in to comment.