Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ce0ebd8

Browse filesBrowse files
martinslotaaduh95
martinslota
authored and
aduh95
committed
http: fix keep-alive socket reuse race in requestOnFinish
When the HTTP response ends before the request's 'finish' event fires, responseOnEnd() and requestOnFinish() can both call responseKeepAlive(), corrupting the socket that the agent may have already handed to another request. This commit adds a !req.destroyed guard to requestOnFinish() so the second call is skipped when the socket has already been released. Fixes: #60001 PR-URL: #61710 Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent da1a08a commit ce0ebd8
Copy full SHA for ce0ebd8

3 files changed

+219-1Lines changed: 219 additions & 1 deletion

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+5-1Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,11 @@ function responseOnTimeout() {
870870
function requestOnFinish() {
871871
const req = this;
872872

873-
if (req.shouldKeepAlive && req._ended)
873+
// If the response ends before this request finishes writing, `responseOnEnd()`
874+
// already released the socket. When `finish` fires later, that socket may
875+
// belong to a different request, so only call `responseKeepAlive()` when the
876+
// original request is still alive (`!req.destroyed`).
877+
if (req.shouldKeepAlive && req._ended && !req.destroyed)
874878
responseKeepAlive(req);
875879
}
876880

Collapse file

‎test/parallel/test-http-expect-continue-reuse-race.js‎

Copy file name to clipboard
+117Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
'use strict';
2+
3+
// Regression test for a keep-alive socket reuse race condition.
4+
//
5+
// The race is between responseOnEnd() and requestOnFinish(), both of which
6+
// can call responseKeepAlive(). The window is: req.end() has been called,
7+
// the socket write has completed (writableFinished true), but the write
8+
// callback that emits the 'finish' event has not fired yet.
9+
//
10+
// With plain HTTP the window is normally too narrow to hit. This test
11+
// widens it by delaying every client-socket write *callback* by a few
12+
// milliseconds (the actual I/O is not delayed, so writableFinished becomes
13+
// true while the 'finish'-emitting callback is still pending).
14+
//
15+
// With Expect: 100-continue, the server responds quickly while the client
16+
// delays req.end() just slightly (setTimeout 0), creating the perfect
17+
// timing for the response to arrive in that window.
18+
//
19+
// On unpatched Node, the double responseKeepAlive() call corrupts the
20+
// socket by stripping a subsequent request's listeners and emitting a
21+
// spurious 'free' event, causing requests to hang / time out.
22+
23+
const common = require('../common');
24+
const assert = require('assert');
25+
const http = require('http');
26+
27+
const REQUEST_COUNT = 100;
28+
const agent = new http.Agent({ keepAlive: true, maxSockets: 1 });
29+
30+
// Delay every write *callback* on the client socket so that
31+
// socket.writableLength drops to 0 (writableFinished becomes true) before
32+
// the callback that ultimately emits the 'finish' event fires. With
33+
// HTTPS the TLS layer provides this gap naturally; for plain HTTP we
34+
// need to create it artificially.
35+
const patchedSockets = new WeakSet();
36+
function patchSocket(socket) {
37+
if (patchedSockets.has(socket)) return;
38+
patchedSockets.add(socket);
39+
const delay = 5;
40+
const origWrite = socket.write;
41+
socket.write = function(chunk, encoding, cb) {
42+
if (typeof encoding === 'function') {
43+
cb = encoding;
44+
encoding = null;
45+
}
46+
if (typeof cb === 'function') {
47+
const orig = cb;
48+
cb = (...args) => setTimeout(() => orig(...args), delay);
49+
}
50+
return origWrite.call(this, chunk, encoding, cb);
51+
};
52+
}
53+
54+
const server = http.createServer(common.mustCall((req, res) => {
55+
req.on('error', common.mustNotCall());
56+
res.writeHead(200);
57+
res.end();
58+
}, REQUEST_COUNT));
59+
60+
server.listen(0, common.mustCall(() => {
61+
const { port } = server.address();
62+
63+
async function run() {
64+
try {
65+
for (let i = 0; i < REQUEST_COUNT; i++) {
66+
await sendRequest(port);
67+
}
68+
} finally {
69+
agent.destroy();
70+
server.close();
71+
}
72+
}
73+
74+
run().then(common.mustCall());
75+
}));
76+
77+
function sendRequest(port) {
78+
let timeout;
79+
const promise = new Promise((resolve, reject) => {
80+
function done(err) {
81+
clearTimeout(timeout);
82+
if (err)
83+
reject(err);
84+
else
85+
resolve();
86+
}
87+
88+
const req = http.request({
89+
port,
90+
host: '127.0.0.1',
91+
method: 'POST',
92+
agent,
93+
headers: {
94+
'Content-Length': '0',
95+
'Expect': '100-continue',
96+
},
97+
}, common.mustCall((res) => {
98+
assert.strictEqual(res.statusCode, 200);
99+
res.resume();
100+
res.once('end', done);
101+
res.once('error', done);
102+
}));
103+
104+
req.on('socket', patchSocket);
105+
106+
timeout = setTimeout(() => {
107+
const err = new Error('request timed out');
108+
req.destroy(err);
109+
done(err);
110+
}, common.platformTimeout(5000));
111+
112+
req.once('error', done);
113+
114+
setTimeout(() => req.end(Buffer.alloc(0)), 0);
115+
});
116+
return promise.finally(() => clearTimeout(timeout));
117+
}
Collapse file

‎test/parallel/test-https-expect-continue-reuse-race.js‎

Copy file name to clipboard
+97Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
'use strict';
2+
3+
// Regression test for a keep-alive socket reuse race condition.
4+
//
5+
// The race is between responseOnEnd() and requestOnFinish(), both of which
6+
// can call responseKeepAlive(). The window is: req.end() has been called,
7+
// the socket write has completed (writableFinished true), but the write
8+
// callback that emits the 'finish' event has not fired yet.
9+
//
10+
// HTTPS widens this window because the TLS layer introduces async
11+
// indirection between the actual write completion and the JS callback.
12+
//
13+
// With Expect: 100-continue, the server responds quickly while the client
14+
// delays req.end() just slightly (setTimeout 0), creating the perfect
15+
// timing for the response to arrive in that window.
16+
//
17+
// On unpatched Node, the double responseKeepAlive() call corrupts the
18+
// socket by stripping a subsequent request's listeners and emitting a
19+
// spurious 'free' event, causing requests to hang / time out.
20+
21+
const common = require('../common');
22+
23+
if (!common.hasCrypto)
24+
common.skip('missing crypto');
25+
26+
const assert = require('assert');
27+
const https = require('https');
28+
const fixtures = require('../common/fixtures');
29+
30+
const REQUEST_COUNT = 100;
31+
const agent = new https.Agent({ keepAlive: true, maxSockets: 1 });
32+
33+
const key = fixtures.readKey('agent1-key.pem');
34+
const cert = fixtures.readKey('agent1-cert.pem');
35+
const server = https.createServer({ key, cert }, common.mustCall((req, res) => {
36+
req.on('error', common.mustNotCall());
37+
res.writeHead(200);
38+
res.end();
39+
}, REQUEST_COUNT));
40+
41+
server.listen(0, common.mustCall(() => {
42+
const { port } = server.address();
43+
44+
async function run() {
45+
try {
46+
for (let i = 0; i < REQUEST_COUNT; i++) {
47+
await sendRequest(port);
48+
}
49+
} finally {
50+
agent.destroy();
51+
server.close();
52+
}
53+
}
54+
55+
run().then(common.mustCall());
56+
}));
57+
58+
function sendRequest(port) {
59+
let timeout;
60+
const promise = new Promise((resolve, reject) => {
61+
function done(err) {
62+
clearTimeout(timeout);
63+
if (err)
64+
reject(err);
65+
else
66+
resolve();
67+
}
68+
69+
const req = https.request({
70+
port,
71+
host: '127.0.0.1',
72+
rejectUnauthorized: false,
73+
method: 'POST',
74+
agent,
75+
headers: {
76+
'Content-Length': '0',
77+
'Expect': '100-continue',
78+
},
79+
}, common.mustCall((res) => {
80+
assert.strictEqual(res.statusCode, 200);
81+
res.resume();
82+
res.once('end', done);
83+
res.once('error', done);
84+
}));
85+
86+
timeout = setTimeout(() => {
87+
const err = new Error('request timed out');
88+
req.destroy(err);
89+
done(err);
90+
}, common.platformTimeout(5000));
91+
92+
req.once('error', done);
93+
94+
setTimeout(() => req.end(Buffer.alloc(0)), 0);
95+
});
96+
return promise.finally(() => clearTimeout(timeout));
97+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.