From b9204dbc0eedcb3c21b5e80a9dac63301de2c729 Mon Sep 17 00:00:00 2001
From: Brian White <mscdex@mscdex.net>
Date: Thu, 31 Jan 2019 20:44:33 -0500
Subject: [PATCH] http: fix error check in `Execute()`

Refs: https://github.com/nodejs/node/pull/24738
Fixes: https://github.com/nodejs/node/issues/25858

PR-URL: https://github.com/nodejs/node/pull/25863
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 src/node_http_parser.cc                    | 30 ++++++++++++++++++++--
 test/parallel/test-http-max-header-size.js | 15 ++++++++++-
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index 4411b6b32f8e2b..b29cdf4b28b9be 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -605,7 +605,28 @@ class Parser : public AsyncWrap, public StreamListener {
     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();
@@ -619,7 +640,7 @@ class Parser : public AsyncWrap, public StreamListener {
     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());
@@ -631,6 +652,11 @@ class Parser : public AsyncWrap, public StreamListener {
 
       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
index 07fbe902963525..b70deff764dc5d 100644
--- 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.mustCall((res) => {
+      assert.strictEqual(res.statusCode, 400);
+      server.close();
+    }));
+  }));
+}