From ed7b316bc9eb4d309c53bcada00db50e093f1f5c Mon Sep 17 00:00:00 2001 From: Alex R Date: Wed, 30 Oct 2019 23:33:33 +0100 Subject: [PATCH 1/4] 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. Fixes: https://github.com/nodejs/node/issues/27363 --- lib/_http_client.js | 3 +- lib/_http_common.js | 2 + lib/_http_server.js | 46 +++++------------ src/node_http_parser.cc | 34 ++++++++++++- ...low-headers-keepalive-multiple-requests.js | 51 +++++++++++++++++++ 5 files changed, 99 insertions(+), 37 deletions(-) create mode 100644 test/parallel/test-http-slow-headers-keepalive-multiple-requests.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 3f335aa6eaa7ce..28d28b95a0a5f7 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -727,7 +727,8 @@ function tickOnSocket(req, socket) { new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req), req.maxHeaderSize || 0, req.insecureHTTPParser === undefined ? - isLenient() : req.insecureHTTPParser); + isLenient() : req.insecureHTTPParser, + 0); parser.socket = socket; parser.outgoing = req; req.parser = parser; diff --git a/lib/_http_common.js b/lib/_http_common.js index f1386e1a0915dd..eec965d7fcf8f1 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -47,6 +47,7 @@ const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; const kOnBody = HTTPParser.kOnBody | 0; const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; const kOnExecute = HTTPParser.kOnExecute | 0; +const kOnTimeout = HTTPParser.kOnTimeout | 0; const MAX_HEADER_PAIRS = 2000; @@ -165,6 +166,7 @@ const parsers = new FreeList('parsers', 1000, function parsersCb() { parser[kOnHeadersComplete] = parserOnHeadersComplete; parser[kOnBody] = parserOnBody; parser[kOnMessageComplete] = parserOnMessageComplete; + parser[kOnTimeout] = null; return parser; }); diff --git a/lib/_http_server.js b/lib/_http_server.js index 3f9c98e8380363..041185a3450684 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -142,6 +142,7 @@ const STATUS_CODES = { }; const kOnExecute = HTTPParser.kOnExecute | 0; +const kOnTimeout = HTTPParser.kOnTimeout | 0; class HTTPServerAsyncResource { constructor(type, socket) { @@ -426,11 +427,9 @@ function connectionListenerInternal(server, socket) { server.maxHeaderSize || 0, server.insecureHTTPParser === undefined ? isLenient() : server.insecureHTTPParser, + server.headersTimeout, ); parser.socket = socket; - - // We are starting to wait for our headers. - parser.parsingHeadersStart = nowDate(); socket.parser = parser; // Propagate headers limit from server instance to parser @@ -482,6 +481,9 @@ function connectionListenerInternal(server, socket) { parser[kOnExecute] = onParserExecute.bind(undefined, server, socket, parser, state); + parser[kOnTimeout] = + onParserTimeout.bind(undefined, server, socket, parser, state); + socket._paused = false; } @@ -570,21 +572,15 @@ function socketOnData(server, socket, parser, state, d) { function onParserExecute(server, socket, parser, state, ret) { socket._unrefTimer(); - const start = parser.parsingHeadersStart; debug('SERVER socketOnParserExecute %d', ret); + onParserExecuteCommon(server, socket, parser, state, ret, undefined); +} - // If we have not parsed the headers, destroy the socket - // after server.headersTimeout to protect from DoS attacks. - // start === 0 means that we have parsed headers. - if (start !== 0 && nowDate() - start > server.headersTimeout) { - const serverTimeout = server.emit('timeout', socket); +function onParserTimeout(server, socket, parser, state, ret) { + const serverTimeout = server.emit('timeout', socket); - if (!serverTimeout) - socket.destroy(); - return; - } - - onParserExecuteCommon(server, socket, parser, state, ret, undefined); + if (!serverTimeout) + socket.destroy(); } const noop = () => {}; @@ -720,13 +716,6 @@ function emitCloseNT(self) { function parserOnIncoming(server, socket, state, req, keepAlive) { resetSocketTimeout(server, socket, state); - if (server.keepAliveTimeout > 0) { - req.on('end', resetHeadersTimeoutOnReqEnd); - } - - // Set to zero to communicate that we have finished parsing. - socket.parser.parsingHeadersStart = 0; - if (req.upgrade) { req.upgrade = req.method === 'CONNECT' || server.listenerCount('upgrade') > 0; @@ -851,21 +840,10 @@ function generateSocketListenerWrapper(originalFnName) { }; } -function resetHeadersTimeoutOnReqEnd() { - debug('resetHeadersTimeoutOnReqEnd'); - - const parser = this.socket.parser; - // Parser can be null if the socket was destroyed - // in that case, there is nothing to do. - if (parser) { - parser.parsingHeadersStart = nowDate(); - } -} - module.exports = { STATUS_CODES, Server, ServerResponse, _connectionListener: connectionListener, kServerResponse -}; +}; \ No newline at end of file diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 75d7e89a91c34f..bdc5f945fb1d7e 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -74,6 +74,7 @@ const uint32_t kOnHeadersComplete = 1; const uint32_t kOnBody = 2; const uint32_t kOnMessageComplete = 3; const uint32_t kOnExecute = 4; +const uint32_t kOnTimeout = 5; // Any more fields than this will be flushed into JS const size_t kMaxHeaderFieldsCount = 32; @@ -181,6 +182,7 @@ class Parser : public AsyncWrap, public StreamListener { num_fields_ = num_values_ = 0; url_.Reset(); status_message_.Reset(); + header_parsing_start_time_ = uv_hrtime(); return 0; } @@ -504,6 +506,7 @@ class Parser : public AsyncWrap, public StreamListener { bool lenient = args[3]->IsTrue(); uint64_t max_http_header_size = 0; + uint64_t headers_timeout = 0; CHECK(args[0]->IsInt32()); CHECK(args[1]->IsObject()); @@ -516,6 +519,9 @@ class Parser : public AsyncWrap, public StreamListener { max_http_header_size = env->options()->max_http_header_size; } + CHECK(args[4]->IsInt32()); + headers_timeout = args[4].As()->Value(); + llhttp_type_t type = static_cast(args[0].As()->Value()); @@ -532,7 +538,7 @@ class Parser : public AsyncWrap, public StreamListener { parser->set_provider_type(provider); parser->AsyncReset(args[1].As()); - parser->Init(type, max_http_header_size, lenient); + parser->Init(type, max_http_header_size, lenient, headers_timeout); } template @@ -636,6 +642,24 @@ class Parser : public AsyncWrap, public StreamListener { if (ret.IsEmpty()) return; + // check header parsing time + if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) { + auto now = uv_hrtime(); + auto parsing_time = (now - header_parsing_start_time_) / 1e6; + + if (parsing_time > headers_timeout_) { + Local cb = + object()->Get(env()->context(), kOnTimeout).ToLocalChecked(); + + if (!cb->IsFunction()) + return; + + MakeCallback(cb.As(), 0, nullptr); + + return; + } + } + Local cb = object()->Get(env()->context(), kOnExecute).ToLocalChecked(); @@ -779,7 +803,7 @@ class Parser : public AsyncWrap, public StreamListener { } - void Init(llhttp_type_t type, uint64_t max_http_header_size, bool lenient) { + void Init(llhttp_type_t type, uint64_t max_http_header_size, bool lenient, uint64_t headers_timeout) { llhttp_init(&parser_, type, &settings); llhttp_set_lenient(&parser_, lenient); header_nread_ = 0; @@ -790,6 +814,8 @@ class Parser : public AsyncWrap, public StreamListener { have_flushed_ = false; got_exception_ = false; max_http_header_size_ = max_http_header_size; + header_parsing_start_time_ = 0; + headers_timeout_ = headers_timeout; } @@ -831,6 +857,8 @@ class Parser : public AsyncWrap, public StreamListener { bool pending_pause_ = false; uint64_t header_nread_ = 0; uint64_t max_http_header_size_; + uint64_t headers_timeout_; + uint64_t header_parsing_start_time_ = 0; // These are helper functions for filling `http_parser_settings`, which turn // a member function of Parser into a C-style HTTP parser callback. @@ -890,6 +918,8 @@ void InitializeHttpParser(Local target, Integer::NewFromUnsigned(env->isolate(), kOnMessageComplete)); t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnExecute"), Integer::NewFromUnsigned(env->isolate(), kOnExecute)); + t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnTimeout"), + Integer::NewFromUnsigned(env->isolate(), kOnTimeout)); Local methods = Array::New(env->isolate()); #define V(num, name, string) \ diff --git a/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js b/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js new file mode 100644 index 00000000000000..9ea76e8e56e952 --- /dev/null +++ b/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js @@ -0,0 +1,51 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const { finished } = require('stream'); + +const headers = + 'GET / HTTP/1.1\r\n' + + 'Host: localhost\r\n' + + 'Connection: keep-alive\r\n' + + 'Agent: node\r\n'; + +const baseTimeout = 1000; + +const server = http.createServer(common.mustCall((req, res) => { + req.resume(); + res.writeHead(200); + res.end(); +}, 2)); + +server.keepAliveTimeout = 10 * baseTimeout; +server.headersTimeout = baseTimeout; + +server.once('timeout', common.mustNotCall((socket) => { + socket.destroy(); +})); + +server.listen(0, () => { + const client = net.connect(server.address().port); + + // first request + client.write(headers); + client.write('\r\n'); + + setTimeout(() => { + // second request + client.write(headers); + // `headersTimeout` doesn't seem to fire if request + // is sent altogether. + setTimeout(() => { + client.write('\r\n'); + client.end(); + }, 10); + }, baseTimeout + 10); + + client.resume(); + finished(client, common.mustCall((err) => { + server.close(); + })); +}); From f162c42f453acedb8c6b211a51b0214ec91b59c7 Mon Sep 17 00:00:00 2001 From: Alex R Date: Tue, 17 Mar 2020 18:51:37 +0100 Subject: [PATCH 2/4] Clean up --- lib/_http_server.js | 11 +++++------ src/node_http_parser.cc | 11 +++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 041185a3450684..d8900596293399 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -49,7 +49,6 @@ const { OutgoingMessage } = require('_http_outgoing'); const { kOutHeaders, kNeedDrain, - nowDate, emitStatistics } = require('internal/http'); const { @@ -427,7 +426,7 @@ function connectionListenerInternal(server, socket) { server.maxHeaderSize || 0, server.insecureHTTPParser === undefined ? isLenient() : server.insecureHTTPParser, - server.headersTimeout, + server.headersTimeout || 0, ); parser.socket = socket; socket.parser = parser; @@ -481,8 +480,8 @@ function connectionListenerInternal(server, socket) { parser[kOnExecute] = onParserExecute.bind(undefined, server, socket, parser, state); - parser[kOnTimeout] = - onParserTimeout.bind(undefined, server, socket, parser, state); + parser[kOnTimeout] = + onParserTimeout.bind(undefined, server, socket); socket._paused = false; } @@ -576,7 +575,7 @@ function onParserExecute(server, socket, parser, state, ret) { onParserExecuteCommon(server, socket, parser, state, ret, undefined); } -function onParserTimeout(server, socket, parser, state, ret) { +function onParserTimeout(server, socket) { const serverTimeout = server.emit('timeout', socket); if (!serverTimeout) @@ -846,4 +845,4 @@ module.exports = { ServerResponse, _connectionListener: connectionListener, kServerResponse -}; \ No newline at end of file +}; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index bdc5f945fb1d7e..5a3db3f11bff7b 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -519,8 +519,10 @@ class Parser : public AsyncWrap, public StreamListener { max_http_header_size = env->options()->max_http_header_size; } - CHECK(args[4]->IsInt32()); - headers_timeout = args[4].As()->Value(); + if (args.Length() > 4) { + CHECK(args[4]->IsInt32()); + headers_timeout = args[4].As()->Value(); + } llhttp_type_t type = static_cast(args[0].As()->Value()); @@ -803,7 +805,8 @@ class Parser : public AsyncWrap, public StreamListener { } - void Init(llhttp_type_t type, uint64_t max_http_header_size, bool lenient, uint64_t headers_timeout) { + void Init(llhttp_type_t type, uint64_t max_http_header_size, + bool lenient, uint64_t headers_timeout) { llhttp_init(&parser_, type, &settings); llhttp_set_lenient(&parser_, lenient); header_nread_ = 0; @@ -857,7 +860,7 @@ class Parser : public AsyncWrap, public StreamListener { bool pending_pause_ = false; uint64_t header_nread_ = 0; uint64_t max_http_header_size_; - uint64_t headers_timeout_; + uint64_t headers_timeout_; uint64_t header_parsing_start_time_ = 0; // These are helper functions for filling `http_parser_settings`, which turn From 2d91c305b85d7783c654f5a3092d468a6cc35d90 Mon Sep 17 00:00:00 2001 From: Alex R Date: Thu, 19 Mar 2020 15:42:52 +0100 Subject: [PATCH 3/4] Update async_hooks test --- test/async-hooks/test-graph.http.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index db0c1265670a6f..3e36646e54e2ee 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -39,10 +39,12 @@ process.on('exit', () => { id: 'httpclientrequest:1', triggerAsyncId: 'tcpserver:1' }, { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' }, - { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' }, { type: 'HTTPINCOMINGMESSAGE', id: 'httpincomingmessage:1', triggerAsyncId: 'tcp:2' }, + { type: 'Timeout', + id: 'timeout:1', + triggerAsyncId: 'httpincomingmessage:1' }, { type: 'SHUTDOWNWRAP', id: 'shutdown:1', triggerAsyncId: 'tcp:2' } ] From 9536c1b3c7fbf875881a7808f2a4863686c027c5 Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Mon, 30 Mar 2020 11:23:49 +0200 Subject: [PATCH 4/4] Update src/node_http_parser.cc Co-Authored-By: Anna Henningsen --- src/node_http_parser.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 5a3db3f11bff7b..2c75e6a532fff1 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -646,8 +646,8 @@ class Parser : public AsyncWrap, public StreamListener { // check header parsing time if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) { - auto now = uv_hrtime(); - auto parsing_time = (now - header_parsing_start_time_) / 1e6; + uint64_t now = uv_hrtime(); + uint64_t parsing_time = (now - header_parsing_start_time_) / 1e6; if (parsing_time > headers_timeout_) { Local cb =