From 3fe3bc961f751e929a3386c084d22ac7b051f521 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 29 Nov 2018 22:01:33 -0500 Subject: [PATCH] http: fix error return in `Finish()` `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. Fix: #24585 PR-URL: https://github.com/nodejs/node/pull/24738 Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- src/node_http_parser.cc | 32 ++++++++++++++++--- .../parallel/test-http-parser-finish-error.js | 27 ++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-parser-finish-error.js diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 18ac6599d80919..4d683b4db2cfaa 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -491,7 +491,10 @@ class Parser : public AsyncWrap, public StreamListener { ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); CHECK(parser->current_buffer_.IsEmpty()); - parser->Execute(nullptr, 0); + Local ret = parser->Execute(nullptr, 0); + + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret); } @@ -684,11 +687,28 @@ class Parser : public AsyncWrap, public StreamListener { } #else /* !NODE_EXPERIMENTAL_HTTP */ size_t nread = http_parser_execute(&parser_, &settings, data, len); - if (data != nullptr) { + 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 (nread) { + case 0: + err = HPE_OK; + break; + case 1: + nread = 0; + break; + default: + UNREACHABLE(); + } + + // Regular Execute() + } else { Save(); } - - err = HTTP_PARSER_ERRNO(&parser_); #endif /* NODE_EXPERIMENTAL_HTTP */ // Unassign the 'buffer_' variable @@ -738,6 +758,10 @@ class Parser : public AsyncWrap, public StreamListener { return scope.Escape(e); } + // No return value is needed for `Finish()` + if (data == nullptr) { + return scope.Escape(Local()); + } return scope.Escape(nread_obj); } diff --git a/test/parallel/test-http-parser-finish-error.js b/test/parallel/test-http-parser-finish-error.js new file mode 100644 index 00000000000000..6d47a762dc8bfa --- /dev/null +++ b/test/parallel/test-http-parser-finish-error.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const net = require('net'); +const http = require('http'); +const assert = require('assert'); + +const str = 'GET / HTTP/1.1\r\n' + + 'Content-Length:'; + + +const server = http.createServer(common.mustNotCall()); +server.on('clientError', common.mustCall((err, socket) => { + assert(/^Parse Error/.test(err.message)); + assert.strictEqual(err.code, 'HPE_INVALID_EOF_STATE'); + socket.destroy(); +}, 1)); +server.listen(0, () => { + const client = net.connect({ port: server.address().port }, () => { + client.on('data', common.mustNotCall()); + client.on('end', common.mustCall(() => { + server.close(); + })); + client.write(str); + client.end(); + }); +});