Skip to content

Commit

Permalink
http: fix error return in Finish()
Browse files Browse the repository at this point in the history
`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: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
indutny committed Dec 2, 2018
1 parent 3fb627b commit 175164e
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 4 deletions.
32 changes: 28 additions & 4 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value> ret = parser->Execute(nullptr, 0);

if (!ret.IsEmpty())
args.GetReturnValue().Set(ret);
}


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<Value>());
}
return scope.Escape(nread_obj);
}

Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-http-parser-finish-error.js
Original file line number Diff line number Diff line change
@@ -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();
});
});

0 comments on commit 175164e

Please sign in to comment.