http: align parser with StreamBase interface changes

The `StreamBase` interface changed, so that `OnStreamRead()`
and `OnStreamAlloc()` are not guaranteed to be emitted in the
same tick any more.

This means that, while it isn’t causing any trouble right now,
we should not assume that it’s safe to return a static buffer
in the HTTP parser’s `OnStreamAlloc()` method.

PR-URL: https://github.com/nodejs/node/pull/18936
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

4 files changed, 36 insertions, 0 deletions

diff --git a/src/env-inl.h b/src/env-inl.h
index 05c7a90c52..3fe57f808c 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -469,6 +469,14 @@ inline void Environment::set_http_parser_buffer(char* buffer) {
   http_parser_buffer_ = buffer;
 }
 
+inline bool Environment::http_parser_buffer_in_use() const {
+  return http_parser_buffer_in_use_;
+}
+
+inline void Environment::set_http_parser_buffer_in_use(bool in_use) {
+  http_parser_buffer_in_use_ = in_use;
+}
+
 inline http2::http2_state* Environment::http2_state() const {
   return http2_state_.get();
 }
diff --git a/src/env.h b/src/env.h
index 868f3e9f21..4b87467793 100644
--- a/src/env.h
+++ b/src/env.h
@@ -646,6 +646,8 @@ class Environment {
 
   inline char* http_parser_buffer() const;
   inline void set_http_parser_buffer(char* buffer);
+  inline bool http_parser_buffer_in_use() const;
+  inline void set_http_parser_buffer_in_use(bool in_use);
 
   inline http2::http2_state* http2_state() const;
   inline void set_http2_state(std::unique_ptr<http2::http2_state> state);
@@ -828,6 +830,7 @@ class Environment {
   double* heap_space_statistics_buffer_ = nullptr;
 
   char* http_parser_buffer_;
+  bool http_parser_buffer_in_use_ = false;
   std::unique_ptr<http2::http2_state> http2_state_;
 
   // stat fields contains twice the number of entries because `fs.StatWatcher`
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index 207310f406..8ab13e0734 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -525,6 +525,14 @@ class Parser : public AsyncWrap, public StreamListener {
   static const size_t kAllocBufferSize = 64 * 1024;
 
   uv_buf_t OnStreamAlloc(size_t suggested_size) override {
+    // For most types of streams, OnStreamRead will be immediately after
+    // OnStreamAlloc, and will consume all data, so using a static buffer for
+    // reading is more efficient. For other streams, just use the default
+    // allocator, which uses Malloc().
+    if (env()->http_parser_buffer_in_use())
+      return StreamListener::OnStreamAlloc(suggested_size);
+    env()->set_http_parser_buffer_in_use(true);
+
     if (env()->http_parser_buffer() == nullptr)
       env()->set_http_parser_buffer(new char[kAllocBufferSize]);
 
@@ -534,6 +542,15 @@ class Parser : public AsyncWrap, public StreamListener {
 
   void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override {
     HandleScope scope(env()->isolate());
+    // Once we’re done here, either indicate that the HTTP parser buffer
+    // is free for re-use, or free() the data if it didn’t come from there
+    // in the first place.
+    OnScopeLeave on_scope_leave([&]() {
+      if (buf.base == env()->http_parser_buffer())
+        env()->set_http_parser_buffer_in_use(false);
+      else
+        free(buf.base);
+    });
 
     if (nread < 0) {
       PassReadErrorToPreviousListener(nread);
diff --git a/src/util.h b/src/util.h
index c822390ec5..e871fc63a5 100644
--- a/src/util.h
+++ b/src/util.h
@@ -436,6 +436,14 @@ class BufferValue : public MaybeStackBuffer<char> {
 template <typename T> inline void USE(T&&) {}
 }  // namespace node
 
+// Run a function when exiting the current scope.
+struct OnScopeLeave {
+  std::function<void()> fn_;
+
+  explicit OnScopeLeave(std::function<void()> fn) : fn_(fn) {}
+  ~OnScopeLeave() { fn_(); }
+};
+
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #endif  // SRC_UTIL_H_