From f654592647aeb420674af4b981328daac722295b Mon Sep 17 00:00:00 2001 From: Shogun Date: Wed, 30 Mar 2022 08:18:20 +0200 Subject: [PATCH 1/4] http: refactor headersTimeout and requestTimeout logic --- doc/api/http.md | 34 ++- lib/_http_client.js | 3 +- lib/_http_common.js | 12 - lib/_http_server.js | 131 +++++----- lib/https.js | 5 +- src/node_http_parser.cc | 237 +++++++++++++++--- test/async-hooks/test-graph.http.js | 2 +- .../test-http-parser-timeout-reset.js | 3 +- ...-server-headers-timeout-delayed-headers.js | 61 +++++ ...ver-headers-timeout-interrupted-headers.js | 61 +++++ ...t-http-server-headers-timeout-keepalive.js | 103 ++++++++ ...-http-server-headers-timeout-pipelining.js | 76 ++++++ ...ttp-server-request-timeout-delayed-body.js | 17 +- ...-server-request-timeout-delayed-headers.js | 19 +- ...server-request-timeout-interrupted-body.js | 18 +- ...ver-request-timeout-interrupted-headers.js | 18 +- ...t-http-server-request-timeout-keepalive.js | 39 +-- ...-http-server-request-timeout-pipelining.js | 70 ++++++ ...est-http-server-request-timeout-upgrade.js | 12 +- ...test-http-server-request-timeouts-mixed.js | 133 ++++++++++ ...low-headers-keepalive-multiple-requests.js | 51 ---- .../test-http-slow-headers-keepalive.js | 51 ---- test/parallel/test-http-slow-headers.js | 50 ---- .../test-https-server-headers-timeout.js | 21 ++ test/parallel/test-https-slow-headers.js | 63 ----- 25 files changed, 899 insertions(+), 391 deletions(-) create mode 100644 test/parallel/test-http-server-headers-timeout-delayed-headers.js create mode 100644 test/parallel/test-http-server-headers-timeout-interrupted-headers.js create mode 100644 test/parallel/test-http-server-headers-timeout-keepalive.js create mode 100644 test/parallel/test-http-server-headers-timeout-pipelining.js create mode 100644 test/parallel/test-http-server-request-timeout-pipelining.js create mode 100644 test/parallel/test-http-server-request-timeouts-mixed.js delete mode 100644 test/parallel/test-http-slow-headers-keepalive-multiple-requests.js delete mode 100644 test/parallel/test-http-slow-headers-keepalive.js delete mode 100644 test/parallel/test-http-slow-headers.js create mode 100644 test/parallel/test-https-server-headers-timeout.js delete mode 100644 test/parallel/test-https-slow-headers.js diff --git a/doc/api/http.md b/doc/api/http.md index 5c3c7d9a7a7180..d55f53b055f6e2 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -1364,15 +1364,12 @@ added: Limit the amount of time the parser will wait to receive the complete HTTP headers. -In case of inactivity, the rules defined in [`server.timeout`][] apply. However, -that inactivity based timeout would still allow the connection to be kept open -if the headers are being sent very slowly (by default, up to a byte per 2 -minutes). In order to prevent this, whenever header data arrives an additional -check is made that more than `server.headersTimeout` milliseconds has not -passed since the connection was established. If the check fails, a `'timeout'` -event is emitted on the server object, and (by default) the socket is destroyed. -See [`server.timeout`][] for more information on how timeout behavior can be -customized. +If the timeout expires, the server responds with status 408 without +forwarding the request to the request listener and then closes the connection. + +It must be set to a non-zero value (e.g. 120 seconds) to protect against +potential Denial-of-Service attacks in case the server is deployed without a +reverse proxy in front. ### `server.listen()` @@ -2856,6 +2853,10 @@ Found'`. -* {number} **Default:** `0` +* {number} **Default:** `300000` Sets the timeout value in milliseconds for receiving the entire request from the client. @@ -2890,15 +2895,19 @@ changes: * `requestTimeout`: Sets the timeout value in milliseconds for receiving the entire request from the client. See [`server.requestTimeout`][] for more information. + **Default:** `300000`. * `headersTimeout`: Sets the timeout value in milliseconds for receiving the complete HTTP headers from the client. See [`server.headersTimeout`][] for more information. + **Default:** `60000`. * `keepAliveTimeout`: The number of milliseconds of inactivity a server needs to wait for additional incoming data, after it has finished writing the last response, before a socket will be destroyed. See [`server.keepAliveTimeout`][] for more information. + **Default:** `5000`. * `connectionsCheckingInterval`: Sets the interval value in milliseconds to check for request and headers timeout in incomplete requests. + **Default:** `30000`. * `insecureHTTPParser` {boolean} Use an insecure HTTP parser that accepts invalid HTTP headers when `true`. Using the insecure parser should be avoided. See [`--insecure-http-parser`][] for more information. diff --git a/lib/_http_server.js b/lib/_http_server.js index 85beab82e82b18..84be32c78c4075 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -27,6 +27,7 @@ const { ObjectKeys, ObjectSetPrototypeOf, RegExpPrototypeTest, + ReflectApply, Symbol, SymbolFor, } = primordials; @@ -373,7 +374,7 @@ function storeHTTPOptions(options) { validateInteger(requestTimeout, 'requestTimeout', 0); this.requestTimeout = requestTimeout; } else { - this.requestTimeout = 0; + this.requestTimeout = 300_000; // 5 minutes } const headersTimeout = options.headersTimeout; @@ -381,19 +382,19 @@ function storeHTTPOptions(options) { validateInteger(headersTimeout, 'headersTimeout', 0); this.headersTimeout = headersTimeout; } else { - this.headersTimeout = 60 * 1000; // 60 seconds + this.headersTimeout = 60_000; // 60 seconds } - if (this.requestTimeout > 0 && this.headersTimeout > 0 && this.headersTimeout > this.requestTimeout) { + if (this.requestTimeout > 0 && this.headersTimeout > 0 && this.headersTimeout >= this.requestTimeout) { throw new codes.ERR_OUT_OF_RANGE('headersTimeout', '>= requestTimeout', headersTimeout); } const keepAliveTimeout = options.keepAliveTimeout; if (keepAliveTimeout !== undefined) { - validateInteger(keepAliveTimeout, 'keepAliveTimeoutmeout', 0); + validateInteger(keepAliveTimeout, 'keepAliveTimeout', 0); this.keepAliveTimeout = keepAliveTimeout; } else { - this.keepAliveTimeout = 5 * 1000; // 5 seconds; + this.keepAliveTimeout = 5_000; // 5 seconds; } const connectionsCheckingInterval = options.connectionsCheckingInterval; @@ -401,7 +402,7 @@ function storeHTTPOptions(options) { validateInteger(connectionsCheckingInterval, 'connectionsCheckingInterval', 0); this.connectionsCheckingInterval = connectionsCheckingInterval; } else { - this.connectionsCheckingInterval = 10 * 1000; // 10 seconds + this.connectionsCheckingInterval = 30_000; // 30 seconds } } @@ -454,7 +455,7 @@ ObjectSetPrototypeOf(Server, net.Server); Server.prototype.close = function() { clearInterval(this[kConnectionsCheckingInterval]); - net.Server.prototype.close.apply(this, arguments); + ReflectApply(net.Server.prototype.close, this, arguments); }; Server.prototype.setTimeout = function setTimeout(msecs, callback) { diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 0425d84c268798..0257e2cc009de6 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -208,8 +208,12 @@ class ConnectionsList: public BaseObject { // This is needed due to the forward declaration of CompareParsers std::set - newMembers(ConnectionsList::CompareParsers); - list->members = newMembers; + newAllConnections(ConnectionsList::CompareParsers); + std::set + newActiveConnections(ConnectionsList::CompareParsers); + + list->allConnections = newAllConnections; + list->activeConnections = newActiveConnections; } static bool CompareParsers( @@ -225,7 +229,7 @@ class ConnectionsList: public BaseObject { return lhs->last_message_start_ < rhs->last_message_start_; } - static void Pending(const FunctionCallbackInfo& args) { + static void All(const FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); Local context = isolate->GetCurrentContext(); @@ -235,7 +239,43 @@ class ConnectionsList: public BaseObject { ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); uint32_t i = 0; - for (auto parser : list->members) { + for (auto parser : list->allConnections) { + expired->Set(context, i++, parser->AsJSObject()).Check(); + } + + return args.GetReturnValue().Set(expired); + } + + static void Idle(const FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local expired = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->allConnections) { + if (parser->last_message_start_ == 0) { + expired->Set(context, i++, parser->AsJSObject()).Check(); + } + } + + return args.GetReturnValue().Set(expired); + } + + static void Active(const FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local expired = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->activeConnections) { expired->Set(context, i++, parser->AsJSObject()).Check(); } @@ -252,8 +292,10 @@ class ConnectionsList: public BaseObject { ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); CHECK(args[0]->IsNumber()); CHECK(args[1]->IsNumber()); - uint64_t headers_timeout = args[0].As()->Value() * 1000000; - uint64_t request_timeout = args[1].As()->Value() * 1000000; + uint64_t headers_timeout = + static_cast(args[0].As()->Value()) * 1000000; + uint64_t request_timeout = + static_cast(args[1].As()->Value()) * 1000000; if (headers_timeout == 0 && request_timeout == 0) { return args.GetReturnValue().Set(expired); @@ -268,8 +310,8 @@ class ConnectionsList: public BaseObject { request_timeout > 0 ? now - request_timeout : 0; uint32_t i = 0; - auto iter = list->members.begin(); - auto end = list->members.end(); + auto iter = list->activeConnections.begin(); + auto end = list->activeConnections.end(); while (iter != end) { ExpirableParser* parser = *iter; iter++; @@ -283,10 +325,8 @@ class ConnectionsList: public BaseObject { parser->last_message_start_ < request_deadline) ) { expired->Set(context, i++, parser->AsJSObject()).Check(); - list->members.erase(parser); + list->activeConnections.erase(parser); } - - // TODO@PI: How do we bail out earlier? } return args.GetReturnValue().Set(expired); @@ -296,11 +336,19 @@ class ConnectionsList: public BaseObject { : BaseObject(env, object) {} void Push(ExpirableParser* parser) { - members.insert(parser); + allConnections.insert(parser); } void Pop(ExpirableParser* parser) { - members.erase(parser); + allConnections.erase(parser); + } + + void PushActive(ExpirableParser* parser) { + activeConnections.insert(parser); + } + + void PopActive(ExpirableParser* parser) { + activeConnections.erase(parser); } SET_NO_MEMORY_INFO() @@ -310,7 +358,10 @@ class ConnectionsList: public BaseObject { protected: std::set< ExpirableParser*, bool(*)(const ExpirableParser*, const ExpirableParser*) - > members; + > allConnections; + std::set< + ExpirableParser*, bool(*)(const ExpirableParser*, const ExpirableParser*) + > activeConnections; }; class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { @@ -336,7 +387,7 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { otherwise std::set.erase will fail. */ if (connectionsList_ != nullptr) { - connectionsList_->Pop(this); + connectionsList_->PopActive(this); } num_fields_ = num_values_ = 0; @@ -346,7 +397,7 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { status_message_.Reset(); if (connectionsList_ != nullptr) { - connectionsList_->Push(this); + connectionsList_->PushActive(this); } Local cb = object()->Get(env()->context(), kOnMessageBegin) @@ -573,7 +624,7 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { otherwise std::set.erase will fail. */ if (connectionsList_ != nullptr) { - connectionsList_->Pop(this); + connectionsList_->PopActive(this); } last_message_start_ = 0; @@ -635,6 +686,11 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); + if (parser->connectionsList_ != nullptr) { + parser->connectionsList_->Pop(parser); + parser->connectionsList_->PopActive(parser); + } + // Since the Parser destructor isn't going to run the destroy() callbacks // it needs to be triggered manually. parser->EmitTraceEventDestroy(); @@ -743,13 +799,15 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { if (connectionsList != nullptr) { parser->connectionsList_ = connectionsList; + parser->connectionsList_->Push(parser); + /* This protects from DOS attack where an attacker establish the connection without sending any data on applications where server.timeout is left to the default value of zero. */ parser->last_message_start_ = uv_hrtime(); - parser->connectionsList_->Push(parser); + parser->connectionsList_->PushActive(parser); } else { parser->connectionsList_ = nullptr; } @@ -1212,7 +1270,9 @@ void InitializeHttpParser(Local target, Local c = env->NewFunctionTemplate(ConnectionsList::New); c->InstanceTemplate() ->SetInternalFieldCount(ConnectionsList::kInternalFieldCount); - env->SetProtoMethod(c, "pending", ConnectionsList::Pending); + env->SetProtoMethod(c, "all", ConnectionsList::All); + env->SetProtoMethod(c, "idle", ConnectionsList::Idle); + env->SetProtoMethod(c, "active", ConnectionsList::Active); env->SetProtoMethod(c, "expired", ConnectionsList::Expired); env->SetConstructorFunction(target, "ConnectionsList", c); } diff --git a/test/parallel/test-http-parser-timeout-reset.js b/test/parallel/test-http-parser-timeout-reset.js index 23d911f077f072..c51daffaafb056 100644 --- a/test/parallel/test-http-parser-timeout-reset.js +++ b/test/parallel/test-http-parser-timeout-reset.js @@ -26,7 +26,7 @@ const server = net.createServer((socket) => { HTTPParser.RESPONSE, {}, 0, - 0 + 0, ); parser[HTTPParser.kOnTimeout] = common.mustNotCall(); diff --git a/test/parallel/test-http-server-headers-timeout-delayed-headers.js b/test/parallel/test-http-server-headers-timeout-delayed-headers.js index eea73ae647601c..5c18492384911e 100644 --- a/test/parallel/test-http-server-headers-timeout-delayed-headers.js +++ b/test/parallel/test-http-server-headers-timeout-delayed-headers.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout, requestTimeout: 0, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); diff --git a/test/parallel/test-http-server-headers-timeout-interrupted-headers.js b/test/parallel/test-http-server-headers-timeout-interrupted-headers.js index 645c30c7392b86..ea47ae8e19e12e 100644 --- a/test/parallel/test-http-server-headers-timeout-interrupted-headers.js +++ b/test/parallel/test-http-server-headers-timeout-interrupted-headers.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout, requestTimeout: 0, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); diff --git a/test/parallel/test-http-server-headers-timeout-keepalive.js b/test/parallel/test-http-server-headers-timeout-keepalive.js index d94838fdb1bb0b..c7e5e56da90050 100644 --- a/test/parallel/test-http-server-headers-timeout-keepalive.js +++ b/test/parallel/test-http-server-headers-timeout-keepalive.js @@ -32,7 +32,7 @@ const server = createServer({ headersTimeout, requestTimeout: 0, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCallAtLeast((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end(); diff --git a/test/parallel/test-http-server-headers-timeout-pipelining.js b/test/parallel/test-http-server-headers-timeout-pipelining.js index 833a63cdc8ccfa..13f89c60c05cf5 100644 --- a/test/parallel/test-http-server-headers-timeout-pipelining.js +++ b/test/parallel/test-http-server-headers-timeout-pipelining.js @@ -14,7 +14,7 @@ const server = createServer({ headersTimeout, requestTimeout: 0, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCallAtLeast((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end(); diff --git a/test/parallel/test-http-server-request-timeout-delayed-body.js b/test/parallel/test-http-server-request-timeout-delayed-body.js index 4740d267a28e74..c5ecb720e47eaf 100644 --- a/test/parallel/test-http-server-request-timeout-delayed-body.js +++ b/test/parallel/test-http-server-request-timeout-delayed-body.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCall((req, res) => { let body = ''; req.setEncoding('utf-8'); diff --git a/test/parallel/test-http-server-request-timeout-delayed-headers.js b/test/parallel/test-http-server-request-timeout-delayed-headers.js index aabb778948b145..7174afec47fcc0 100644 --- a/test/parallel/test-http-server-request-timeout-delayed-headers.js +++ b/test/parallel/test-http-server-request-timeout-delayed-headers.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); diff --git a/test/parallel/test-http-server-request-timeout-interrupted-body.js b/test/parallel/test-http-server-request-timeout-interrupted-body.js index 4f790e8b79df79..ee087719e4ee05 100644 --- a/test/parallel/test-http-server-request-timeout-interrupted-body.js +++ b/test/parallel/test-http-server-request-timeout-interrupted-body.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCall((req, res) => { let body = ''; req.setEncoding('utf-8'); diff --git a/test/parallel/test-http-server-request-timeout-interrupted-headers.js b/test/parallel/test-http-server-request-timeout-interrupted-headers.js index 3d88c85e31c0c1..64511c6b50ce3a 100644 --- a/test/parallel/test-http-server-request-timeout-interrupted-headers.js +++ b/test/parallel/test-http-server-request-timeout-interrupted-headers.js @@ -15,7 +15,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); diff --git a/test/parallel/test-http-server-request-timeout-keepalive.js b/test/parallel/test-http-server-request-timeout-keepalive.js index ddb7b2b4f87d03..0b8f798c3eb4a1 100644 --- a/test/parallel/test-http-server-request-timeout-keepalive.js +++ b/test/parallel/test-http-server-request-timeout-keepalive.js @@ -32,7 +32,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCallAtLeast((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end(); diff --git a/test/parallel/test-http-server-request-timeout-pipelining.js b/test/parallel/test-http-server-request-timeout-pipelining.js index 03b6fe6b35bdf0..4e6977b3270dab 100644 --- a/test/parallel/test-http-server-request-timeout-pipelining.js +++ b/test/parallel/test-http-server-request-timeout-pipelining.js @@ -14,7 +14,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustCallAtLeast((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }); res.end(); diff --git a/test/parallel/test-http-server-request-timeout-upgrade.js b/test/parallel/test-http-server-request-timeout-upgrade.js index fa665f91745093..b1974a128b9df3 100644 --- a/test/parallel/test-http-server-request-timeout-upgrade.js +++ b/test/parallel/test-http-server-request-timeout-upgrade.js @@ -13,7 +13,7 @@ const server = createServer({ headersTimeout: 0, requestTimeout, keepAliveTimeout: 0, - connectionsCheckingInterval: common.platformTimeout(250) + connectionsCheckingInterval: common.platformTimeout(250), }, common.mustNotCall()); server.on('connection', common.mustCall(() => { assert.strictEqual(typeof sendDelayedRequestHeaders, 'function'); diff --git a/test/parallel/test-http-server-request-timeouts-mixed.js b/test/parallel/test-http-server-request-timeouts-mixed.js deleted file mode 100644 index 5f837a0e56c3b0..00000000000000 --- a/test/parallel/test-http-server-request-timeouts-mixed.js +++ /dev/null @@ -1,133 +0,0 @@ -'use strict'; - -const common = require('../common'); -const assert = require('assert'); -const { createServer } = require('http'); -const { connect } = require('net'); - -// This test validates that request are correct checked for both requests and headers timeout in various situations. - -const requestBodyPart1 = 'POST / HTTP/1.1\r\nContent-Length: 20\r\n'; -const requestBodyPart2 = 'Connection: close\r\n\r\n1234567890'; -const requestBodyPart3 = '1234567890'; - -const responseOk = 'HTTP/1.1 200 OK\r\n'; -const responseTimeout = 'HTTP/1.1 408 Request Timeout\r\n'; - -const headersTimeout = common.platformTimeout(1000); -const connectionsCheckingInterval = common.platformTimeout(250); - -const server = createServer({ - headersTimeout, - requestTimeout: headersTimeout * 2, - keepAliveTimeout: 0, - connectionsCheckingInterval -}, common.mustCall((req, res) => { - req.on('data', () => { - // No-op - }); - - req.on('end', () => { - res.writeHead(200, { 'Content-Type': 'text/plain' }); - res.end(); - }); -}, 4)); - -assert.strictEqual(server.headersTimeout, headersTimeout); -assert.strictEqual(server.requestTimeout, headersTimeout * 2); - -let i = 0; -function createClient(server) { - const request = { - index: i++, - client: connect(server.address().port), - response: '', - completed: false - }; - - request.client.on('data', common.mustCallAtLeast((chunk) => { - request.response += chunk.toString('utf-8'); - })); - - request.client.on('end', common.mustCall(() => { - request.completed = true; - })); - - request.client.on('error', common.mustNotCall()); - - request.client.resume(); - - return request; -} - -server.listen(0, common.mustCall(() => { - const request1 = createClient(server); - let request2; - let request3; - let request4; - let request5; - - // Send the first request and stop before the body - request1.client.write(requestBodyPart1); - - // After a little while send two new requests - setTimeout(() => { - request2 = createClient(server); - request3 = createClient(server); - - // Send the second request, stop in the middle of the headers - request2.client.write(requestBodyPart1); - // Send the second request, stop in the middle of the headers - request3.client.write(requestBodyPart1); - }, headersTimeout * 0.2); - - // After another little while send the last two new requests - setTimeout(() => { - request4 = createClient(server); - request5 = createClient(server); - - // Send the fourth request, stop in the middle of the headers - request4.client.write(requestBodyPart1); - // Send the fifth request, stop in the middle of the headers - request5.client.write(requestBodyPart1); - }, headersTimeout * 0.6); - - setTimeout(() => { - // Finish the first request - request1.client.write(requestBodyPart2 + requestBodyPart3); - - // Complete headers for all requests but second - request3.client.write(requestBodyPart2); - request4.client.write(requestBodyPart2); - request5.client.write(requestBodyPart2); - }, headersTimeout * 0.8); - - setTimeout(() => { - // After the first timeout, the first request should have been completed and second timedout - assert(request1.completed); - assert(request2.completed); - assert(!request3.completed); - assert(!request4.completed); - assert(!request5.completed); - - assert(request1.response.startsWith(responseOk)); - assert(request2.response.startsWith(responseTimeout)); // It is expired due to headersTimeout - }, headersTimeout * 1.2 + connectionsCheckingInterval); - - setTimeout(() => { - // Complete the body for the fourth request - request4.client.write(requestBodyPart3); - }, headersTimeout * 1.5); - - setTimeout(() => { - // All request should be completed now, either with 200 or 408 - assert(request3.completed); - assert(request4.completed); - assert(request5.completed); - - assert(request3.response.startsWith(responseTimeout)); // It is expired due to requestTimeout - assert(request4.response.startsWith(responseOk)); - assert(request5.response.startsWith(responseTimeout)); // It is expired due to requestTimeout - server.close(); - }, headersTimeout * 0.6 + headersTimeout * 2 + connectionsCheckingInterval); -})); diff --git a/test/parallel/test-https-server-headers-timeout.js b/test/parallel/test-https-server-headers-timeout.js index b621d287356add..45457e39425127 100644 --- a/test/parallel/test-https-server-headers-timeout.js +++ b/test/parallel/test-https-server-headers-timeout.js @@ -9,7 +9,7 @@ const fixtures = require('../common/fixtures'); const options = { key: fixtures.readKey('agent1-key.pem'), - cert: fixtures.readKey('agent1-cert.pem') + cert: fixtures.readKey('agent1-cert.pem'), }; const server = createServer(options); diff --git a/test/parallel/test-https-server-request-timeout.js b/test/parallel/test-https-server-request-timeout.js index 66a1cb9f25f82e..00bac8ea3991fa 100644 --- a/test/parallel/test-https-server-request-timeout.js +++ b/test/parallel/test-https-server-request-timeout.js @@ -14,8 +14,8 @@ const options = { const server = createServer(options); -// 0 seconds is the default -assert.strictEqual(server.requestTimeout, 0); +// 300 seconds is the default +assert.strictEqual(server.requestTimeout, 300000); const requestTimeout = common.platformTimeout(1000); server.requestTimeout = requestTimeout; assert.strictEqual(server.requestTimeout, requestTimeout); From 35dae531d87e9ab223c158376626f09f6fce242c Mon Sep 17 00:00:00 2001 From: Shogun Date: Wed, 13 Apr 2022 08:16:09 +0200 Subject: [PATCH 3/4] http: make ConnectionList weak --- src/node_http_parser.cc | 338 ++++++++++++++++++++-------------------- 1 file changed, 167 insertions(+), 171 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 0257e2cc009de6..55aa7075d700cc 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -60,6 +60,7 @@ using v8::FunctionTemplate; using v8::HandleScope; using v8::Int32; using v8::Integer; +using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Number; @@ -187,184 +188,58 @@ struct StringPtr { size_t size_; }; -class ConnectionsList; +class Parser; -class ExpirableParser { - friend class ConnectionsList; - - protected: - virtual Local AsJSObject() = 0; - uint64_t last_message_start_; - bool headers_completed_; +struct ParserComparer { + bool operator()(const Parser* lhs, const Parser* rhs) const; }; -class ConnectionsList: public BaseObject { +class ConnectionsList : public BaseObject { public: - static void New(const FunctionCallbackInfo& args) { - Local context = args.GetIsolate()->GetCurrentContext(); - Environment* env = Environment::GetCurrent(context); - - ConnectionsList* list = new ConnectionsList(env, args.This()); - - // This is needed due to the forward declaration of CompareParsers - std::set - newAllConnections(ConnectionsList::CompareParsers); - std::set - newActiveConnections(ConnectionsList::CompareParsers); - - list->allConnections = newAllConnections; - list->activeConnections = newActiveConnections; - } - - static bool CompareParsers( - const ExpirableParser* lhs, - const ExpirableParser* rhs - ) { - if (lhs->last_message_start_ == 0) { - return false; - } else if (rhs->last_message_start_ == 0) { - return true; - } - - return lhs->last_message_start_ < rhs->last_message_start_; - } - - static void All(const FunctionCallbackInfo& args) { - v8::Isolate* isolate = args.GetIsolate(); - Local context = isolate->GetCurrentContext(); - - Local expired = Array::New(isolate); - ConnectionsList* list; - - ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); - - uint32_t i = 0; - for (auto parser : list->allConnections) { - expired->Set(context, i++, parser->AsJSObject()).Check(); - } - - return args.GetReturnValue().Set(expired); - } - - static void Idle(const FunctionCallbackInfo& args) { - v8::Isolate* isolate = args.GetIsolate(); - Local context = isolate->GetCurrentContext(); - - Local expired = Array::New(isolate); - ConnectionsList* list; - - ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); - - uint32_t i = 0; - for (auto parser : list->allConnections) { - if (parser->last_message_start_ == 0) { - expired->Set(context, i++, parser->AsJSObject()).Check(); - } - } - - return args.GetReturnValue().Set(expired); - } - - static void Active(const FunctionCallbackInfo& args) { - v8::Isolate* isolate = args.GetIsolate(); - Local context = isolate->GetCurrentContext(); - - Local expired = Array::New(isolate); - ConnectionsList* list; - - ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + static void New(const FunctionCallbackInfo& args); - uint32_t i = 0; - for (auto parser : list->activeConnections) { - expired->Set(context, i++, parser->AsJSObject()).Check(); - } - - return args.GetReturnValue().Set(expired); - } + static void All(const FunctionCallbackInfo& args); - static void Expired(const FunctionCallbackInfo& args) { - v8::Isolate* isolate = args.GetIsolate(); - Local context = isolate->GetCurrentContext(); - - Local expired = Array::New(isolate); - ConnectionsList* list; - - ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); - CHECK(args[0]->IsNumber()); - CHECK(args[1]->IsNumber()); - uint64_t headers_timeout = - static_cast(args[0].As()->Value()) * 1000000; - uint64_t request_timeout = - static_cast(args[1].As()->Value()) * 1000000; - - if (headers_timeout == 0 && request_timeout == 0) { - return args.GetReturnValue().Set(expired); - } else if (request_timeout > 0 && headers_timeout > request_timeout) { - std::swap(headers_timeout, request_timeout); - } + static void Idle(const FunctionCallbackInfo& args); - const uint64_t now = uv_hrtime(); - const uint64_t headers_deadline = - headers_timeout > 0 ? now - headers_timeout : 0; - const uint64_t request_deadline = - request_timeout > 0 ? now - request_timeout : 0; - - uint32_t i = 0; - auto iter = list->activeConnections.begin(); - auto end = list->activeConnections.end(); - while (iter != end) { - ExpirableParser* parser = *iter; - iter++; - - // Check for expiration - if ( - (!parser->headers_completed_ && headers_deadline > 0 && - parser->last_message_start_ < headers_deadline) || - ( - request_deadline > 0 && - parser->last_message_start_ < request_deadline) - ) { - expired->Set(context, i++, parser->AsJSObject()).Check(); - list->activeConnections.erase(parser); - } - } + static void Active(const FunctionCallbackInfo& args); - return args.GetReturnValue().Set(expired); - } + static void Expired(const FunctionCallbackInfo& args); - ConnectionsList(Environment* env, Local object) - : BaseObject(env, object) {} - - void Push(ExpirableParser* parser) { + void Push(Parser* parser) { allConnections.insert(parser); } - void Pop(ExpirableParser* parser) { + void Pop(Parser* parser) { allConnections.erase(parser); } - void PushActive(ExpirableParser* parser) { + void PushActive(Parser* parser) { activeConnections.insert(parser); } - void PopActive(ExpirableParser* parser) { + void PopActive(Parser* parser) { activeConnections.erase(parser); } SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(ConnectionsList) - SET_SELF_SIZE(std::unordered_set) + SET_SELF_SIZE(ConnectionsList) - protected: - std::set< - ExpirableParser*, bool(*)(const ExpirableParser*, const ExpirableParser*) - > allConnections; - std::set< - ExpirableParser*, bool(*)(const ExpirableParser*, const ExpirableParser*) - > activeConnections; + private: + std::set allConnections; + std::set activeConnections; + + ConnectionsList(Environment* env, Local object) + : BaseObject(env, object) { + MakeWeak(); + } }; -class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { +class Parser : public AsyncWrap, public StreamListener { + friend class ConnectionsList; + friend struct ParserComparer; + public: Parser(BindingData* binding_data, Local wrap) : AsyncWrap(binding_data->env(), wrap), @@ -382,10 +257,8 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { SET_SELF_SIZE(Parser) int on_message_begin() { - /* - Important: Pop from the list BEFORE resetting the last_message_start_ - otherwise std::set.erase will fail. - */ + // Important: Pop from the list BEFORE resetting the last_message_start_ + // otherwise std::set.erase will fail. if (connectionsList_ != nullptr) { connectionsList_->PopActive(this); } @@ -619,10 +492,8 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { int on_message_complete() { HandleScope scope(env()->isolate()); - /* - Important: Pop from the list BEFORE resetting the last_message_start_ - otherwise std::set.erase will fail. - */ + // Important: Pop from the list BEFORE resetting the last_message_start_ + // otherwise std::set.erase will fail. if (connectionsList_ != nullptr) { connectionsList_->PopActive(this); } @@ -711,10 +582,6 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { } } - Local AsJSObject() override { - return object(); - } - // var bytesParsed = parser->execute(buffer); static void Execute(const FunctionCallbackInfo& args) { Parser* parser; @@ -801,11 +668,9 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { parser->connectionsList_->Push(parser); - /* - This protects from DOS attack where an attacker establish - the connection without sending any data on applications where - server.timeout is left to the default value of zero. - */ + // This protects from a DoS attack where an attacker establishes + // the connection without sending any data on applications where + // server.timeout is left to the default value of zero. parser->last_message_start_ = uv_hrtime(); parser->connectionsList_->PushActive(parser); } else { @@ -877,8 +742,8 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { return; } - uint64_t duration = (uv_hrtime() - parser->last_message_start_) / 1000000; - args.GetReturnValue().Set(static_cast(duration)); + double duration = (uv_hrtime() - parser->last_message_start_) / 1000000; + args.GetReturnValue().Set(duration); } static void HeadersCompleted(const FunctionCallbackInfo& args) { @@ -1148,9 +1013,11 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { size_t current_buffer_len_; const char* current_buffer_data_; unsigned int execute_depth_ = 0; + bool headers_completed_ = false; bool pending_pause_ = false; uint64_t header_nread_ = 0; uint64_t max_http_header_size_; + uint64_t last_message_start_; ConnectionsList* connectionsList_; BaseObjectPtr binding_data_; @@ -1176,6 +1043,135 @@ class Parser : public ExpirableParser, public AsyncWrap, public StreamListener { static const llhttp_settings_t settings; }; +bool ParserComparer::operator()(const Parser* lhs, const Parser* rhs) const { + if (lhs->last_message_start_ == 0) { + return false; + } else if (rhs->last_message_start_ == 0) { + return true; + } + + return lhs->last_message_start_ < rhs->last_message_start_; +} + +void ConnectionsList::New(const FunctionCallbackInfo& args) { + Local context = args.GetIsolate()->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + + new ConnectionsList(env, args.This()); +} + +void ConnectionsList::All(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local all = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->allConnections) { + if (all->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + + return args.GetReturnValue().Set(all); +} + +void ConnectionsList::Idle(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local idle = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->allConnections) { + if (parser->last_message_start_ == 0) { + if (idle->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + } + + return args.GetReturnValue().Set(idle); +} + +void ConnectionsList::Active(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local active = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + + uint32_t i = 0; + for (auto parser : list->activeConnections) { + if (active->Set(context, i++, parser->object()).IsNothing()) { + return; + } + } + + return args.GetReturnValue().Set(active); +} + +void ConnectionsList::Expired(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local expired = Array::New(isolate); + ConnectionsList* list; + + ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); + CHECK(args[0]->IsNumber()); + CHECK(args[1]->IsNumber()); + uint64_t headers_timeout = + static_cast(args[0].As()->Value()) * 1000000; + uint64_t request_timeout = + static_cast(args[1].As()->Value()) * 1000000; + + if (headers_timeout == 0 && request_timeout == 0) { + return args.GetReturnValue().Set(expired); + } else if (request_timeout > 0 && headers_timeout > request_timeout) { + std::swap(headers_timeout, request_timeout); + } + + const uint64_t now = uv_hrtime(); + const uint64_t headers_deadline = + headers_timeout > 0 ? now - headers_timeout : 0; + const uint64_t request_deadline = + request_timeout > 0 ? now - request_timeout : 0; + + uint32_t i = 0; + auto iter = list->activeConnections.begin(); + auto end = list->activeConnections.end(); + while (iter != end) { + Parser* parser = *iter; + iter++; + + // Check for expiration. + if ( + (!parser->headers_completed_ && headers_deadline > 0 && + parser->last_message_start_ < headers_deadline) || + ( + request_deadline > 0 && + parser->last_message_start_ < request_deadline) + ) { + if (expired->Set(context, i++, parser->object()).IsNothing()) { + return; + } + + list->activeConnections.erase(parser); + } + } + + return args.GetReturnValue().Set(expired); +} + const llhttp_settings_t Parser::settings = { Proxy::Raw, Proxy::Raw, From 55e3c7915602a6d4939f98202501d053ef691f21 Mon Sep 17 00:00:00 2001 From: Shogun Date: Wed, 13 Apr 2022 11:14:38 +0200 Subject: [PATCH 4/4] http: minor improvements --- src/node_http_parser.cc | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 55aa7075d700cc..c1255b8cbd3b13 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -190,7 +190,7 @@ struct StringPtr { class Parser; -struct ParserComparer { +struct ParserComparator { bool operator()(const Parser* lhs, const Parser* rhs) const; }; @@ -207,19 +207,19 @@ class ConnectionsList : public BaseObject { static void Expired(const FunctionCallbackInfo& args); void Push(Parser* parser) { - allConnections.insert(parser); + all_connections_.insert(parser); } void Pop(Parser* parser) { - allConnections.erase(parser); + all_connections_.erase(parser); } void PushActive(Parser* parser) { - activeConnections.insert(parser); + active_connections_.insert(parser); } void PopActive(Parser* parser) { - activeConnections.erase(parser); + active_connections_.erase(parser); } SET_NO_MEMORY_INFO() @@ -227,18 +227,18 @@ class ConnectionsList : public BaseObject { SET_SELF_SIZE(ConnectionsList) private: - std::set allConnections; - std::set activeConnections; - ConnectionsList(Environment* env, Local object) : BaseObject(env, object) { MakeWeak(); } + + std::set all_connections_; + std::set active_connections_; }; class Parser : public AsyncWrap, public StreamListener { friend class ConnectionsList; - friend struct ParserComparer; + friend struct ParserComparator; public: Parser(BindingData* binding_data, Local wrap) @@ -742,7 +742,7 @@ class Parser : public AsyncWrap, public StreamListener { return; } - double duration = (uv_hrtime() - parser->last_message_start_) / 1000000; + double duration = (uv_hrtime() - parser->last_message_start_) / 1e6; args.GetReturnValue().Set(duration); } @@ -1043,7 +1043,7 @@ class Parser : public AsyncWrap, public StreamListener { static const llhttp_settings_t settings; }; -bool ParserComparer::operator()(const Parser* lhs, const Parser* rhs) const { +bool ParserComparator::operator()(const Parser* lhs, const Parser* rhs) const { if (lhs->last_message_start_ == 0) { return false; } else if (rhs->last_message_start_ == 0) { @@ -1070,7 +1070,7 @@ void ConnectionsList::All(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); uint32_t i = 0; - for (auto parser : list->allConnections) { + for (auto parser : list->all_connections_) { if (all->Set(context, i++, parser->object()).IsNothing()) { return; } @@ -1089,7 +1089,7 @@ void ConnectionsList::Idle(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); uint32_t i = 0; - for (auto parser : list->allConnections) { + for (auto parser : list->all_connections_) { if (parser->last_message_start_ == 0) { if (idle->Set(context, i++, parser->object()).IsNothing()) { return; @@ -1110,7 +1110,7 @@ void ConnectionsList::Active(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder()); uint32_t i = 0; - for (auto parser : list->activeConnections) { + for (auto parser : list->active_connections_) { if (active->Set(context, i++, parser->object()).IsNothing()) { return; } @@ -1130,9 +1130,9 @@ void ConnectionsList::Expired(const FunctionCallbackInfo& args) { CHECK(args[0]->IsNumber()); CHECK(args[1]->IsNumber()); uint64_t headers_timeout = - static_cast(args[0].As()->Value()) * 1000000; + static_cast(args[0].As()->Value()) * 1000000; uint64_t request_timeout = - static_cast(args[1].As()->Value()) * 1000000; + static_cast(args[1].As()->Value()) * 1000000; if (headers_timeout == 0 && request_timeout == 0) { return args.GetReturnValue().Set(expired); @@ -1147,8 +1147,8 @@ void ConnectionsList::Expired(const FunctionCallbackInfo& args) { request_timeout > 0 ? now - request_timeout : 0; uint32_t i = 0; - auto iter = list->activeConnections.begin(); - auto end = list->activeConnections.end(); + auto iter = list->active_connections_.begin(); + auto end = list->active_connections_.end(); while (iter != end) { Parser* parser = *iter; iter++; @@ -1165,7 +1165,7 @@ void ConnectionsList::Expired(const FunctionCallbackInfo& args) { return; } - list->activeConnections.erase(parser); + list->active_connections_.erase(parser); } }