From f7281ca7bd268fa7cadbeb4cf3285652c10633e8 Mon Sep 17 00:00:00 2001 From: Alex R Date: Wed, 30 Oct 2019 23:33:33 +0100 Subject: [PATCH 1/5] 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_common.js | 2 + lib/_http_server.js | 28 ++++------- src/node_http_parser.cc | 20 ++++++++ ...low-headers-keepalive-multiple-requests.js | 50 +++++++++++++++++++ 4 files changed, 81 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-http-slow-headers-keepalive-multiple-requests.js diff --git a/lib/_http_common.js b/lib/_http_common.js index f1386e1a0915dd..d898edcafd5b8d 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 kOnMessageBegin = HTTPParser.kOnMessageBegin | 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[kOnMessageBegin] = null; return parser; }); diff --git a/lib/_http_server.js b/lib/_http_server.js index 3f9c98e8380363..d327e6c8b5434e 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -142,6 +142,7 @@ const STATUS_CODES = { }; const kOnExecute = HTTPParser.kOnExecute | 0; +const kOnMessageBegin = HTTPParser.kOnMessageBegin | 0; class HTTPServerAsyncResource { constructor(type, socket) { @@ -428,9 +429,6 @@ function connectionListenerInternal(server, socket) { isLenient() : server.insecureHTTPParser, ); 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 @@ -481,6 +479,7 @@ function connectionListenerInternal(server, socket) { } parser[kOnExecute] = onParserExecute.bind(undefined, server, socket, parser, state); + parser[kOnMessageBegin] = onParserMessageBegin.bind(undefined, parser); socket._paused = false; } @@ -568,11 +567,17 @@ function socketOnData(server, socket, parser, state, d) { onParserExecuteCommon(server, socket, parser, state, ret, d); } +function onParserMessageBegin(parser) { + // We are starting to wait for the headers. + parser.parsingHeadersStart = nowDate(); +} + function onParserExecute(server, socket, parser, state, ret) { socket._unrefTimer(); - const start = parser.parsingHeadersStart; + debug('SERVER socketOnParserExecute %d', ret); + const start = parser.parsingHeadersStart; // 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. @@ -720,10 +725,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; @@ -851,17 +852,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..b4567e1a9e852d 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 kOnMessageBegin = 5; // Any more fields than this will be flushed into JS const size_t kMaxHeaderFieldsCount = 32; @@ -181,6 +182,23 @@ class Parser : public AsyncWrap, public StreamListener { num_fields_ = num_values_ = 0; url_.Reset(); status_message_.Reset(); + + Local obj = object(); + Local cb = obj->Get(env()->context(), + kOnMessageBegin).ToLocalChecked(); + + if (!cb->IsFunction()) + return 0; + + Local argv[0]; + MaybeLocal r = MakeCallback(cb.As(), 0, argv); + + if (r.IsEmpty()) { + got_exception_ = true; + llhttp_set_error_reason(&parser_, "HPE_JS_EXCEPTION:JS Exception"); + return HPE_USER; + } + return 0; } @@ -890,6 +908,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(), "kOnMessageBegin"), + Integer::NewFromUnsigned(env->isolate(), kOnMessageBegin)); 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..159b4f34af1b98 --- /dev/null +++ b/test/parallel/test-http-slow-headers-keepalive-multiple-requests.js @@ -0,0 +1,50 @@ +'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 headers + // are sent in one packet. + setTimeout(() => { + client.write('\r\n'); + client.end(); + }, 10); + }, baseTimeout + 10); + + finished(client, common.mustCall((err) => { + server.close(); + })); +}); From 8cfd1b8d39c422b91f6ba1c95a200c955a220ed8 Mon Sep 17 00:00:00 2001 From: Alex R Date: Thu, 31 Oct 2019 08:30:28 +0100 Subject: [PATCH 2/5] test: fix flaky test-http-highwatermark --- test/parallel/test-http-highwatermark.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-highwatermark.js b/test/parallel/test-http-highwatermark.js index 79d9c46a558b60..b2cfa8ee2636c5 100644 --- a/test/parallel/test-http-highwatermark.js +++ b/test/parallel/test-http-highwatermark.js @@ -14,7 +14,7 @@ let requestReceived = 0; const server = http.createServer(function(req, res) { const id = ++requestReceived; const enoughToDrain = req.connection.writableHighWaterMark; - const body = 'x'.repeat(enoughToDrain * 100); + const body = 'x'.repeat(enoughToDrain * 200); if (id === 1) { // Case of needParse = false From aa8399326e5c8ffbffa90b7887d7c8dd00b7eed8 Mon Sep 17 00:00:00 2001 From: Alex R Date: Fri, 1 Nov 2019 11:19:22 +0100 Subject: [PATCH 3/5] http: add nowDate() to async scope --- lib/_http_server.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/_http_server.js b/lib/_http_server.js index d327e6c8b5434e..b477c5ae93901a 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -429,6 +429,7 @@ function connectionListenerInternal(server, socket) { isLenient() : server.insecureHTTPParser, ); parser.socket = socket; + parser.parsingHeadersStart = nowDate(); socket.parser = parser; // Propagate headers limit from server instance to parser From 845d4bb186084b4bf7997dfeb1f01555c56f3d41 Mon Sep 17 00:00:00 2001 From: Alex R Date: Sun, 5 Jan 2020 16:49:17 +0100 Subject: [PATCH 4/5] http: use nullptr to fix win build --- src/node_http_parser.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index b4567e1a9e852d..8d906e10022003 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -190,8 +190,7 @@ class Parser : public AsyncWrap, public StreamListener { if (!cb->IsFunction()) return 0; - Local argv[0]; - MaybeLocal r = MakeCallback(cb.As(), 0, argv); + MaybeLocal r = MakeCallback(cb.As(), 0, nullptr); if (r.IsEmpty()) { got_exception_ = true; From 01c2228ccf49d3d79cd8652efe9847a328bb1943 Mon Sep 17 00:00:00 2001 From: Alex R Date: Sun, 12 Jan 2020 19:22:09 +0100 Subject: [PATCH 5/5] http: revert flaky test fix --- test/parallel/test-http-highwatermark.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-highwatermark.js b/test/parallel/test-http-highwatermark.js index b2cfa8ee2636c5..79d9c46a558b60 100644 --- a/test/parallel/test-http-highwatermark.js +++ b/test/parallel/test-http-highwatermark.js @@ -14,7 +14,7 @@ let requestReceived = 0; const server = http.createServer(function(req, res) { const id = ++requestReceived; const enoughToDrain = req.connection.writableHighWaterMark; - const body = 'x'.repeat(enoughToDrain * 200); + const body = 'x'.repeat(enoughToDrain * 100); if (id === 1) { // Case of needParse = false