diff options
author | Brian White <mscdex@mscdex.net> | 2019-02-04 21:29:47 -0500 |
---|---|---|
committer | Beth Griggs <Bethany.Griggs@uk.ibm.com> | 2019-03-20 17:16:10 +0000 |
commit | ac9b8f7645da1818068336c269e605025b0fb467 (patch) | |
tree | c205af25c2484abbf3c07e4b3f79bf7150a5586b | |
parent | 1d862610f85aa3ee8ca2e7a4f120794eff7133c4 (diff) | |
download | node-new-ac9b8f7645da1818068336c269e605025b0fb467.tar.gz |
http: fix error check in `Execute()`
`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: https://github.com/nodejs/node/pull/25938
Fix: #24585
PR-URL: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
-rw-r--r-- | src/node_http_parser.cc | 30 | ||||
-rw-r--r-- | 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 504130e5d7..1b03fe1c71 100644 --- 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 index 07fbe90296..43dcbec5ab 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.mustNotCall()).once('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ECONNRESET'); + server.close(); + })); + })); +} |