Skip to content

Commit

Permalink
http: defer reentrant execution of Parser::Execute
Browse files Browse the repository at this point in the history 
PR-URL: #43369
Fixes: #39671
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
@ShogunPanda @targos
ShogunPanda authored and targos committed Jul 31, 2022
1 parent 5fd26da commit 58eebaf
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 73 deletions.
7 changes: 3 additions & 4 deletions lib/_http_common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,16 @@ 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.
if (stream === null)
return;

// 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);
if (!stream._dumped) {
const ret = stream.push(b);
if (!ret)
readStop(this.socket);
}
Expand Down
60 changes: 9 additions & 51 deletions src/node_http_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,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)

Expand Down Expand Up @@ -392,32 +388,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<Object> obj = object();
Local<Value> cb = obj->Get(env()->context(), kOnBody).ToLocalChecked();
Environment* env = this->env();
HandleScope handle_scope(env->isolate());

Local<Value> cb = object()->Get(env->context(), kOnBody).ToLocalChecked();

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());
}
Local<Value> buffer = Buffer::Copy(env, at, length).ToLocalChecked();

Local<Value> argv[3] = {
current_buffer_,
Integer::NewFromUnsigned(
env()->isolate(), static_cast<uint32_t>(at - current_buffer_data_)),
Integer::NewFromUnsigned(env()->isolate(), length)};

MaybeLocal<Value> r = MakeCallback(cb.As<Function>(),
arraysize(argv),
argv);
MaybeLocal<Value> r = MakeCallback(cb.As<Function>(), 1, &buffer);

if (r.IsEmpty()) {
got_exception_ = true;
Expand Down Expand Up @@ -514,17 +498,9 @@ class Parser : public AsyncWrap, public StreamListener {
static void Execute(const FunctionCallbackInfo<Value>& args) {
Parser* parser;
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
CHECK(parser->current_buffer_.IsEmpty());
CHECK_EQ(parser->current_buffer_len_, 0);
CHECK_NULL(parser->current_buffer_data_);

ArrayBufferViewContents<char> 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<Object>();

Local<Value> ret = parser->Execute(buffer.data(), buffer.length());

if (!ret.IsEmpty())
Expand All @@ -536,7 +512,6 @@ class Parser : public AsyncWrap, public StreamListener {
Parser* parser;
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());

CHECK(parser->current_buffer_.IsEmpty());
Local<Value> ret = parser->Execute(nullptr, 0);

if (!ret.IsEmpty())
Expand Down Expand Up @@ -600,11 +575,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 {
Expand Down Expand Up @@ -686,7 +656,6 @@ class Parser : public AsyncWrap, public StreamListener {
if (nread == 0)
return;

current_buffer_.Clear();
Local<Value> ret = Execute(buf.base, nread);

// Exception
Expand Down Expand Up @@ -737,17 +706,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;
Expand All @@ -767,8 +731,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;

Expand Down Expand Up @@ -893,8 +855,6 @@ class Parser : public AsyncWrap, public StreamListener {


int MaybePause() {
CHECK_NE(execute_depth_, 0);

if (!pending_pause_) {
return 0;
}
Expand Down Expand Up @@ -922,10 +882,8 @@ class Parser : public AsyncWrap, public StreamListener {
size_t num_values_;
bool have_flushed_;
bool got_exception_;
Local<Object> current_buffer_;
size_t current_buffer_len_;
const char* current_buffer_data_;
unsigned int execute_depth_ = 0;
bool pending_pause_ = false;
uint64_t header_nread_ = 0;
uint64_t max_http_header_size_;
Expand Down
37 changes: 37 additions & 0 deletions test/parallel/test-http-parser-multiple-execute.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const { request } = require('http');
const { Duplex } = require('stream');

let socket;

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');
}

callback();
}
});

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);
}));

req.end();
34 changes: 16 additions & 18 deletions test/parallel/test-http-parser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down Expand Up @@ -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');
};

Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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');
};

Expand Down Expand Up @@ -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++]);
};

Expand Down Expand Up @@ -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++]);
};

Expand Down Expand Up @@ -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);
};
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
};
Expand Down

0 comments on commit 58eebaf

Please sign in to comment.