From 7f11465572888340b4c7b399c1f46598d1c4ea50 Mon Sep 17 00:00:00 2001 From: Anna Henningsen 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). [This backport also applies changes from 83e1b9744317 and required some manual work due to the lack of `AllocatedBuffer` on v10.x.] Refs: https://github.com/nodejs/node/pull/26201 Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/29122 Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/node_http2.cc | 57 ++++++++++++------- src/node_http2.h | 3 +- .../test-http2-max-session-memory.js | 2 +- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 6882c9d957c436..7d3b117f44107b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -650,6 +650,7 @@ Http2Session::~Http2Session() { stream.second->session_ = nullptr; nghttp2_session_del(session_); CHECK_EQ(current_nghttp2_memory_, 0); + free(stream_buf_allocation_.base); } std::string Http2Session::diagnostic_name() const { @@ -1259,7 +1260,17 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { return; } - CHECK(!session->stream_buf_ab_.IsEmpty()); + Local ab; + if (session->stream_buf_ab_.IsEmpty()) { + ab = ArrayBuffer::New(env->isolate(), + session->stream_buf_allocation_.base, + session->stream_buf_allocation_.len, + v8::ArrayBufferCreationMode::kInternalized); + session->stream_buf_allocation_ = uv_buf_init(nullptr, 0); + session->stream_buf_ab_.Reset(env->isolate(), ab); + } else { + ab = session->stream_buf_ab_.Get(env->isolate()); + } // 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 @@ -1271,7 +1282,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { CHECK_LE(offset + buf.len, session->stream_buf_.len); Local buffer = - Buffer::New(env, session->stream_buf_ab_, offset, nread).ToLocalChecked(); + Buffer::New(env, ab, offset, nread).ToLocalChecked(); stream->CallJSOnreadMethod(nread, buffer); } @@ -1803,32 +1814,41 @@ Http2Stream* Http2Session::SubmitRequest( } // Callback used to receive inbound data from the i/o stream -void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { +void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); Http2Scope h2scope(this); CHECK_NOT_NULL(stream_); Debug(this, "receiving %d bytes", nread); - IncrementCurrentSessionMemory(buf.len); + CHECK_EQ(stream_buf_allocation_.base, nullptr); CHECK(stream_buf_ab_.IsEmpty()); - OnScopeLeave on_scope_leave([&]() { - // Once finished handling this write, reset the stream buffer. - // The memory has either been free()d or was handed over to V8. - DecrementCurrentSessionMemory(buf.len); - stream_buf_ab_ = Local(); - stream_buf_ = uv_buf_init(nullptr, 0); - }); - // Only pass data on if nread > 0 if (nread <= 0) { - free(buf.base); + free(buf_.base); if (nread < 0) { PassReadErrorToPreviousListener(nread); } return; } + // Shrink to the actual amount of used data. + uv_buf_t buf = buf_; + buf.base = Realloc(buf.base, nread); + + IncrementCurrentSessionMemory(nread); + OnScopeLeave on_scope_leave([&]() { + // Once finished handling this write, reset the stream buffer. + // The memory has either been free()d or was handed over to V8. + // 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_.Reset(); + free(stream_buf_allocation_.base); + stream_buf_allocation_ = uv_buf_init(nullptr, 0); + stream_buf_ = uv_buf_init(nullptr, 0); + }); + // Make sure that there was no read previously active. CHECK_NULL(stream_buf_.base); CHECK_EQ(stream_buf_.len, 0); @@ -1845,13 +1865,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_ = - ArrayBuffer::New(isolate, - buf.base, - nread, - v8::ArrayBufferCreationMode::kInternalized); + // 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_ = 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 89c0b7d38de3ea..8424ffb89712e7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1005,7 +1005,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 stream_buf_ab_; + v8::Global stream_buf_ab_; + uv_buf_t stream_buf_allocation_ = uv_buf_init(nullptr, 0); size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue outstanding_pings_; diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js index 644a20a3c88a50..f770ee113945fc 100644 --- a/test/sequential/test-http2-max-session-memory.js +++ b/test/sequential/test-http2-max-session-memory.js @@ -8,7 +8,7 @@ const http2 = require('http2'); // Test that maxSessionMemory Caps work -const largeBuffer = Buffer.alloc(1e6); +const largeBuffer = Buffer.alloc(2e6); const server = http2.createServer({ maxSessionMemory: 1 });