http: defer reentrant execution of Parser::Execute

PR-URL: nodejs#43369 Fixes: nodejs#39671 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
F3n67u · Jun 24, 2022 · c555eca · c555eca
1 parent 95fa67f
commit c555eca
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 73 deletions.
diff --git a/lib/_http_common.js b/lib/_http_common.js
@@ -117,17 +117,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);
   }

diff --git 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)
 
@@ -454,32 +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<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;
@@ -593,17 +577,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())
@@ -615,7 +591,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())
@@ -695,11 +670,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 {
@@ -801,7 +771,6 @@ class Parser : public AsyncWrap, public StreamListener {
     if (nread == 0)
       return;
 
-    current_buffer_.Clear();
     Local<Value> ret = Execute(buf.base, nread);
 
     // Exception
@@ -834,17 +803,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;
@@ -864,8 +828,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;
 
@@ -989,8 +951,6 @@ class Parser : public AsyncWrap, public StreamListener {
 
 
   int MaybePause() {
-    CHECK_NE(execute_depth_, 0);
-
     if (!pending_pause_) {
       return 0;
     }
@@ -1018,10 +978,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 headers_completed_ = false;
   bool pending_pause_ = false;
   uint64_t header_nread_ = 0;

diff --git a/test/parallel/test-http-parser-multiple-execute.js b/test/parallel/test-http-parser-multiple-execute.js
@@ -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();
diff --git 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);
   };