From 3caa2c1a005652fdb3e896ef940cd5ffe5fdff10 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Wed, 13 Apr 2022 16:47:59 +0200 Subject: [PATCH] http: refactor headersTimeout and requestTimeout logic PR-URL: https://github.com/nodejs/node/pull/41263 Fixes: https://github.com/nodejs/node/issues/33440 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: James M Snell Reviewed-By: Darshan Sen --- benchmark/http/chunked.js | 5 +- benchmark/http/client-request-body.js | 10 +- benchmark/http/end-vs-write-end.js | 5 +- benchmark/http/headers.js | 5 +- benchmark/http/incoming_headers.js | 5 +- benchmark/http/set-header.js | 7 +- benchmark/http/simple.js | 5 +- benchmark/http/upgrade.js | 2 +- doc/api/http.md | 45 ++- lib/_http_client.js | 3 +- lib/_http_common.js | 12 - lib/_http_server.js | 132 ++++---- lib/https.js | 5 +- src/node_http_parser.cc | 293 ++++++++++++++++-- test/async-hooks/test-graph.http.js | 2 +- .../test-http-parser-timeout-reset.js | 1 - ...-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 +- ...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-https-server-request-timeout.js | 4 +- test/parallel/test-https-slow-headers.js | 63 ---- 33 files changed, 858 insertions(+), 413 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 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/benchmark/http/chunked.js b/benchmark/http/chunked.js index 9ae7bb7495f29a..ec86d0a1f65295 100644 --- a/benchmark/http/chunked.js +++ b/benchmark/http/chunked.js @@ -32,10 +32,11 @@ function main({ len, n, c, duration }) { send(n); }); - server.listen(common.PORT, () => { + server.listen(0, () => { bench.http({ connections: c, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/client-request-body.js b/benchmark/http/client-request-body.js index 6e2323b3f07e8a..3d673d36b7bc63 100644 --- a/benchmark/http/client-request-body.js +++ b/benchmark/http/client-request-body.js @@ -32,7 +32,6 @@ function main({ dur, len, type, method }) { headers: { 'Connection': 'keep-alive', 'Transfer-Encoding': 'chunked' }, agent: new http.Agent({ maxSockets: 1 }), host: '127.0.0.1', - port: common.PORT, path: '/', method: 'POST' }; @@ -40,16 +39,17 @@ function main({ dur, len, type, method }) { const server = http.createServer((req, res) => { res.end(); }); - server.listen(options.port, options.host, () => { + server.listen(0, options.host, () => { setTimeout(done, dur * 1000); bench.start(); - pummel(); + pummel(server.address().port); }); - function pummel() { + function pummel(port) { + options.port = port; const req = http.request(options, (res) => { nreqs++; - pummel(); // Line up next request. + pummel(port); // Line up next request. res.resume(); }); if (method === 'write') { diff --git a/benchmark/http/end-vs-write-end.js b/benchmark/http/end-vs-write-end.js index 60174ef3adf4f2..9f128bf8c1499e 100644 --- a/benchmark/http/end-vs-write-end.js +++ b/benchmark/http/end-vs-write-end.js @@ -48,10 +48,11 @@ function main({ len, type, method, c, duration }) { fn(res); }); - server.listen(common.PORT, () => { + server.listen(0, () => { bench.http({ connections: c, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/headers.js b/benchmark/http/headers.js index a3d2b7810be676..b405868eb0ea2b 100644 --- a/benchmark/http/headers.js +++ b/benchmark/http/headers.js @@ -27,11 +27,12 @@ function main({ len, n, duration }) { res.writeHead(200, headers); res.end(); }); - server.listen(common.PORT, () => { + server.listen(0, () => { bench.http({ path: '/', connections: 10, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/incoming_headers.js b/benchmark/http/incoming_headers.js index 983bd5632fcb7d..7501d2cb92d456 100644 --- a/benchmark/http/incoming_headers.js +++ b/benchmark/http/incoming_headers.js @@ -14,7 +14,7 @@ function main({ connections, headers, w, duration }) { res.end(); }); - server.listen(common.PORT, () => { + server.listen(0, () => { const headers = { 'Content-Type': 'text/plain', 'Accept': 'text/plain', @@ -34,7 +34,8 @@ function main({ connections, headers, w, duration }) { path: '/', connections, headers, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/set-header.js b/benchmark/http/set-header.js index 48e0163a6ced10..47681ebd1b1405 100644 --- a/benchmark/http/set-header.js +++ b/benchmark/http/set-header.js @@ -1,6 +1,5 @@ 'use strict'; const common = require('../common.js'); -const PORT = common.PORT; const bench = common.createBenchmark(main, { res: ['normal', 'setHeader', 'setHeaderWH'], @@ -17,16 +16,16 @@ const c = 50; // setHeader: statusCode = status, setHeader(...) x2 // setHeaderWH: setHeader(...), writeHead(status, ...) function main({ res, duration }) { - process.env.PORT = PORT; const server = require('../fixtures/simple-http-server.js') - .listen(PORT) + .listen(0) .on('listening', () => { const path = `/${type}/${len}/${chunks}/${res}/${chunkedEnc}`; bench.http({ path: path, connections: c, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/simple.js b/benchmark/http/simple.js index 095b15ca4465fb..31e12ca7c27438 100644 --- a/benchmark/http/simple.js +++ b/benchmark/http/simple.js @@ -13,14 +13,15 @@ const bench = common.createBenchmark(main, { function main({ type, len, chunks, c, chunkedEnc, duration }) { const server = require('../fixtures/simple-http-server.js') - .listen(common.PORT) + .listen(0) .on('listening', () => { const path = `/${type}/${len}/${chunks}/normal/${chunkedEnc}`; bench.http({ path, connections: c, - duration + duration, + port: server.address().port, }, () => { server.close(); }); diff --git a/benchmark/http/upgrade.js b/benchmark/http/upgrade.js index 8d365fe46df24e..c4f5cd342284c3 100644 --- a/benchmark/http/upgrade.js +++ b/benchmark/http/upgrade.js @@ -20,7 +20,7 @@ const resData = 'HTTP/1.1 101 Web Socket Protocol Handshake\r\n' + function main({ n }) { const server = require('../fixtures/simple-http-server.js') - .listen(common.PORT) + .listen(0) .on('listening', () => { bench.start(); doBench(server.address(), n, () => { diff --git a/doc/api/http.md b/doc/api/http.md index 5c3c7d9a7a7180..851679dfc310d8 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()` @@ -1401,9 +1398,14 @@ Limits maximum incoming headers count. If set to 0, no limit will be applied. -* {number} **Default:** `0` +* {number} **Default:** `300000` Sets the timeout value in milliseconds for receiving the entire request from the client. @@ -2856,6 +2858,10 @@ Found'`.