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).

PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and targos committed Aug 15, 2019
1 parent 530004e commit 88726f2
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
21 changes: 15 additions & 6 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}


Expand Down Expand Up @@ -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_);

Expand All @@ -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);
});

Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down

0 comments on commit 88726f2

Please sign in to comment.