http: fix error check in Execute()

mscdex · BethGriggs · commit ac9b8f7645da · 2019-03-20T17:16:10.000Z
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The expectation is that no error must be returned if it is `0`, and if it is `1` - a `Error` object must be returned back to user. The introduction of `llhttp` and the refactor that happened during it accidentally removed the error-returning code. This commit reverts it back to its original state. Backport-PR-URL: #25938 Fix: #24585 PR-URL: #24738 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -621,7 +621,28 @@ class Parser : public AsyncWrap {
     size_t nparsed =
       http_parser_execute(&parser_, &settings, data, len);
 
-    Save();
+    enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
+
+    // Finish()
+    if (data == nullptr) {
+      // `http_parser_execute()` returns either `0` or `1` when `len` is 0
+      // (part of the finishing sequence).
+      CHECK_EQ(len, 0);
+      switch (nparsed) {
+        case 0:
+          err = HPE_OK;
+          break;
+        case 1:
+          nparsed = 0;
+          break;
+        default:
+          UNREACHABLE();
+      }
+
+    // Regular Execute()
+    } else {
+      Save();
+    }
 
     // Unassign the 'buffer_' variable
     current_buffer_.Clear();
@@ -635,7 +656,7 @@ class Parser : public AsyncWrap {
     Local<Integer> nparsed_obj = Integer::New(env()->isolate(), nparsed);
     // If there was a parse error in one of the callbacks
     // TODO(bnoordhuis) What if there is an error on EOF?
-    if (!parser_.upgrade && nparsed != len) {
+    if (!parser_.upgrade && err != HPE_OK) {
       enum http_errno err = HTTP_PARSER_ERRNO(&parser_);
 
       Local<Value> e = Exception::Error(env()->parse_error_string());
@@ -646,6 +667,11 @@ class Parser : public AsyncWrap {
 
       return scope.Escape(e);
     }
+
+    // No return value is needed for `Finish()`
+    if (data == nullptr) {
+      return scope.Escape(Local<Value>());
+    }
     return scope.Escape(nparsed_obj);
   }
 
diff --git a/test/parallel/test-http-max-header-size.js b/test/parallel/test-http-max-header-size.js
@@ -1,6 +1,6 @@
 'use strict';
 
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const { spawnSync } = require('child_process');
 const http = require('http');
@@ -9,3 +9,16 @@ assert.strictEqual(http.maxHeaderSize, 8 * 1024);
 const child = spawnSync(process.execPath, ['--max-http-header-size=10', '-p',
                                            'http.maxHeaderSize']);
 assert.strictEqual(+child.stdout.toString().trim(), 10);
+
+{
+  const server = http.createServer(common.mustNotCall());
+  server.listen(0, common.mustCall(() => {
+    http.get({
+      port: server.address().port,
+      headers: { foo: 'x'.repeat(http.maxHeaderSize + 1) }
+    }, common.mustNotCall()).once('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ECONNRESET');
+      server.close();
+    }));
+  }));
+}
