Skip to content

Commit

Permalink
http2: do not create ArrayBuffers when no DATA received
Browse files Browse the repository at this point in the history
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 83e1b97 and required
some manual work due to the lack of `AllocatedBuffer` on v10.x.]

Refs: #26201

Backport-PR-URL: #29123
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and BethGriggs committed Aug 15, 2019
1 parent 2eb914f commit 7f11465
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 22 deletions.
57 changes: 37 additions & 20 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -1259,7 +1260,17 @@ 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 = 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
Expand All @@ -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<Object> buffer =
Buffer::New(env, session->stream_buf_ab_, offset, nread).ToLocalChecked();
Buffer::New(env, ab, offset, nread).ToLocalChecked();

stream->CallJSOnreadMethod(nread, buffer);
}
Expand Down Expand Up @@ -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<ArrayBuffer>();
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);
Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::ArrayBuffer> stream_buf_ab_;
v8::Global<v8::ArrayBuffer> stream_buf_ab_;
uv_buf_t stream_buf_allocation_ = uv_buf_init(nullptr, 0);

size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
std::queue<Http2Ping*> outstanding_pings_;
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-http2-max-session-memory.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });

Expand Down

0 comments on commit 7f11465

Please sign in to comment.