From 88726f2384f3b3b50e103984f31bbbc6881d7ab1 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 10 Aug 2019 01:30:35 +0200
Subject: [PATCH] http2: do not create ArrayBuffers when no DATA received
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Lazily allocate `ArrayBuffer`s for the contents of DATA frames.
Creating `ArrayBuffer`s is, sadly, not a cheap operation with V8.

This is part of performance improvements to mitigate CVE-2019-9513.

Together with the previous commit, these changes improve throughput
in the adversarial case by about 100 %, and there is little more
that we can do besides artificially limiting the rate of incoming
metadata frames (i.e. after this patch, CPU usage is virtually
exclusively in libnghttp2).

PR-URL: https://github.com/nodejs/node/pull/29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node_http2.cc | 21 +++++++++++++++------
 src/node_http2.h  |  3 ++-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/node_http2.cc b/src/node_http2.cc
index 265d1ce3575c6d..3f24c3a25ee363 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1259,7 +1259,13 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
     return;
   }
 
-  CHECK(!session->stream_buf_ab_.IsEmpty());
+  Local<ArrayBuffer> ab;
+  if (session->stream_buf_ab_.IsEmpty()) {
+    ab = session->stream_buf_allocation_.ToArrayBuffer();
+    session->stream_buf_ab_.Reset(env->isolate(), ab);
+  } else {
+    ab = PersistentToLocal::Strong(session->stream_buf_ab_);
+  }
 
   // There is a single large array buffer for the entire data read from the
   // network; create a slice of that array buffer and emit it as the
@@ -1270,7 +1276,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   CHECK_LE(offset, session->stream_buf_.len);
   CHECK_LE(offset + buf.len, session->stream_buf_.len);
 
-  stream->CallJSOnreadMethod(nread, session->stream_buf_ab_, offset);
+  stream->CallJSOnreadMethod(nread, ab, offset);
 }
 
 
@@ -1789,6 +1795,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
   Http2Scope h2scope(this);
   CHECK_NOT_NULL(stream_);
   Debug(this, "receiving %d bytes", nread);
+  CHECK_EQ(stream_buf_allocation_.size(), 0);
   CHECK(stream_buf_ab_.IsEmpty());
   AllocatedBuffer buf(env(), buf_);
 
@@ -1810,7 +1817,8 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
     // We use `nread` instead of `buf.size()` here, because the buffer is
     // cleared as part of the `.ToArrayBuffer()` call below.
     DecrementCurrentSessionMemory(nread);
-    stream_buf_ab_ = Local<ArrayBuffer>();
+    stream_buf_ab_.Reset();
+    stream_buf_allocation_.clear();
     stream_buf_ = uv_buf_init(nullptr, 0);
   });
 
@@ -1824,9 +1832,10 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
 
   Isolate* isolate = env()->isolate();
 
-  // Create an array buffer for the read data. DATA frames will be emitted
-  // as slices of this array buffer to avoid having to copy memory.
-  stream_buf_ab_ = buf.ToArrayBuffer();
+  // Store this so we can create an ArrayBuffer for read data from it.
+  // DATA frames will be emitted as slices of that ArrayBuffer to avoid having
+  // to copy memory.
+  stream_buf_allocation_ = std::move(buf);
 
   statistics_.data_received += nread;
   ssize_t ret = Write(&stream_buf_, 1);
diff --git a/src/node_http2.h b/src/node_http2.h
index 9a156c8bfa883c..47d25f2b076ed0 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -993,7 +993,8 @@ class Http2Session : public AsyncWrap, public StreamListener {
   uint32_t chunks_sent_since_last_write_ = 0;
 
   uv_buf_t stream_buf_ = uv_buf_init(nullptr, 0);
-  v8::Local<v8::ArrayBuffer> stream_buf_ab_;
+  v8::Global<v8::ArrayBuffer> stream_buf_ab_;
+  AllocatedBuffer stream_buf_allocation_;
 
   size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
   std::queue<std::unique_ptr<Http2Ping>> outstanding_pings_;