8000 http: fix socket re-use races · nodejs/node@1ffa9f3 · GitHub
[go: up one dir, main page]
Skip to content

Commit 1ffa9f3

Browse files
ronagMylesBorins
ronag
authored and
MylesBorins
committed
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>
1 parent 55486bc commit 1ffa9f3

8 files changed

+139
-24
lines changed

‎doc/api/http.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,9 @@ added: v0.11.4
239239
An object which contains arrays of sockets currently awaiting use by
240240
the agent when `keepAlive` is enabled. Do not modify.
241241

242+
Sockets in the `freeSockets` list will be automatically destroyed and
243+
removed from the array on `'timeout'`.
244+
242245
### `agent.getName(options)`
243246
<!-- YAML
244247
added: v0.11.4

‎lib/_http_agent.js‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,12 @@ function Agent(options) {
120120
socket[async_id_symbol] = -1;
121121
socket._httpMessage = null;
122122
this.removeSocket(socket, options);
123+
124+
const agentTimeout = this.options.timeout || 0;
125+
if (socket.timeout !== agentTimeout) {
126+
socket.setTimeout(agentTimeout);
127+
}
128+
123129
freeSockets.push(socket);
124130
} else {
125131
// Implementation doesn't want to keep socket alive
@@ -202,12 +208,21 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
202208
this.sockets[name] = [];
203209
}
204210

205-
const freeLen = this.freeSockets[name] ? this.freeSockets[name].length : 0;
211+
const freeSockets = this.freeSockets[name];
212+
let socket;
213+
if (freeSockets) {
214+
while (freeSockets.length && freeSockets[0].destroyed) {
215+
freeSockets.shift();
216+
}
217+
socket = freeSockets.shift();
218+
if (!freeSockets.length)
219+
delete this.freeSockets[name];
220+
}
221+
222+
const freeLen = freeSockets ? freeSockets.length : 0;
206223
const sockLen = freeLen + this.sockets[name].length;
207224

208-
if (freeLen) {
209-
// We have a free socket, so use that.
210-
const socket = this.freeSockets[name].shift();
225+
if (socket) {
211226
// Guard against an uninitialized or user supplied Socket.
212227
const handle = socket._handle;
213228
if (handle && typeof handle.asyncReset === 'function') {
@@ -216,10 +231,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
216231
socket[async_id_symbol] = handle.getAsyncId();
217232
}
218233

219-
// don't leak
220-
if (!this.freeSockets[name].length)
221-
delete this.freeSockets[name];
222-
223234
this.reuseSocket(socket, req);
224235
setRequestSocket(this, req, socket);
225236
this.sockets[name].push(socket);
@@ -319,6 +330,20 @@ function installListeners(agent, s, options) {
319330
}
320331
s.on('close', onClose);
321332

333+
function onTimeout() {
334+
debug('CLIENT socket onTimeout');
335+
336+
// Destroy if in free list.
337+
// TODO(ronag): Always destroy, even if not in free list.
338+
const sockets = agent.freeSockets;
339+
for (const name of ObjectKeys(sockets)) {
340+
if (sockets[name].includes(s)) {
341+
return s.destroy();
342+
}
343+
}
344+
}
345+
s.on('timeout', onTimeout);
346+
322347
function onRemove() {
323348
// We need this function for cases like HTTP 'upgrade'
324349
// (defined by WebSockets) where we need to remove a socket from the
@@ -327,6 +352,7 @@ function installListeners(agent, s, options) {
327352
agent.removeSocket(s, options);
328353
s.removeListener('close', onClose);
329354
s.removeListener('free', onFree);
355+
s.removeListener('timeout', onTimeout);
330356
s.removeListener('agentRemove', onRemove);
331357
}
332358
s.on('agentRemove', onRemove);
@@ -409,14 +435,6 @@ function setRequestSocket(agent, req, socket) {
409435
return;
410436
}
411437
socket.setTimeout(req.timeout);
412-
// Reset timeout after response end
413-
req.once('response', (res) => {
414-
res.once('end', () => {
415-
if (socket.timeout !== agentTimeout) {
416-
socket.setTimeout(agentTimeout);
417-
}
418-
});
419-
});
420438
}
421439

422440
function emitErrorNT(emitter, err) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
1818

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

21-
strictEqual(listeners.length, 1);
22-
strictEqual(listeners[0], request.timeoutCb);
21+
strictEqual(listeners.length, 2);
22+
strictEqual(listeners[1], request.timeoutCb);
2323
}));

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
7+
{
8+
// Ensure reuse of successful sockets.
9+
10+
const agent = new http.Agent({ keepAlive: true });
11+
12+
const server = http.createServer((req, res) => {
13+
res.end();
14+
});
15+
16+
server.listen(0, common.mustCall(() => {
17+
let socket;
18+
http.get({ port: server.address().port, agent })
19+
.on('response', common.mustCall((res) => {
20+
socket = res.socket;
21+
assert(socket);
22+
res.resume();
23+
socket.on('free', common.mustCall(() => {
24+
http.get({ port: server.address().port, agent })
25+
.on('response', common.mustCall((res) => {
26+
assert.strictEqual(socket, res.socket);
27+
assert(socket);
28+
agent.destroy();
29+
server.close();
30+
}));
31+
}));
32+
}));
33+
}));
34+
}
35+
36+
{
37+
// Ensure that timeouted sockets are not reused.
38+
39+
const agent = new http.Agent({ keepAlive: true, timeout: 50 });
40+
41+
const server = http.createServer((req, res) => {
42+
res.end();
43+
});
44+
45+
server.listen(0, common.mustCall(() => {
46+
http.get({ port: server.address().port, agent })
47+
.on('response', common.mustCall((res) => {
48+
const socket = res.socket;
49+
assert(socket);
50+
55C6 res.resume();
51+
socket.on('free', common.mustCall(() => {
52+
socket.on('timeout', common.mustCall(() => {
53+
http.get({ port: server.address().port, agent })
54+
.on('response', common.mustCall((res) => {
55+
assert.notStrictEqual(socket, res.socket);
56+
assert.strictEqual(socket.destroyed, true);
57+
agent.destroy();
58+
server.close();
59+
}));
60+
}));
61+
}));
62+
}));
63+
}));
64+
}
65+
66+
{
67+
// Ensure that destroyed sockets are not reused.
68+
69+
const agent = new http.Agent({ keepAlive: true });
70+
71+
const server = http.createServer((req, res) => {
72+
res.end();
73+
});
74+
75+
server.listen(0, common.mustCall(() => {
76+
let socket;
77+
http.get({ port: server.address().port, agent })
78+
.on('response', common.mustCall((res) => {
79+
socket = res.socket;
80+
assert(socket);
81+
res.resume();
82+
socket.on('free', common.mustCall(() => {
83+
socket.destroy();
84+
http.get({ port: server.address().port, agent })
85+
.on('response', common.mustCall((res) => {
86+
assert.notStrictEqual(socket, res.socket);
87+
assert(socket);
88+
agent.destroy();
89+
server.close();
90+
}));
91+
}));
92+
}));
93+
}));
94+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ server.listen(0, () => {
2020
const req = get({ agent, port }, (res) => {
2121
res.on('end', () => {
2222
strictEqual(req.setTimeout(0), req);
23-
strictEqual(socket.listenerCount('timeout'), 0);
23+
strictEqual(socket.listenerCount('timeout'), 1);
2424
agent.destroy();
2525
server.close();
2626
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ server.listen(0, mustCall(() => {
4242
}));
4343

4444
req.on('timeout', mustCall(() => {
45-
strictEqual(req.socket.listenerCount('timeout'), 0);
45+
strictEqual(req.socket.listenerCount('timeout'), 1);
4646
req.destroy();
4747
}));
4848
}));

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ const options = {
2424
server.listen(0, options.host, common.mustCall(() => {
2525
options.port = server.address().port;
2626
doRequest(common.mustCall((numListeners) => {
27-
assert.strictEqual(numListeners, 1);
27+
assert.strictEqual(numListeners, 2);
2828
doRequest(common.mustCall((numListeners) => {
29-
assert.strictEqual(numListeners, 1);
29+
assert.strictEqual(numListeners, 2);
3030
server.close();
3131
agent.destroy();
3232
}));

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
1818

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

21-
strictEqual(listeners.length, 1);
22-
strictEqual(listeners[0], request.timeoutCb);
21+
strictEqual(listeners.length, 2);
22+
strictEqual(listeners[1], request.timeoutCb);
2323
}));

0 commit comments

Comments
 (0)
0