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: #32329 Fixes: #27363 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · Apr 11, 2020 · 8597df4 · 8597df4
1 parent 4acd7f4
commit 8597df4
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<Number>()->Value();
+    }
+
     llhttp_type_t type =
         static_cast<llhttp_type_t>(args[0].As<Int32>()->Value());
 
@@ -532,7 +540,7 @@ class Parser : public AsyncWrap, public StreamListener {
 
     parser->set_provider_type(provider);
     parser->AsyncReset(args[1].As<Object>());
-    parser->Init(type, max_http_header_size, lenient);
+    parser->Init(type, max_http_header_size, lenient, headers_timeout);
   }
 
   template <bool should_pause>
@@ -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<Value> cb =
+            object()->Get(env()->context(), kOnTimeout).ToLocalChecked();
+
+        if (!cb->IsFunction())
+          return;
+
+        MakeCallback(cb.As<Function>(), 0, nullptr);
+
+        return;
+      }
+    }
+
     Local<Value> 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<Object> 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<Array> 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
@@ -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
@@ -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();
+  }));
+});