FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: avoid stream listeners on idle agent sockets · nodejs/node@44c8ebc · GitHub

/ node Public

Commit 44c8ebc

Browse files
mcollina
authored and
sxa
committed
http: avoid stream listeners on idle agent sockets
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #64004 Fixes: #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>
1 parent 61b20f6 commit 44c8ebc

3 files changed

Lines changed: 67 additions & 13 deletions

File tree

‎lib/_http_agent.js‎

Lines changed: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,22 @@ const {
4343
getGlobalAgent,
4444
} = require('internal/http');
4545
const { AsyncResource } = require('async_hooks');
46-
const { async_id_symbol } = require('internal/async_hooks').symbols;
46+
const {
47+
async_id_symbol,
48+
owner_symbol,
49+
} = require('internal/async_hooks').symbols;
4750
const {
4851
getLazy,
4952
kEmptyObject,
5053
once,
5154
} = require('internal/util');
55+
const {
56+
onStreamRead,
57+
} = require('internal/stream_base_commons');
58+
const {
59+
kReadBytesOrError,
60+
streamBaseState,
61+
} = internalBinding('stream_wrap');
5262
const {
5363
validateNumber,
5464
validateOneOf,
@@ -60,6 +70,7 @@ const { getOptionValue } = require('internal/options');
6070
const kOnKeylog = Symbol('onkeylog');
6171
const kRequestOptions = Symbol('requestOptions');
6272
const kRequestAsyncResource = Symbol('requestAsyncResource');
73+
const kFreeSocketDataGuard = Symbol('freeSocketDataGuard');
6374

6475
// New Agent code.
6576

@@ -92,9 +103,51 @@ function freeSocketErrorListener(err) {
92103
// in the TCP buffer and be silently consumed as the response for the
93104
// *next* request that reuses the socket (response-queue poisoning).
94105
// See: https://hackerone.com/reports/3582376
95-
function freeSocketDataGuard() {
96-
debug('DATA on FREE socket - destroying poisoned socket');
97-
this.destroy();
106+
function freeSocketOnReadGuard() {
107+
const nread = streamBaseState[kReadBytesOrError];
108+
if (nread === 0) return;
109+
110+
debug('READ on FREE socket - destroying poisoned socket');
111+
this[owner_symbol].destroy();
112+
}
113+
114+
function installFreeSocketDataGuard(socket) {
115+
if (socket.readableLength > 0) {
116+
debug('BUFFERED DATA on FREE socket - destroying poisoned socket');
117+
socket.destroy();
118+
return;
119+
}
120+
121+
if (socket.connecting) {
122+
socket[kFreeSocketDataGuard] = function onConnect() {
123+
socket[kFreeSocketDataGuard] = null;
124+
installFreeSocketDataGuard(socket);
125+
};
126+
socket.once('connect', socket[kFreeSocketDataGuard]);
127+
return;
128+
}
129+
130+
const handle = socket._handle;
131+
if (handle) {
132+
handle.onread = freeSocketOnReadGuard;
133+
if (!handle.reading) {
134+
handle.reading = true;
135+
const err = handle.readStart();
136+
if (err) socket.destroy();
137+
}
138+
}
139+
}
140+
141+
function removeFreeSocketDataGuard(socket) {
142+
if (socket[kFreeSocketDataGuard]) {
143+
socket.removeListener('connect', socket[kFreeSocketDataGuard]);
144+
socket[kFreeSocketDataGuard] = null;
145+
}
146+
147+
const handle = socket._handle;
148+
if (handle?.onread === freeSocketOnReadGuard) {
149+
handle.onread = onStreamRead;
150+
}
98151
}
99152

100153
function Agent(options) {
@@ -207,8 +260,7 @@ function Agent(options) {
207260
this.removeSocket(socket, options);
208261

209262
socket.once('error', freeSocketErrorListener);
210-
socket.on('data', freeSocketDataGuard);
211-
socket.resume();
263+
installFreeSocketDataGuard(socket);
212264
freeSockets.push(socket);
213265
});
214266

@@ -600,7 +652,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
600652
Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
601653
debug('have free socket');
602654
socket.removeListener('error', freeSocketErrorListener);
603-
socket.removeListener('data', freeSocketDataGuard);
655+
removeFreeSocketDataGuard(socket);
604656
req.reusedSocket = true;
605657
socket.ref();
606658
};

‎test/parallel/test-http-agent-free-socket-data-guard.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88
// writes a full HTTP response during this window, it is consumed as the
99
// response for the *next* request — poisoning the response queue.
1010
//
11-
// The fix attaches a data guard listener + resume() on idle sockets so
12-
// that unsolicited data causes the socket to be destroyed.
11+
// The fix installs a read guard on idle sockets so that unsolicited data
12+
// causes the socket to be destroyed without adding public stream listeners.
1313

1414
const common = require('../common');
1515
const assert = require('assert');
@@ -48,8 +48,9 @@ server.listen(0, common.mustCall(() => {
4848
assert.strictEqual(agent.freeSockets[name]?.length, 1);
4949
const freeSocket = agent.freeSockets[name][0];
5050
assert.strictEqual(freeSocket.parser, null);
51-
// With the fix, a data guard listener is attached
52-
assert.strictEqual(freeSocket.listenerCount('data'), 1);
51+
// With the fix, no public stream listeners are added.
52+
assert.strictEqual(freeSocket.listenerCount('data'), 0);
53+
assert.strictEqual(freeSocket.listenerCount('readable'), 0);
5354

5455
// Step 2: Server injects a poisoned response while socket is idle
5556
serverSocket.write(

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,9 @@ server.listen(0, common.mustCall(() => {
149149
function checkListeners(socket) {
150150
const callback = common.mustCall(() => {
151151
if (!socket.destroyed) {
152-
// Sockets have freeSocketDataGuard while in the free pool.
153-
assert.strictEqual(socket.listenerCount('data'), 1);
152+
// Sockets have no public stream guard listeners while in the free pool.
153+
assert.strictEqual(socket.listenerCount('readable'), 0);
154+
assert.strictEqual(socket.listenerCount('data'), 0);
154155
assert.strictEqual(socket.listenerCount('drain'), 0);
155156
// Sockets have freeSocketErrorListener.
156157
assert.strictEqual(socket.listenerCount('error'), 1);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL