From 1ac14244763a7f87d51a517f0817eca82cbc86b6 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 7 Mar 2018 14:16:34 +0100
Subject: [PATCH] http: align parser with StreamBase interface changes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 src/env-inl.h           |  8 ++++++++
 src/env.h               |  3 +++
 src/node_http_parser.cc | 17 +++++++++++++++++
 src/util.h              |  8 ++++++++
 4 files changed, 36 insertions(+)

diff --git a/src/env-inl.h b/src/env-inl.h
index 05c7a90c52fee7..3fe57f808cfec7 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 868f3e9f212f67..4b874677935edb 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 207310f4068f43..8ab13e07340929 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 c822390ec56f8b..e871fc63a5c46a 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_