From 25fed3b4e6c518270cdc1aa9dd4652e432328b16 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Fri, 10 Jun 2022 16:25:15 +0200 Subject: [PATCH 1/6] http: defer reentrant execution of Parser::Execute --- src/node_http_parser.cc | 29 +++++++++++++++++++ .../test-http-parser-multiple-execute.js | 26 +++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 test/parallel/test-http-parser-multiple-execute.js diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 7ff5b0fe2ff76f..8ffc9449f70268 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -593,6 +593,35 @@ class Parser : public AsyncWrap, public StreamListener { static void Execute(const FunctionCallbackInfo& args) { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); + + // If parser.Execute is invoked within one of the callbacks, + // like kOnHeadersComplete, it is scheduled before the buffer is + // emptied and thus all assertions fails. For this reason we + // postpone the actual execution. + if (!parser->current_buffer_.IsEmpty()) { + ArrayBufferViewContents buffer(args[0]); + + Environment::GetCurrent(args)->SetImmediate( + [parser, args, buffer](Environment* env) { + CHECK(parser->current_buffer_.IsEmpty()); + CHECK_EQ(parser->current_buffer_len_, 0); + CHECK_NULL(parser->current_buffer_data_); + + // This is a hack to get the current_buffer to the callbacks + // with the least amount of overhead. Nothing else will run + // while http_parser_execute() runs, therefore this pointer + // can be set and used for the execution. + parser->current_buffer_ = args[0].As(); + + Local ret = parser->Execute(buffer.data(), buffer.length()); + + if (!ret.IsEmpty()) + args.GetReturnValue().Set(ret); + }); + + return; + } + CHECK(parser->current_buffer_.IsEmpty()); CHECK_EQ(parser->current_buffer_len_, 0); CHECK_NULL(parser->current_buffer_data_); diff --git a/test/parallel/test-http-parser-multiple-execute.js b/test/parallel/test-http-parser-multiple-execute.js new file mode 100644 index 00000000000000..c85b8362d35636 --- /dev/null +++ b/test/parallel/test-http-parser-multiple-execute.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { HTTPParser } = process.binding('http_parser'); + +let second = false; +const parser = new HTTPParser(HTTPParser.RESPONSE, false); + +parser.initialize(HTTPParser.RESPONSE, {}, 0, 0); + +parser[HTTPParser.kOnHeadersComplete] = common.mustCall( + function(_versionMajor, _versionMinor, _headers, _method, _url, statusCode) { + if (!second) { + second = true; + + assert.strictEqual(statusCode, 100); + parser.execute(Buffer.from('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n')); + } else { + assert.strictEqual(statusCode, 200); + } + }, + 2 +); + +parser.execute(Buffer.from('HTTP/1.1 100 Continue\r\n\r\n')); From a9aa27834751f63d465ad694e51288c25dc29db0 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Wed, 15 Jun 2022 10:41:35 +0200 Subject: [PATCH 2/6] http: simplify parser --- src/node_http_parser.cc | 75 ++++------------------------------------- 1 file changed, 6 insertions(+), 69 deletions(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 8ffc9449f70268..bea7243a2ce272 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -248,11 +248,7 @@ class Parser : public AsyncWrap, public StreamListener { binding_data_(binding_data) { } - - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("current_buffer", current_buffer_); - } - + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(Parser) SET_SELF_SIZE(Parser) @@ -462,19 +458,14 @@ class Parser : public AsyncWrap, public StreamListener { if (!cb->IsFunction()) return 0; - // We came from consumed stream - if (current_buffer_.IsEmpty()) { - // Make sure Buffer will be in parent HandleScope - current_buffer_ = scope.Escape(Buffer::Copy( - env()->isolate(), - current_buffer_data_, - current_buffer_len_).ToLocalChecked()); - } + // Make sure Buffer will be in parent HandleScope + Local current_buffer = scope.Escape( + Buffer::Copy(env()->isolate(), at, length).ToLocalChecked()); Local argv[3] = { - current_buffer_, + current_buffer, Integer::NewFromUnsigned( - env()->isolate(), static_cast(at - current_buffer_data_)), + env()->isolate(), 0), Integer::NewFromUnsigned(env()->isolate(), length)}; MaybeLocal r = MakeCallback(cb.As(), @@ -594,45 +585,8 @@ class Parser : public AsyncWrap, public StreamListener { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - // If parser.Execute is invoked within one of the callbacks, - // like kOnHeadersComplete, it is scheduled before the buffer is - // emptied and thus all assertions fails. For this reason we - // postpone the actual execution. - if (!parser->current_buffer_.IsEmpty()) { - ArrayBufferViewContents buffer(args[0]); - - Environment::GetCurrent(args)->SetImmediate( - [parser, args, buffer](Environment* env) { - CHECK(parser->current_buffer_.IsEmpty()); - CHECK_EQ(parser->current_buffer_len_, 0); - CHECK_NULL(parser->current_buffer_data_); - - // This is a hack to get the current_buffer to the callbacks - // with the least amount of overhead. Nothing else will run - // while http_parser_execute() runs, therefore this pointer - // can be set and used for the execution. - parser->current_buffer_ = args[0].As(); - - Local ret = parser->Execute(buffer.data(), buffer.length()); - - if (!ret.IsEmpty()) - args.GetReturnValue().Set(ret); - }); - - return; - } - - CHECK(parser->current_buffer_.IsEmpty()); - CHECK_EQ(parser->current_buffer_len_, 0); - CHECK_NULL(parser->current_buffer_data_); - ArrayBufferViewContents buffer(args[0]); - // This is a hack to get the current_buffer to the callbacks with the least - // amount of overhead. Nothing else will run while http_parser_execute() - // runs, therefore this pointer can be set and used for the execution. - parser->current_buffer_ = args[0].As(); - Local ret = parser->Execute(buffer.data(), buffer.length()); if (!ret.IsEmpty()) @@ -644,7 +598,6 @@ class Parser : public AsyncWrap, public StreamListener { Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); - CHECK(parser->current_buffer_.IsEmpty()); Local ret = parser->Execute(nullptr, 0); if (!ret.IsEmpty()) @@ -724,11 +677,6 @@ class Parser : public AsyncWrap, public StreamListener { // Should always be called from the same context. CHECK_EQ(env, parser->env()); - if (parser->execute_depth_) { - parser->pending_pause_ = should_pause; - return; - } - if (should_pause) { llhttp_pause(&parser->parser_); } else { @@ -830,7 +778,6 @@ class Parser : public AsyncWrap, public StreamListener { if (nread == 0) return; - current_buffer_.Clear(); Local ret = Execute(buf.base, nread); // Exception @@ -863,17 +810,12 @@ class Parser : public AsyncWrap, public StreamListener { llhttp_errno_t err; - // Do not allow re-entering `http_parser_execute()` - CHECK_EQ(execute_depth_, 0); - - execute_depth_++; if (data == nullptr) { err = llhttp_finish(&parser_); } else { err = llhttp_execute(&parser_, data, len); Save(); } - execute_depth_--; // Calculate bytes read and resume after Upgrade/CONNECT pause size_t nread = len; @@ -893,8 +835,6 @@ class Parser : public AsyncWrap, public StreamListener { llhttp_pause(&parser_); } - // Unassign the 'buffer_' variable - current_buffer_.Clear(); current_buffer_len_ = 0; current_buffer_data_ = nullptr; @@ -1018,8 +958,6 @@ class Parser : public AsyncWrap, public StreamListener { int MaybePause() { - CHECK_NE(execute_depth_, 0); - if (!pending_pause_) { return 0; } @@ -1047,7 +985,6 @@ class Parser : public AsyncWrap, public StreamListener { size_t num_values_; bool have_flushed_; bool got_exception_; - Local current_buffer_; size_t current_buffer_len_; const char* current_buffer_data_; unsigned int execute_depth_ = 0; From 0627a78a0b89e1828f8c4d28ebd540d3e1d56d03 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Wed, 15 Jun 2022 14:22:09 +0200 Subject: [PATCH 3/6] http: remove useless slice --- lib/_http_common.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index 2055ca205b84a3..53737957cc9bda 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -126,8 +126,7 @@ function parserOnBody(b, start, len) { // Pretend this was the result of a stream._read call. if (len > 0 && !stream._dumped) { - const slice = b.slice(start, start + len); - const ret = stream.push(slice); + const ret = stream.push(b); if (!ret) readStop(this.socket); } From 5c14718e154231b215ff948a369380649f8fac6b Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Fri, 17 Jun 2022 10:14:03 +0200 Subject: [PATCH 4/6] http: simplify method --- lib/_http_common.js | 4 ++-- src/node_http_parser.cc | 23 ++++++++------------- test/parallel/test-http-parser.js | 34 +++++++++++++++---------------- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index 53737957cc9bda..4430869520f813 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -117,7 +117,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, return parser.onIncoming(incoming, shouldKeepAlive); } -function parserOnBody(b, start, len) { +function parserOnBody(b) { const stream = this.incoming; // If the stream has already been removed, then drop it. @@ -125,7 +125,7 @@ function parserOnBody(b, start, len) { return; // Pretend this was the result of a stream._read call. - if (len > 0 && !stream._dumped) { + if (!stream._dumped) { const ret = stream.push(b); if (!ret) readStop(this.socket); diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index bea7243a2ce272..57b0ab29c5dc0a 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -450,27 +450,20 @@ class Parser : public AsyncWrap, public StreamListener { int on_body(const char* at, size_t length) { - EscapableHandleScope scope(env()->isolate()); + if (length == 0) + return 0; - Local obj = object(); - Local cb = obj->Get(env()->context(), kOnBody).ToLocalChecked(); + Environment* env = this->env(); + HandleScope handle_scope(env->isolate()); + + Local cb = object()->Get(env->context(), kOnBody).ToLocalChecked(); if (!cb->IsFunction()) return 0; - // Make sure Buffer will be in parent HandleScope - Local current_buffer = scope.Escape( - Buffer::Copy(env()->isolate(), at, length).ToLocalChecked()); + Local buffer = Buffer::Copy(env, at, length).ToLocalChecked(); - Local argv[3] = { - current_buffer, - Integer::NewFromUnsigned( - env()->isolate(), 0), - Integer::NewFromUnsigned(env()->isolate(), length)}; - - MaybeLocal r = MakeCallback(cb.As(), - arraysize(argv), - argv); + MaybeLocal r = MakeCallback(cb.As(), 1, &buffer); if (r.IsEmpty()) { got_exception_ = true; diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 97dc57f755ad88..739beaa3d67cd9 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -62,8 +62,8 @@ function newParser(type) { function expectBody(expected) { - return mustCall(function(buf, start, len) { - const body = String(buf.slice(start, start + len)); + return mustCall(function(buf) { + const body = String(buf); assert.strictEqual(body, expected); }); } @@ -126,8 +126,8 @@ function expectBody(expected) { assert.strictEqual(statusMessage, 'OK'); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'pong'); }; @@ -195,8 +195,8 @@ function expectBody(expected) { parser[kOnHeaders] = mustCall(onHeaders); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'ping'); seen_body = true; }; @@ -291,8 +291,8 @@ function expectBody(expected) { assert.strictEqual(versionMinor, 1); }; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, 'foo=42&bar=1337'); }; @@ -332,8 +332,8 @@ function expectBody(expected) { let body_part = 0; const body_parts = ['123', '123456', '1234567890']; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, body_parts[body_part++]); }; @@ -371,8 +371,8 @@ function expectBody(expected) { const body_parts = ['123', '123456', '123456789', '123456789ABC', '123456789ABCDEF']; - const onBody = (buf, start, len) => { - const body = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const body = String(buf); assert.strictEqual(body, body_parts[body_part++]); }; @@ -428,8 +428,8 @@ function expectBody(expected) { let expected_body = '123123456123456789123456789ABC123456789ABCDEF'; - const onBody = (buf, start, len) => { - const chunk = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const chunk = String(buf); assert.strictEqual(expected_body.indexOf(chunk), 0); expected_body = expected_body.slice(chunk.length); }; @@ -445,9 +445,7 @@ function expectBody(expected) { for (let i = 1; i < request.length - 1; ++i) { const a = request.slice(0, i); - console.error(`request.slice(0, ${i}) = ${JSON.stringify(a.toString())}`); const b = request.slice(i); - console.error(`request.slice(${i}) = ${JSON.stringify(b.toString())}`); test(a, b); } } @@ -488,8 +486,8 @@ function expectBody(expected) { let expected_body = '123123456123456789123456789ABC123456789ABCDEF'; - const onBody = (buf, start, len) => { - const chunk = String(buf.slice(start, start + len)); + const onBody = (buf) => { + const chunk = String(buf); assert.strictEqual(expected_body.indexOf(chunk), 0); expected_body = expected_body.slice(chunk.length); }; From 2f292a5029da9ecb681e8ddf21494886dcce7537 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Fri, 17 Jun 2022 11:48:28 +0200 Subject: [PATCH 5/6] http: test using public API only --- .../test-http-parser-multiple-execute.js | 45 ++++++++++++------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-http-parser-multiple-execute.js b/test/parallel/test-http-parser-multiple-execute.js index c85b8362d35636..d05a973752395a 100644 --- a/test/parallel/test-http-parser-multiple-execute.js +++ b/test/parallel/test-http-parser-multiple-execute.js @@ -2,25 +2,36 @@ const common = require('../common'); const assert = require('assert'); -const { HTTPParser } = process.binding('http_parser'); +const { request } = require('http'); +const { Duplex } = require('stream'); -let second = false; -const parser = new HTTPParser(HTTPParser.RESPONSE, false); +let socket; -parser.initialize(HTTPParser.RESPONSE, {}, 0, 0); +function createConnection(...args) { + socket = new Duplex({ + read() {}, + write(chunk, encoding, callback) { + if (chunk.toString().includes('\r\n\r\n')) { + this.push('HTTP/1.1 100 Continue\r\n\r\n'); + } -parser[HTTPParser.kOnHeadersComplete] = common.mustCall( - function(_versionMajor, _versionMinor, _headers, _method, _url, statusCode) { - if (!second) { - second = true; - - assert.strictEqual(statusCode, 100); - parser.execute(Buffer.from('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n')); - } else { - assert.strictEqual(statusCode, 200); + callback(); } - }, - 2 -); + }); + + return socket; +} + +const req = request('http://localhost:8080', { createConnection }); + +req.on('information', common.mustCall(({ statusCode }) => { + assert.strictEqual(statusCode, 100); + socket.push('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n'); + socket.push(null); +})); + +req.on('response', common.mustCall(({ statusCode }) => { + assert.strictEqual(statusCode, 200); +})); -parser.execute(Buffer.from('HTTP/1.1 100 Continue\r\n\r\n')); +req.end(); From 0ea549a5f791701255c399f78a36e1e931495d6e Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Tue, 21 Jun 2022 15:08:42 +0200 Subject: [PATCH 6/6] http: removed useless field --- src/node_http_parser.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 57b0ab29c5dc0a..52c8e9e2589dd4 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -980,7 +980,6 @@ class Parser : public AsyncWrap, public StreamListener { bool got_exception_; size_t current_buffer_len_; const char* current_buffer_data_; - unsigned int execute_depth_ = 0; bool headers_completed_ = false; bool pending_pause_ = false; uint64_t header_nread_ = 0;