From 8597df48f79408b0a72049197982c67e424552c0 Mon Sep 17 00:00:00 2001 From: Alex R Date: Wed, 30 Oct 2019 23:33:33 +0100 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/32329 Fixes: https://github.com/nodejs/node/issues/27363 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- lib/_http_client.js | 3 +- lib/_http_common.js | 2 + lib/_http_server.js | 45 ++++------------ src/node_http_parser.cc | 37 +++++++++++++- test/async-hooks/test-graph.http.js | 4 +- ...low-headers-keepalive-multiple-requests.js | 51 +++++++++++++++++++ 6 files changed, 104 insertions(+), 38 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 b414cd41a02041..2b83b8aa78ace0 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -681,7 +681,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 7b0cd8cd8ef597..c8b27dcd42cb5b 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 { @@ -142,6 +141,7 @@ const STATUS_CODES = { }; const kOnExecute = HTTPParser.kOnExecute | 0; +const kOnTimeout = HTTPParser.kOnTimeout | 0; class HTTPServerAsyncResource { constructor(type, socket) { @@ -426,11 +426,9 @@ function connectionListenerInternal(server, socket) { server.maxHeaderSize || 0, server.insecureHTTPParser === undefined ? isLenient() : server.insecureHTTPParser, + server.headersTimeout || 0, ); 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 +480,9 @@ function connectionListenerInternal(server, socket) { parser[kOnExecute] = onParserExecute.bind(undefined, server, socket, parser, state); + parser[kOnTimeout] = + onParserTimeout.bind(undefined, server, socket); + socket._paused = false; } @@ -570,21 +571,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); - - if (!serverTimeout) - socket.destroy(); - return; - } +function onParserTimeout(server, socket) { + const serverTimeout = server.emit('timeout', socket); - onParserExecuteCommon(server, socket, parser, state, ret, undefined); + if (!serverTimeout) + socket.destroy(); } const noop = () => {}; @@ -722,13 +717,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; @@ -853,17 +841,6 @@ 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, diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 75d7e89a91c34f..2c75e6a532fff1 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,11 @@ class Parser : public AsyncWrap, public StreamListener { max_http_header_size = env->options()->max_http_header_size; } + if (args.Length() > 4) { + CHECK(args[4]->IsInt32()); + headers_timeout = args[4].As()->Value(); + } + llhttp_type_t type = static_cast(args[0].As()->Value()); @@ -532,7 +540,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 +644,24 @@ class Parser : public AsyncWrap, public StreamListener { if (ret.IsEmpty()) return; + // check header parsing time + if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) { + uint64_t now = uv_hrtime(); + uint64_t 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 +805,8 @@ 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 +817,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 +860,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 +921,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/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' } ] 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(); + })); +});