Skip to content

Commit ec1df7b

Browse files
OrKoNBethGriggs
authored andcommitted
http: fix incorrect headersTimeout measurement
For keep-alive connections, the headersTimeout may fire during subsequent request because the measurement was reset after a request and not before a request. Backport-PR-URL: #34131 PR-URL: #32329 Fixes: #27363 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent ca83634 commit ec1df7b

6 files changed

+103
-43
lines changed

lib/_http_client.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,8 @@ function tickOnSocket(req, socket) {
674674
parser.initialize(HTTPParser.RESPONSE,
675675
new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req),
676676
req.insecureHTTPParser === undefined ?
677-
isLenient() : req.insecureHTTPParser);
677+
isLenient() : req.insecureHTTPParser,
678+
0);
678679
parser.socket = socket;
679680
parser.outgoing = req;
680681
req.parser = parser;

lib/_http_common.js

+2
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;
5050
const kOnBody = HTTPParser.kOnBody | 0;
5151
const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0;
5252
const kOnExecute = HTTPParser.kOnExecute | 0;
53+
const kOnTimeout = HTTPParser.kOnTimeout | 0;
5354

5455
const MAX_HEADER_PAIRS = 2000;
5556

@@ -168,6 +169,7 @@ const parsers = new FreeList('parsers', 1000, function parsersCb() {
168169
parser[kOnHeadersComplete] = parserOnHeadersComplete;
169170
parser[kOnBody] = parserOnBody;
170171
parser[kOnMessageComplete] = parserOnMessageComplete;
172+
parser[kOnTimeout] = null;
171173

172174
return parser;
173175
});

lib/_http_server.js

+11-38
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ const { OutgoingMessage } = require('_http_outgoing');
4848
const {
4949
kOutHeaders,
5050
kNeedDrain,
51-
nowDate,
5251
emitStatistics
5352
} = require('internal/http');
5453
const {
@@ -143,6 +142,7 @@ const STATUS_CODES = {
143142
};
144143

145144
const kOnExecute = HTTPParser.kOnExecute | 0;
145+
const kOnTimeout = HTTPParser.kOnTimeout | 0;
146146

147147
class HTTPServerAsyncResource {
148148
constructor(type, socket) {
@@ -422,11 +422,9 @@ function connectionListenerInternal(server, socket) {
422422
new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket),
423423
server.insecureHTTPParser === undefined ?
424424
isLenient() : server.insecureHTTPParser,
425+
server.headersTimeout || 0,
425426
);
426427
parser.socket = socket;
427-
428-
// We are starting to wait for our headers.
429-
parser.parsingHeadersStart = nowDate();
430428
socket.parser = parser;
431429

432430
// Propagate headers limit from server instance to parser
@@ -478,6 +476,9 @@ function connectionListenerInternal(server, socket) {
478476
parser[kOnExecute] =
479477
onParserExecute.bind(undefined, server, socket, parser, state);
480478

479+
parser[kOnTimeout] =
480+
onParserTimeout.bind(undefined, server, socket);
481+
481482
socket._paused = false;
482483
}
483484

@@ -566,25 +567,15 @@ function socketOnData(server, socket, parser, state, d) {
566567

567568
function onParserExecute(server, socket, parser, state, ret) {
568569
socket._unrefTimer();
569-
const start = parser.parsingHeadersStart;
570570
debug('SERVER socketOnParserExecute %d', ret);
571+
onParserExecuteCommon(server, socket, parser, state, ret, undefined);
572+
}
571573

572-
// If we have not parsed the headers, destroy the socket
573-
// after server.headersTimeout to protect from DoS attacks.
574-
// start === 0 means that we have parsed headers, while
575-
// server.headersTimeout === 0 means user disabled this check.
576-
if (
577-
start !== 0 && server.headersTimeout &&
578-
nowDate() - start > server.headersTimeout
579-
) {
580-
const serverTimeout = server.emit('timeout', socket);
581-
582-
if (!serverTimeout)
583-
socket.destroy();
584-
return;
585-
}
574+
function onParserTimeout(server, socket) {
575+
const serverTimeout = server.emit('timeout', socket);
586576

587-
onParserExecuteCommon(server, socket, parser, state, ret, undefined);
577+
if (!serverTimeout)
578+
socket.destroy();
588579
}
589580

590581
const noop = () => {};
@@ -721,13 +712,6 @@ function emitCloseNT(self) {
721712
function parserOnIncoming(server, socket, state, req, keepAlive) {
722713
resetSocketTimeout(server, socket, state);
723714

724-
if (server.keepAliveTimeout > 0) {
725-
req.on('end', resetHeadersTimeoutOnReqEnd);
726-
}
727-
728-
// Set to zero to communicate that we have finished parsing.
729-
socket.parser.parsingHeadersStart = 0;
730-
731715
if (req.upgrade) {
732716
req.upgrade = req.method === 'CONNECT' ||
733717
server.listenerCount('upgrade') > 0;
@@ -852,17 +836,6 @@ function generateSocketListenerWrapper(originalFnName) {
852836
};
853837
}
854838

855-
function resetHeadersTimeoutOnReqEnd() {
856-
debug('resetHeadersTimeoutOnReqEnd');
857-
858-
const parser = this.socket.parser;
859-
// Parser can be null if the socket was destroyed
860-
// in that case, there is nothing to do.
861-
if (parser) {
862-
parser.parsingHeadersStart = nowDate();
863-
}
864-
}
865-
866839
module.exports = {
867840
STATUS_CODES,
868841
Server,

src/node_http_parser_impl.h

+35-2
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ const uint32_t kOnHeadersComplete = 1;
7878
const uint32_t kOnBody = 2;
7979
const uint32_t kOnMessageComplete = 3;
8080
const uint32_t kOnExecute = 4;
81+
const uint32_t kOnTimeout = 5;
8182
// Any more fields than this will be flushed into JS
8283
const size_t kMaxHeaderFieldsCount = 32;
8384

@@ -185,6 +186,7 @@ class Parser : public AsyncWrap, public StreamListener {
185186
num_fields_ = num_values_ = 0;
186187
url_.Reset();
187188
status_message_.Reset();
189+
header_parsing_start_time_ = uv_hrtime();
188190
return 0;
189191
}
190192

@@ -518,9 +520,16 @@ class Parser : public AsyncWrap, public StreamListener {
518520
Environment* env = Environment::GetCurrent(args);
519521
bool lenient = args[2]->IsTrue();
520522

523+
uint64_t headers_timeout = 0;
524+
521525
CHECK(args[0]->IsInt32());
522526
CHECK(args[1]->IsObject());
523527

528+
if (args.Length() > 3) {
529+
CHECK(args[3]->IsInt32());
530+
headers_timeout = args[3].As<Int32>()->Value();
531+
}
532+
524533
parser_type_t type =
525534
static_cast<parser_type_t>(args[0].As<Int32>()->Value());
526535

@@ -537,7 +546,7 @@ class Parser : public AsyncWrap, public StreamListener {
537546

538547
parser->set_provider_type(provider);
539548
parser->AsyncReset(args[1].As<Object>());
540-
parser->Init(type, lenient);
549+
parser->Init(type, lenient, headers_timeout);
541550
}
542551

543552
template <bool should_pause>
@@ -645,6 +654,24 @@ class Parser : public AsyncWrap, public StreamListener {
645654
if (ret.IsEmpty())
646655
return;
647656

657+
// check header parsing time
658+
if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) {
659+
uint64_t now = uv_hrtime();
660+
uint64_t parsing_time = (now - header_parsing_start_time_) / 1e6;
661+
662+
if (parsing_time > headers_timeout_) {
663+
Local<Value> cb =
664+
object()->Get(env()->context(), kOnTimeout).ToLocalChecked();
665+
666+
if (!cb->IsFunction())
667+
return;
668+
669+
MakeCallback(cb.As<Function>(), 0, nullptr);
670+
671+
return;
672+
}
673+
}
674+
648675
Local<Value> cb =
649676
object()->Get(env()->context(), kOnExecute).ToLocalChecked();
650677

@@ -821,7 +848,7 @@ class Parser : public AsyncWrap, public StreamListener {
821848
}
822849

823850

824-
void Init(parser_type_t type, bool lenient) {
851+
void Init(parser_type_t type, bool lenient, uint64_t headers_timeout) {
825852
#ifdef NODE_EXPERIMENTAL_HTTP
826853
llhttp_init(&parser_, type, &settings);
827854
llhttp_set_lenient(&parser_, lenient);
@@ -836,6 +863,8 @@ class Parser : public AsyncWrap, public StreamListener {
836863
num_values_ = 0;
837864
have_flushed_ = false;
838865
got_exception_ = false;
866+
header_parsing_start_time_ = 0;
867+
headers_timeout_ = headers_timeout;
839868
}
840869

841870

@@ -884,6 +913,8 @@ class Parser : public AsyncWrap, public StreamListener {
884913
bool pending_pause_ = false;
885914
uint64_t header_nread_ = 0;
886915
#endif /* NODE_EXPERIMENTAL_HTTP */
916+
uint64_t headers_timeout_;
917+
uint64_t header_parsing_start_time_ = 0;
887918

888919
// These are helper functions for filling `http_parser_settings`, which turn
889920
// a member function of Parser into a C-style HTTP parser callback.
@@ -957,6 +988,8 @@ void InitializeHttpParser(Local<Object> target,
957988
Integer::NewFromUnsigned(env->isolate(), kOnMessageComplete));
958989
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnExecute"),
959990
Integer::NewFromUnsigned(env->isolate(), kOnExecute));
991+
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnTimeout"),
992+
Integer::NewFromUnsigned(env->isolate(), kOnTimeout));
960993

961994
Local<Array> methods = Array::New(env->isolate());
962995
#define V(num, name, string) \

test/async-hooks/test-graph.http.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ process.on('exit', () => {
4444
id: 'httpincomingmessage:1',
4545
triggerAsyncId: 'tcp:2' },
4646
{ type: 'Timeout',
47-
id: 'timeout:2',
48-
triggerAsyncId: 'tcp:2' },
47+
id: 'timeout:1',
48+
triggerAsyncId: 'httpincomingmessage:1' },
4949
{ type: 'SHUTDOWNWRAP',
5050
id: 'shutdown:1',
5151
triggerAsyncId: 'tcp:2' } ]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const http = require('http');
5+
const net = require('net');
6+
const { finished } = require('stream');
7+
8+
const headers =
9+
'GET / HTTP/1.1\r\n' +
10+
'Host: localhost\r\n' +
11+
'Connection: keep-alive\r\n' +
12+
'Agent: node\r\n';
13+
14+
const baseTimeout = 1000;
15+
16+
const server = http.createServer(common.mustCall((req, res) => {
17+
req.resume();
18+
res.writeHead(200);
19+
res.end();
20+
}, 2));
21+
22+
server.keepAliveTimeout = 10 * baseTimeout;
23+
server.headersTimeout = baseTimeout;
24+
25+
server.once('timeout', common.mustNotCall((socket) => {
26+
socket.destroy();
27+
}));
28+
29+
server.listen(0, () => {
30+
const client = net.connect(server.address().port);
31+
32+
// first request
33+
client.write(headers);
34+
client.write('\r\n');
35+
36+
setTimeout(() => {
37+
// second request
38+
client.write(headers);
39+
// `headersTimeout` doesn't seem to fire if request
40+
// is sent altogether.
41+
setTimeout(() => {
42+
client.write('\r\n');
43+
client.end();
44+
}, 10);
45+
}, baseTimeout + 10);
46+
47+
client.resume();
48+
finished(client, common.mustCall((err) => {
49+
server.close();
50+
}));
51+
});

0 commit comments

Comments
 (0)