From 00f153da1352b941d8749758edb6083a6acda715 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 30 Oct 2018 16:17:33 -0700 Subject: [PATCH 01/19] http2: improve http2 code a bit Multiple general improvements to http2 internals for readability and efficiency [This backport applied to v10.x cleanly.] Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/23984 Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Ujjwal Sharma Reviewed-By: Matteo Collina --- benchmark/http2/headers.js | 3 +- benchmark/http2/respond-with-fd.js | 6 +- benchmark/http2/simple.js | 6 +- lib/internal/http2/util.js | 44 ++-- src/node_http2.cc | 347 +++++++++++++++-------------- 5 files changed, 212 insertions(+), 194 deletions(-) diff --git a/benchmark/http2/headers.js b/benchmark/http2/headers.js index ad1eb50007a92d..4e6585bbfd5f92 100644 --- a/benchmark/http2/headers.js +++ b/benchmark/http2/headers.js @@ -40,8 +40,7 @@ function main({ n, nheaders }) { function doRequest(remaining) { const req = client.request(headersObject); - req.end(); - req.on('data', () => {}); + req.resume(); req.on('end', () => { if (remaining > 0) { doRequest(remaining - 1); diff --git a/benchmark/http2/respond-with-fd.js b/benchmark/http2/respond-with-fd.js index cc9992e8ca0bd5..b8c6e81fd24b04 100644 --- a/benchmark/http2/respond-with-fd.js +++ b/benchmark/http2/respond-with-fd.js @@ -7,9 +7,9 @@ const fs = require('fs'); const file = path.join(path.resolve(__dirname, '../fixtures'), 'alice.html'); const bench = common.createBenchmark(main, { - requests: [100, 1000, 10000, 100000], - streams: [100, 200, 1000], - clients: [1, 2], + requests: [100, 1000, 5000], + streams: [1, 10, 20, 40, 100, 200], + clients: [2], benchmarker: ['h2load'] }, { flags: ['--no-warnings', '--expose-http2'] }); diff --git a/benchmark/http2/simple.js b/benchmark/http2/simple.js index cf017e6735411e..ce80dfd769c3c5 100644 --- a/benchmark/http2/simple.js +++ b/benchmark/http2/simple.js @@ -6,9 +6,9 @@ const fs = require('fs'); const file = path.join(path.resolve(__dirname, '../fixtures'), 'alice.html'); const bench = common.createBenchmark(main, { - requests: [100, 1000, 10000, 100000], - streams: [100, 200, 1000], - clients: [1, 2], + requests: [100, 1000, 5000], + streams: [1, 10, 20, 40, 100, 200], + clients: [2], benchmarker: ['h2load'] }, { flags: ['--no-warnings', '--expose-http2'] }); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 94dc1198ea1060..c8701af616f327 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -430,14 +430,20 @@ function mapToHeaders(map, let count = 0; const keys = Object.keys(map); const singles = new Set(); - for (var i = 0; i < keys.length; i++) { - let key = keys[i]; - let value = map[key]; + let i; + let isArray; + let key; + let value; + let isSingleValueHeader; + let err; + for (i = 0; i < keys.length; i++) { + key = keys[i]; + value = map[key]; if (value === undefined || key === '') continue; key = key.toLowerCase(); - const isSingleValueHeader = kSingleValueHeaders.has(key); - let isArray = Array.isArray(value); + isSingleValueHeader = kSingleValueHeaders.has(key); + isArray = Array.isArray(value); if (isArray) { switch (value.length) { case 0: @@ -459,26 +465,26 @@ function mapToHeaders(map, singles.add(key); } if (key[0] === ':') { - const err = assertValuePseudoHeader(key); + err = assertValuePseudoHeader(key); if (err !== undefined) return err; ret = `${key}\0${value}\0${ret}`; count++; - } else { - if (isIllegalConnectionSpecificHeader(key, value)) { - return new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key); - } - if (isArray) { - for (var k = 0; k < value.length; k++) { - const val = String(value[k]); - ret += `${key}\0${val}\0`; - } - count += value.length; - } else { - ret += `${key}\0${value}\0`; - count++; + continue; + } + if (isIllegalConnectionSpecificHeader(key, value)) { + return new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key); + } + if (isArray) { + for (var k = 0; k < value.length; k++) { + const val = String(value[k]); + ret += `${key}\0${val}\0`; } + count += value.length; + continue; } + ret += `${key}\0${value}\0`; + count++; } return [ret, count]; diff --git a/src/node_http2.cc b/src/node_http2.cc index 6bd282593e28f5..b87f80218eed23 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -913,8 +913,10 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, Debug(session, "beginning headers for stream %d", id); Http2Stream* stream = session->FindStream(id); - if (stream == nullptr) { - if (!session->CanAddStream()) { + // The common case is that we're creating a new stream. The less likely + // case is that we're receiving a set of trailers + if (LIKELY(stream == nullptr)) { + if (UNLIKELY(!session->CanAddStream())) { // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); @@ -942,7 +944,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, // If stream is null at this point, either something odd has happened // or the stream was closed locally while header processing was occurring. // either way, do not proceed and close the stream. - if (stream == nullptr) + if (UNLIKELY(stream == nullptr)) return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; // If the stream has already been destroyed, ignore. @@ -957,7 +959,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, // Called by nghttp2 when a complete HTTP2 frame has been received. There are -// only a handful of frame types tha we care about handling here. +// only a handful of frame types that we care about handling here. int Http2Session::OnFrameReceive(nghttp2_session* handle, const nghttp2_frame* frame, void* user_data) { @@ -1034,22 +1036,25 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, Environment* env = session->env(); Debug(session, "frame type %d was not sent, code: %d", frame->hd.type, error_code); - // Do not report if the frame was not sent due to the session closing - if (error_code != NGHTTP2_ERR_SESSION_CLOSING && - error_code != NGHTTP2_ERR_STREAM_CLOSED && - error_code != NGHTTP2_ERR_STREAM_CLOSING) { - Isolate* isolate = env->isolate(); - HandleScope scope(isolate); - Local context = env->context(); - Context::Scope context_scope(context); - Local argv[3] = { - Integer::New(isolate, frame->hd.stream_id), - Integer::New(isolate, frame->hd.type), - Integer::New(isolate, error_code) - }; - session->MakeCallback(env->onframeerror_string(), arraysize(argv), argv); + // Do not report if the frame was not sent due to the session closing + if (error_code == NGHTTP2_ERR_SESSION_CLOSING || + error_code == NGHTTP2_ERR_STREAM_CLOSED || + error_code == NGHTTP2_ERR_STREAM_CLOSING) { + return 0; } + + Isolate* isolate = env->isolate(); + HandleScope scope(isolate); + Local context = env->context(); + Context::Scope context_scope(context); + + Local argv[3] = { + Integer::New(isolate, frame->hd.stream_id), + Integer::New(isolate, frame->hd.type), + Integer::New(isolate, error_code) + }; + session->MakeCallback(env->onframeerror_string(), arraysize(argv), argv); return 0; } @@ -1076,25 +1081,26 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, Http2Stream* stream = session->FindStream(id); // Intentionally ignore the callback if the stream does not exist or has // already been destroyed - if (stream != nullptr && !stream->IsDestroyed()) { - stream->Close(code); - // It is possible for the stream close to occur before the stream is - // ever passed on to the javascript side. If that happens, skip straight - // to destroying the stream. We can check this by looking for the - // onstreamclose function. If it exists, then the stream has already - // been passed on to javascript. - Local fn = - stream->object()->Get(context, env->onstreamclose_string()) - .ToLocalChecked(); - if (fn->IsFunction()) { - Local argv[] = { - Integer::NewFromUnsigned(isolate, code) - }; - stream->MakeCallback(fn.As(), arraysize(argv), argv); - } else { - stream->Destroy(); - } + if (stream == nullptr || stream->IsDestroyed()) + return 0; + + stream->Close(code); + // It is possible for the stream close to occur before the stream is + // ever passed on to the javascript side. If that happens, skip straight + // to destroying the stream. We can check this by looking for the + // onstreamclose function. If it exists, then the stream has already + // been passed on to javascript. + Local fn = + stream->object()->Get(context, env->onstreamclose_string()) + .ToLocalChecked(); + + if (!fn->IsFunction()) { + stream->Destroy(); + return 0; } + + Local arg = Integer::NewFromUnsigned(isolate, code); + stream->MakeCallback(fn.As(), 1, &arg); return 0; } @@ -1127,53 +1133,56 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, "%d, flags: %d", id, len, flags); Environment* env = session->env(); HandleScope scope(env->isolate()); + // We should never actually get a 0-length chunk so this check is // only a precaution at this point. - if (len > 0) { - // Notify nghttp2 that we've consumed a chunk of data on the connection - // so that it can send a WINDOW_UPDATE frame. This is a critical part of - // the flow control process in http2 - CHECK_EQ(nghttp2_session_consume_connection(handle, len), 0); - Http2Stream* stream = session->FindStream(id); - // If the stream has been destroyed, ignore this chunk - if (stream->IsDestroyed()) - return 0; - - stream->statistics_.received_bytes += len; - - // Repeatedly ask the stream's owner for memory, and copy the read data - // into those buffers. - // The typical case is actually the exception here; Http2StreamListeners - // know about the HTTP2 session associated with this stream, so they know - // about the larger from-socket read buffer, so they do not require copying. - do { - uv_buf_t buf = stream->EmitAlloc(len); - ssize_t avail = len; - if (static_cast(buf.len) < avail) - avail = buf.len; - - // `buf.base == nullptr` is the default Http2StreamListener's way - // of saying that it wants a pointer to the raw original. - // Since it has access to the original socket buffer from which the data - // was read in the first place, it can use that to minimize ArrayBuffer - // allocations. - if (LIKELY(buf.base == nullptr)) - buf.base = reinterpret_cast(const_cast(data)); - else - memcpy(buf.base, data, avail); - data += avail; - len -= avail; - stream->EmitRead(avail, buf); - - // If the stream owner (e.g. the JS Http2Stream) wants more data, just - // tell nghttp2 that all data has been consumed. Otherwise, defer until - // more data is being requested. - if (stream->IsReading()) - nghttp2_session_consume_stream(handle, id, avail); - else - stream->inbound_consumed_data_while_paused_ += avail; - } while (len != 0); - } + if (len == 0) + return 0; + + // Notify nghttp2 that we've consumed a chunk of data on the connection + // so that it can send a WINDOW_UPDATE frame. This is a critical part of + // the flow control process in http2 + CHECK_EQ(nghttp2_session_consume_connection(handle, len), 0); + Http2Stream* stream = session->FindStream(id); + // If the stream has been destroyed, ignore this chunk + if (stream->IsDestroyed()) + return 0; + + stream->statistics_.received_bytes += len; + + // Repeatedly ask the stream's owner for memory, and copy the read data + // into those buffers. + // The typical case is actually the exception here; Http2StreamListeners + // know about the HTTP2 session associated with this stream, so they know + // about the larger from-socket read buffer, so they do not require copying. + do { + uv_buf_t buf = stream->EmitAlloc(len); + ssize_t avail = len; + if (static_cast(buf.len) < avail) + avail = buf.len; + + // `buf.base == nullptr` is the default Http2StreamListener's way + // of saying that it wants a pointer to the raw original. + // Since it has access to the original socket buffer from which the data + // was read in the first place, it can use that to minimize ArrayBuffer + // allocations. + if (LIKELY(buf.base == nullptr)) + buf.base = reinterpret_cast(const_cast(data)); + else + memcpy(buf.base, data, avail); + data += avail; + len -= avail; + stream->EmitRead(avail, buf); + + // If the stream owner (e.g. the JS Http2Stream) wants more data, just + // tell nghttp2 that all data has been consumed. Otherwise, defer until + // more data is being requested. + if (stream->IsReading()) + nghttp2_session_consume_stream(handle, id, avail); + else + stream->inbound_consumed_data_while_paused_ += avail; + } while (len != 0); + return 0; } @@ -1435,7 +1444,7 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) { nghttp2_extension ext = frame->ext; nghttp2_ext_origin* origin = static_cast(ext.payload); - Local holder = Array::New(isolate); + Local holder = Array::New(isolate); Local fn = env()->push_values_to_array_function(); Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX]; @@ -1454,9 +1463,7 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) { fn->Call(context, holder, j, argv).ToLocalChecked(); } - Local args[1] = { holder }; - - MakeCallback(env()->onorigin_string(), arraysize(args), args); + MakeCallback(env()->onorigin_string(), 1, &holder); } // Called by OnFrameReceived when a complete PING frame has been received. @@ -1469,9 +1476,8 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { Http2Ping* ping = PopPing(); - if (ping != nullptr) { - ping->Done(true, frame->ping.opaque_data); - } else { + + if (ping == nullptr) { // PING Ack is unsolicited. Treat as a connection error. The HTTP/2 // spec does not require this, but there is no legitimate reason to // receive an unsolicited PING ack on a connection. Either the peer @@ -1479,46 +1485,51 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { // nonsense. arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); MakeCallback(env()->error_string(), 1, &arg); + return; } - } else { - // Notify the session that a ping occurred - arg = Buffer::Copy(env(), - reinterpret_cast(frame->ping.opaque_data), - 8).ToLocalChecked(); - MakeCallback(env()->onping_string(), 1, &arg); + + ping->Done(true, frame->ping.opaque_data); + return; } + + // Notify the session that a ping occurred + arg = Buffer::Copy(env(), + reinterpret_cast(frame->ping.opaque_data), + 8).ToLocalChecked(); + MakeCallback(env()->onping_string(), 1, &arg); } // Called by OnFrameReceived when a complete SETTINGS frame has been received. void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; - if (ack) { - // If this is an acknowledgement, we should have an Http2Settings - // object for it. - Http2Settings* settings = PopSettings(); - if (settings != nullptr) { - settings->Done(true); - } else { - // SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2 - // spec does not require this, but there is no legitimate reason to - // receive an unsolicited SETTINGS ack on a connection. Either the peer - // is buggy or malicious, and we're not going to tolerate such - // nonsense. - // Note that nghttp2 currently prevents this from happening for SETTINGS - // frames, so this block is purely defensive just in case that behavior - // changes. Specifically, unlike unsolicited PING acks, unsolicited - // SETTINGS acks should *never* make it this far. - Isolate* isolate = env()->isolate(); - HandleScope scope(isolate); - Local context = env()->context(); - Context::Scope context_scope(context); - Local arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); - MakeCallback(env()->error_string(), 1, &arg); - } - } else { - // Otherwise, notify the session about a new settings + if (!ack) { + // This is not a SETTINGS acknowledgement, notify and return MakeCallback(env()->onsettings_string(), 0, nullptr); + return; } + + // If this is an acknowledgement, we should have an Http2Settings + // object for it. + Http2Settings* settings = PopSettings(); + if (settings != nullptr) { + settings->Done(true); + return; + } + // SETTINGS Ack is unsolicited. Treat as a connection error. The HTTP/2 + // spec does not require this, but there is no legitimate reason to + // receive an unsolicited SETTINGS ack on a connection. Either the peer + // is buggy or malicious, and we're not going to tolerate such + // nonsense. + // Note that nghttp2 currently prevents this from happening for SETTINGS + // frames, so this block is purely defensive just in case that behavior + // changes. Specifically, unlike unsolicited PING acks, unsolicited + // SETTINGS acks should *never* make it this far. + Isolate* isolate = env()->isolate(); + HandleScope scope(isolate); + Local context = env()->context(); + Context::Scope context_scope(context); + Local arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); + MakeCallback(env()->error_string(), 1, &arg); } // Callback used when data has been written to the stream. @@ -1540,7 +1551,10 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { // queue), but only if a write has not already been scheduled. void Http2Session::MaybeScheduleWrite() { CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0); - if (session_ != nullptr && nghttp2_session_want_write(session_)) { + if (UNLIKELY(session_ == nullptr)) + return; + + if (nghttp2_session_want_write(session_)) { HandleScope handle_scope(env()->isolate()); Debug(this, "scheduling write"); flags_ |= SESSION_STATE_WRITE_SCHEDULED; @@ -1599,7 +1613,7 @@ void Http2Session::ClearOutgoing(int status) { for (int32_t stream_id : current_pending_rst_streams) { Http2Stream* stream = FindStream(stream_id); - if (stream != nullptr) + if (LIKELY(stream != nullptr)) stream->FlushRstStream(); } } @@ -1774,7 +1788,7 @@ Http2Stream* Http2Session::SubmitRequest( Http2Stream::Provider::Stream prov(options); *ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); - if (*ret > 0) + if (LIKELY(*ret > 0)) stream = new Http2Stream(this, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -1789,59 +1803,58 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { IncrementCurrentSessionMemory(buf.len); 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); if (nread < 0) { PassReadErrorToPreviousListener(nread); } - } else { - // Only pass data on if nread > 0 - - // Makre sure that there was no read previously active. - CHECK_NULL(stream_buf_.base); - CHECK_EQ(stream_buf_.len, 0); - - // Remember the current buffer, so that OnDataChunkReceived knows the - // offset of a DATA frame's data into the socket read buffer. - stream_buf_ = uv_buf_init(buf.base, nread); - - // Verify that currently: There is memory allocated into which - // the data has been read, and that memory buffer is at least as large - // as the amount of data we have read, but we have not yet made an - // ArrayBuffer out of it. - CHECK_LE(static_cast(nread), stream_buf_.len); - - 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); - - statistics_.data_received += nread; - ssize_t ret = Write(&stream_buf_, 1); - - if (ret < 0) { - Debug(this, "fatal error receiving data: %d", ret); - - Local argv[] = { - Integer::New(isolate, ret), - }; - MakeCallback(env()->error_string(), arraysize(argv), argv); - } else { - MaybeStopReading(); - } + return; } - // Since we are finished handling this write, reset the stream buffer. - // The memory has either been free()d or was handed over to V8. - DecrementCurrentSessionMemory(buf.len); + // Make sure that there was no read previously active. + CHECK_NULL(stream_buf_.base); + CHECK_EQ(stream_buf_.len, 0); + + // Remember the current buffer, so that OnDataChunkReceived knows the + // offset of a DATA frame's data into the socket read buffer. + stream_buf_ = uv_buf_init(buf.base, nread); + + // Verify that currently: There is memory allocated into which + // the data has been read, and that memory buffer is at least as large + // as the amount of data we have read, but we have not yet made an + // ArrayBuffer out of it. + CHECK_LE(static_cast(nread), stream_buf_.len); + + Isolate* isolate = env()->isolate(); - stream_buf_ab_ = Local(); - stream_buf_ = uv_buf_init(nullptr, 0); + // 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); + + statistics_.data_received += nread; + ssize_t ret = Write(&stream_buf_, 1); + + if (UNLIKELY(ret < 0)) { + Debug(this, "fatal error receiving data: %d", ret); + Local arg = Integer::New(isolate, ret); + MakeCallback(env()->error_string(), 1, &arg); + return; + } + + MaybeStopReading(); } bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) { From a0a14c809ffe59146fa16284c2f3f84e3f8f02c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 18:43:05 +0100 Subject: [PATCH 02/19] src: pass along errors from http2 object creation [This backport applied to v10.x cleanly.] Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/25822 Reviewed-By: Gireesh Punathil --- src/node_http2.cc | 125 ++++++++++++++++++++++++++-------------------- src/node_http2.h | 24 ++++++--- 2 files changed, 87 insertions(+), 62 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index b87f80218eed23..c7ccaf78690f09 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -228,27 +228,18 @@ void Http2Session::Http2Settings::Init() { count_ = n; } -Http2Session::Http2Settings::Http2Settings(Environment* env, - Http2Session* session, uint64_t start_time) - : AsyncWrap(env, - env->http2settings_constructor_template() - ->NewInstance(env->context()) - .ToLocalChecked(), - PROVIDER_HTTP2SETTINGS), - session_(session), - startTime_(start_time) { - Init(); -} - - -Http2Session::Http2Settings::Http2Settings(Environment* env) - : Http2Settings(env, nullptr, 0) {} - // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray // that is shared with the JavaScript side. -Http2Session::Http2Settings::Http2Settings(Http2Session* session) - : Http2Settings(session->env(), session, uv_hrtime()) {} +Http2Session::Http2Settings::Http2Settings(Environment* env, + Http2Session* session, + Local obj, + uint64_t start_time) + : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), + session_(session), + startTime_(start_time) { + Init(); +} // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded @@ -916,13 +907,14 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, // The common case is that we're creating a new stream. The less likely // case is that we're receiving a set of trailers if (LIKELY(stream == nullptr)) { - if (UNLIKELY(!session->CanAddStream())) { + if (UNLIKELY(!session->CanAddStream() || + Http2Stream::New(session, id, frame->headers.cat) == + nullptr)) { // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - new Http2Stream(session, id, frame->headers.cat); } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } @@ -1789,7 +1781,7 @@ Http2Stream* Http2Session::SubmitRequest( *ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); if (LIKELY(*ret > 0)) - stream = new Http2Stream(this, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(this, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -1875,20 +1867,30 @@ void Http2Session::Consume(Local external) { Debug(this, "i/o stream consumed"); } - -Http2Stream::Http2Stream( - Http2Session* session, - int32_t id, - nghttp2_headers_category category, - int options) : AsyncWrap(session->env(), - session->env()->http2stream_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2STREAM), - StreamBase(session->env()), - session_(session), - id_(id), - current_headers_category_(category) { +Http2Stream* Http2Stream::New(Http2Session* session, + int32_t id, + nghttp2_headers_category category, + int options) { + Local obj; + if (!session->env() + ->http2stream_constructor_template() + ->NewInstance(session->env()->context()) + .ToLocal(&obj)) { + return nullptr; + } + return new Http2Stream(session, obj, id, category, options); +} + +Http2Stream::Http2Stream(Http2Session* session, + Local obj, + int32_t id, + nghttp2_headers_category category, + int options) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2STREAM), + StreamBase(session->env()), + session_(session), + id_(id), + current_headers_category_(category) { MakeWeak(); statistics_.start_time = uv_hrtime(); @@ -2132,7 +2134,7 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); Http2Stream* stream = nullptr; if (*ret > 0) - stream = new Http2Stream(session_, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -2354,7 +2356,14 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Http2Session::Http2Settings settings(env); + // TODO(addaleax): We should not be creating a full AsyncWrap for this. + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + Http2Session::Http2Settings settings(env, nullptr, obj); args.GetReturnValue().Set(settings.Pack()); } @@ -2483,7 +2492,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { session->Http2Session::SubmitRequest(*priority, *list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(session, "could not submit request: %s", nghttp2_strerror(ret)); return args.GetReturnValue().Set(ret); } @@ -2656,7 +2665,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { int32_t ret = 0; Http2Stream* stream = parent->SubmitPushPromise(*list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(parent, "failed to create push stream: %d", ret); return args.GetReturnValue().Set(ret); } @@ -2792,9 +2801,15 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { CHECK_EQ(Buffer::Length(args[0]), 8); } - Http2Session::Http2Ping* ping = new Http2Ping(session); - Local obj = ping->object(); - obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); + Local obj; + if (!env->http2ping_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) + return; + Http2Session::Http2Ping* ping = new Http2Ping(session, obj); // To prevent abuse, we strictly limit the number of unacknowledged PING // frames that may be sent at any given time. This is configurable in the @@ -2818,10 +2833,17 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - Http2Session::Http2Settings* settings = new Http2Settings(session); - Local obj = settings->object(); - obj->Set(env->context(), env->ondone_string(), args[0]).FromJust(); + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) + return; + Http2Session::Http2Settings* settings = + new Http2Settings(session->env(), session, obj, 0); if (!session->AddSettings(settings)) { settings->Done(false); return args.GetReturnValue().Set(false); @@ -2868,15 +2890,10 @@ bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) { return true; } -Http2Session::Http2Ping::Http2Ping( - Http2Session* session) - : AsyncWrap(session->env(), - session->env()->http2ping_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2PING), - session_(session), - startTime_(uv_hrtime()) { } +Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), + session_(session), + startTime_(uv_hrtime()) {} void Http2Session::Http2Ping::Send(uint8_t* payload) { uint8_t data[8]; diff --git a/src/node_http2.h b/src/node_http2.h index a1490fa4cb7c4b..91c0673d295910 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -444,10 +444,11 @@ class Http2StreamListener : public StreamListener { class Http2Stream : public AsyncWrap, public StreamBase { public: - Http2Stream(Http2Session* session, - int32_t id, - nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - int options = 0); + static Http2Stream* New( + Http2Session* session, + int32_t id, + nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, + int options = 0); ~Http2Stream() override; nghttp2_stream* operator*(); @@ -604,6 +605,12 @@ class Http2Stream : public AsyncWrap, Statistics statistics_ = {}; private: + Http2Stream(Http2Session* session, + v8::Local obj, + int32_t id, + nghttp2_headers_category category, + int options); + Http2Session* session_ = nullptr; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) @@ -1080,7 +1087,7 @@ class Http2StreamPerformanceEntry : public PerformanceEntry { class Http2Session::Http2Ping : public AsyncWrap { public: - explicit Http2Ping(Http2Session* session); + explicit Http2Ping(Http2Session* session, v8::Local obj); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1104,8 +1111,10 @@ class Http2Session::Http2Ping : public AsyncWrap { // structs. class Http2Session::Http2Settings : public AsyncWrap { public: - explicit Http2Settings(Environment* env); - explicit Http2Settings(Http2Session* session); + Http2Settings(Environment* env, + Http2Session* session, + v8::Local obj, + uint64_t start_time = uv_hrtime()); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1129,7 +1138,6 @@ class Http2Session::Http2Settings : public AsyncWrap { get_setting fn); private: - Http2Settings(Environment* env, Http2Session* session, uint64_t start_time); void Init(); Http2Session* session_; uint64_t startTime_; From ab0f2ace36a116f1e905c17e61904cc48abd76c2 Mon Sep 17 00:00:00 2001 From: gengjiawen Date: Sat, 30 Mar 2019 00:22:54 +0800 Subject: [PATCH 03/19] deps: update nghttp2 to 1.37.0 Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/26990 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- deps/nghttp2/lib/CMakeLists.txt | 25 +++++++------ deps/nghttp2/lib/includes/nghttp2/nghttp2.h | 8 ++++- .../nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 +-- deps/nghttp2/lib/nghttp2_hd.c | 2 +- deps/nghttp2/lib/nghttp2_session.h | 3 -- deps/nghttp2/lib/nghttp2_stream.c | 35 +++++++++---------- deps/nghttp2/lib/nghttp2_stream.h | 4 +-- 7 files changed, 43 insertions(+), 38 deletions(-) diff --git a/deps/nghttp2/lib/CMakeLists.txt b/deps/nghttp2/lib/CMakeLists.txt index 17e422b22db790..c27ee99bb7fa46 100644 --- a/deps/nghttp2/lib/CMakeLists.txt +++ b/deps/nghttp2/lib/CMakeLists.txt @@ -38,16 +38,23 @@ if(WIN32) endif() # Public shared library -add_library(nghttp2 SHARED ${NGHTTP2_SOURCES} ${NGHTTP2_RES}) -set_target_properties(nghttp2 PROPERTIES - COMPILE_FLAGS "${WARNCFLAGS}" - VERSION ${LT_VERSION} SOVERSION ${LT_SOVERSION} - C_VISIBILITY_PRESET hidden -) -target_include_directories(nghttp2 INTERFACE +if(ENABLE_SHARED_LIB) + add_library(nghttp2 SHARED ${NGHTTP2_SOURCES} ${NGHTTP2_RES}) + set_target_properties(nghttp2 PROPERTIES + COMPILE_FLAGS "${WARNCFLAGS}" + VERSION ${LT_VERSION} SOVERSION ${LT_SOVERSION} + C_VISIBILITY_PRESET hidden + ) + target_include_directories(nghttp2 INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/includes" "${CMAKE_CURRENT_SOURCE_DIR}/includes" - ) + ) + + install(TARGETS nghttp2 + ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}" + LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}" + RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}") +endif() if(HAVE_CUNIT OR ENABLE_STATIC_LIB) # Static library (for unittests because of symbol visibility) @@ -64,8 +71,6 @@ if(HAVE_CUNIT OR ENABLE_STATIC_LIB) endif() endif() -install(TARGETS nghttp2 - DESTINATION "${CMAKE_INSTALL_LIBDIR}") install(FILES "${CMAKE_CURRENT_BINARY_DIR}/libnghttp2.pc" DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig") diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h index e7198b3d27314d..925a4cbcaf6d6a 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h @@ -31,6 +31,11 @@ # define WIN32 #endif +/* Compatibility for non-Clang compilers */ +#ifndef __has_declspec_attribute +# define __has_declspec_attribute(x) 0 +#endif + #ifdef __cplusplus extern "C" { #endif @@ -51,7 +56,8 @@ extern "C" { #ifdef NGHTTP2_STATICLIB # define NGHTTP2_EXTERN -#elif defined(WIN32) +#elif defined(WIN32) || (__has_declspec_attribute(dllexport) && \ + __has_declspec_attribute(dllimport)) # ifdef BUILDING_NGHTTP2 # define NGHTTP2_EXTERN __declspec(dllexport) # else /* !BUILDING_NGHTTP2 */ diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 420adbd53dddce..2384f5278773cb 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.34.0" +#define NGHTTP2_VERSION "1.37.0" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012200 +#define NGHTTP2_VERSION_NUM 0x012500 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_hd.c b/deps/nghttp2/lib/nghttp2_hd.c index a61f0d47a6f79d..11ca3345f3c6b3 100644 --- a/deps/nghttp2/lib/nghttp2_hd.c +++ b/deps/nghttp2/lib/nghttp2_hd.c @@ -1390,7 +1390,7 @@ static int deflate_nv(nghttp2_hd_deflater *deflater, nghttp2_bufs *bufs, if (indexing_mode == NGHTTP2_HD_WITH_INDEXING) { nghttp2_hd_nv hd_nv; - if (idx != -1 && idx < (ssize_t)NGHTTP2_STATIC_TABLE_LENGTH) { + if (idx != -1) { hd_nv.name = nghttp2_hd_table_get(&deflater->ctx, (size_t)idx).name; nghttp2_rcbuf_incref(hd_nv.name); } else { diff --git a/deps/nghttp2/lib/nghttp2_session.h b/deps/nghttp2/lib/nghttp2_session.h index 40a8865a04b12f..bd7a5b35817599 100644 --- a/deps/nghttp2/lib/nghttp2_session.h +++ b/deps/nghttp2/lib/nghttp2_session.h @@ -209,9 +209,6 @@ struct nghttp2_session { nghttp2_session_callbacks callbacks; /* Memory allocator */ nghttp2_mem mem; - /* Base value when we schedule next DATA frame write. This is - updated when one frame was written. */ - uint64_t last_cycle; void *user_data; /* Points to the latest incoming closed stream. NULL if there is no closed stream. Only used when session is initialized as diff --git a/deps/nghttp2/lib/nghttp2_stream.c b/deps/nghttp2/lib/nghttp2_stream.c index eccd3174ef7bda..dc3a6b11ccbf75 100644 --- a/deps/nghttp2/lib/nghttp2_stream.c +++ b/deps/nghttp2/lib/nghttp2_stream.c @@ -30,6 +30,7 @@ #include "nghttp2_session.h" #include "nghttp2_helper.h" #include "nghttp2_debug.h" +#include "nghttp2_frame.h" /* Maximum distance between any two stream's cycle in the same prirority queue. Imagine stream A's cycle is A, and stream B's @@ -40,7 +41,8 @@ words, B is really greater than or equal to A. Otherwise, A is a result of overflow, and it is actually A > B if we consider that fact. */ -#define NGHTTP2_MAX_CYCLE_DISTANCE (16384 * 256 + 255) +#define NGHTTP2_MAX_CYCLE_DISTANCE \ + ((uint64_t)NGHTTP2_MAX_FRAME_SIZE_MAX * 256 + 255) static int stream_less(const void *lhsx, const void *rhsx) { const nghttp2_stream *lhs, *rhs; @@ -52,11 +54,7 @@ static int stream_less(const void *lhsx, const void *rhsx) { return lhs->seq < rhs->seq; } - if (lhs->cycle < rhs->cycle) { - return rhs->cycle - lhs->cycle <= NGHTTP2_MAX_CYCLE_DISTANCE; - } - - return lhs->cycle - rhs->cycle > NGHTTP2_MAX_CYCLE_DISTANCE; + return rhs->cycle - lhs->cycle <= NGHTTP2_MAX_CYCLE_DISTANCE; } void nghttp2_stream_init(nghttp2_stream *stream, int32_t stream_id, @@ -135,14 +133,14 @@ static int stream_subtree_active(nghttp2_stream *stream) { /* * Returns next cycle for |stream|. */ -static void stream_next_cycle(nghttp2_stream *stream, uint32_t last_cycle) { - uint32_t penalty; +static void stream_next_cycle(nghttp2_stream *stream, uint64_t last_cycle) { + uint64_t penalty; - penalty = (uint32_t)stream->last_writelen * NGHTTP2_MAX_WEIGHT + + penalty = (uint64_t)stream->last_writelen * NGHTTP2_MAX_WEIGHT + stream->pending_penalty; stream->cycle = last_cycle + penalty / (uint32_t)stream->weight; - stream->pending_penalty = penalty % (uint32_t)stream->weight; + stream->pending_penalty = (uint32_t)(penalty % (uint32_t)stream->weight); } static int stream_obq_push(nghttp2_stream *dep_stream, nghttp2_stream *stream) { @@ -153,7 +151,7 @@ static int stream_obq_push(nghttp2_stream *dep_stream, nghttp2_stream *stream) { stream_next_cycle(stream, dep_stream->descendant_last_cycle); stream->seq = dep_stream->descendant_next_seq++; - DEBUGF("stream: stream=%d obq push cycle=%d\n", stream->stream_id, + DEBUGF("stream: stream=%d obq push cycle=%lu\n", stream->stream_id, stream->cycle); DEBUGF("stream: push stream %d to stream %d\n", stream->stream_id, @@ -239,7 +237,7 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); - DEBUGF("stream: stream=%d obq resched cycle=%d\n", stream->stream_id, + DEBUGF("stream: stream=%d obq resched cycle=%lu\n", stream->stream_id, stream->cycle); dep_stream->last_writelen = stream->last_writelen; @@ -248,9 +246,9 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { nghttp2_stream *dep_stream; - uint32_t last_cycle; + uint64_t last_cycle; int32_t old_weight; - uint32_t wlen_penalty; + uint64_t wlen_penalty; if (stream->weight == weight) { return; @@ -273,7 +271,7 @@ void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); - wlen_penalty = (uint32_t)stream->last_writelen * NGHTTP2_MAX_WEIGHT; + wlen_penalty = (uint64_t)stream->last_writelen * NGHTTP2_MAX_WEIGHT; /* Compute old stream->pending_penalty we used to calculate stream->cycle */ @@ -289,9 +287,8 @@ void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { place */ stream_next_cycle(stream, last_cycle); - if (stream->cycle < dep_stream->descendant_last_cycle && - (dep_stream->descendant_last_cycle - stream->cycle) <= - NGHTTP2_MAX_CYCLE_DISTANCE) { + if (dep_stream->descendant_last_cycle - stream->cycle <= + NGHTTP2_MAX_CYCLE_DISTANCE) { stream->cycle = dep_stream->descendant_last_cycle; } @@ -299,7 +296,7 @@ void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); - DEBUGF("stream: stream=%d obq resched cycle=%d\n", stream->stream_id, + DEBUGF("stream: stream=%d obq resched cycle=%lu\n", stream->stream_id, stream->cycle); } diff --git a/deps/nghttp2/lib/nghttp2_stream.h b/deps/nghttp2/lib/nghttp2_stream.h index fb8dc14d67be6d..a1b807d295c05c 100644 --- a/deps/nghttp2/lib/nghttp2_stream.h +++ b/deps/nghttp2/lib/nghttp2_stream.h @@ -148,9 +148,9 @@ struct nghttp2_stream { /* Received body so far */ int64_t recv_content_length; /* Base last_cycle for direct descendent streams. */ - uint32_t descendant_last_cycle; + uint64_t descendant_last_cycle; /* Next scheduled time to sent item */ - uint32_t cycle; + uint64_t cycle; /* Next seq used for direct descendant streams */ uint64_t descendant_next_seq; /* Secondary key for prioritization to break a tie for cycle. This From fedfa12a33bc81fec62ba6a6e0bba0b788ace38a Mon Sep 17 00:00:00 2001 From: gengjiawen Date: Thu, 18 Apr 2019 21:26:14 +0800 Subject: [PATCH 04/19] deps: update nghttp2 to 1.38.0 Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/27295 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Yongsheng Zhang Reviewed-By: Masashi Hirano --- .../nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 +- deps/nghttp2/lib/nghttp2_session.c | 98 ++++++++++--------- 2 files changed, 52 insertions(+), 50 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 2384f5278773cb..7ddf43e7030ee6 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.37.0" +#define NGHTTP2_VERSION "1.38.0" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012500 +#define NGHTTP2_VERSION_NUM 0x012600 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index ef4932af4e4f58..33d987667edaf5 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -3619,71 +3619,73 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame, if (call_header_cb && (inflate_flags & NGHTTP2_HD_INFLATE_EMIT)) { rv = 0; - if (subject_stream && session_enforce_http_messaging(session)) { - rv = nghttp2_http_on_header(session, subject_stream, frame, &nv, - trailer); + if (subject_stream) { + if (session_enforce_http_messaging(session)) { + rv = nghttp2_http_on_header(session, subject_stream, frame, &nv, + trailer); - if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) { - /* Don't overwrite rv here */ - int rv2; + if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) { + /* Don't overwrite rv here */ + int rv2; - rv2 = session_call_on_invalid_header(session, frame, &nv); - if (rv2 == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { - rv = NGHTTP2_ERR_HTTP_HEADER; - } else { - if (rv2 != 0) { - return rv2; + rv2 = session_call_on_invalid_header(session, frame, &nv); + if (rv2 == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + rv = NGHTTP2_ERR_HTTP_HEADER; + } else { + if (rv2 != 0) { + return rv2; + } + + /* header is ignored */ + DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n", + frame->hd.type, frame->hd.stream_id, (int)nv.name->len, + nv.name->base, (int)nv.value->len, nv.value->base); + + rv2 = session_call_error_callback( + session, NGHTTP2_ERR_HTTP_HEADER, + "Ignoring received invalid HTTP header field: frame type: " + "%u, stream: %d, name: [%.*s], value: [%.*s]", + frame->hd.type, frame->hd.stream_id, (int)nv.name->len, + nv.name->base, (int)nv.value->len, nv.value->base); + + if (nghttp2_is_fatal(rv2)) { + return rv2; + } } + } - /* header is ignored */ - DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n", + if (rv == NGHTTP2_ERR_HTTP_HEADER) { + DEBUGF("recv: HTTP error: type=%u, id=%d, header %.*s: %.*s\n", frame->hd.type, frame->hd.stream_id, (int)nv.name->len, nv.name->base, (int)nv.value->len, nv.value->base); - rv2 = session_call_error_callback( + rv = session_call_error_callback( session, NGHTTP2_ERR_HTTP_HEADER, - "Ignoring received invalid HTTP header field: frame type: " + "Invalid HTTP header field was received: frame type: " "%u, stream: %d, name: [%.*s], value: [%.*s]", frame->hd.type, frame->hd.stream_id, (int)nv.name->len, nv.name->base, (int)nv.value->len, nv.value->base); - if (nghttp2_is_fatal(rv2)) { - return rv2; + if (nghttp2_is_fatal(rv)) { + return rv; } - } - } - - if (rv == NGHTTP2_ERR_HTTP_HEADER) { - DEBUGF("recv: HTTP error: type=%u, id=%d, header %.*s: %.*s\n", - frame->hd.type, frame->hd.stream_id, (int)nv.name->len, - nv.name->base, (int)nv.value->len, nv.value->base); - rv = session_call_error_callback( - session, NGHTTP2_ERR_HTTP_HEADER, - "Invalid HTTP header field was received: frame type: " - "%u, stream: %d, name: [%.*s], value: [%.*s]", - frame->hd.type, frame->hd.stream_id, (int)nv.name->len, - nv.name->base, (int)nv.value->len, nv.value->base); - - if (nghttp2_is_fatal(rv)) { - return rv; + rv = session_handle_invalid_stream2(session, + subject_stream->stream_id, + frame, NGHTTP2_ERR_HTTP_HEADER); + if (nghttp2_is_fatal(rv)) { + return rv; + } + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - - rv = - session_handle_invalid_stream2(session, subject_stream->stream_id, - frame, NGHTTP2_ERR_HTTP_HEADER); - if (nghttp2_is_fatal(rv)) { + } + if (rv == 0) { + rv = session_call_on_header(session, frame, &nv); + /* This handles NGHTTP2_ERR_PAUSE and + NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */ + if (rv != 0) { return rv; } - return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; - } - } - if (rv == 0) { - rv = session_call_on_header(session, frame, &nv); - /* This handles NGHTTP2_ERR_PAUSE and - NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */ - if (rv != 0) { - return rv; } } } From a397c881ecb13a47663ccbbfca4762dc044ee4d6 Mon Sep 17 00:00:00 2001 From: gengjiawen Date: Thu, 27 Jun 2019 21:42:36 +0800 Subject: [PATCH 05/19] deps: update nghttp2 to 1.39.1 Backport-PR-URL: https://github.com/nodejs/node/pull/29123 PR-URL: https://github.com/nodejs/node/pull/28448 Reviewed-By: Trivikram Kamat Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 ++-- deps/nghttp2/lib/nghttp2_http.c | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 7ddf43e7030ee6..210cfaa16ae816 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.38.0" +#define NGHTTP2_VERSION "1.39.1" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012600 +#define NGHTTP2_VERSION_NUM 0x012701 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_http.c b/deps/nghttp2/lib/nghttp2_http.c index 6e8acfdcc141af..8d990299838193 100644 --- a/deps/nghttp2/lib/nghttp2_http.c +++ b/deps/nghttp2/lib/nghttp2_http.c @@ -263,11 +263,14 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_hd_nv *nv, stream->content_length = 0; return NGHTTP2_ERR_REMOVE_HTTP_HEADER; } - if (stream->status_code / 100 == 1 || - (stream->status_code / 100 == 2 && - (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT))) { + if (stream->status_code / 100 == 1) { return NGHTTP2_ERR_HTTP_HEADER; } + /* https://tools.ietf.org/html/rfc7230#section-3.3.3 */ + if (stream->status_code / 100 == 2 && + (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT)) { + return NGHTTP2_ERR_REMOVE_HTTP_HEADER; + } if (stream->content_length != -1) { return NGHTTP2_ERR_HTTP_HEADER; } From 74507fae34deae8a0011b6bebf81a450de4ab8a9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 14 Aug 2019 11:32:06 +0200 Subject: [PATCH 06/19] deps: update nghttp2 to 1.39.2 This includes mitigations for CVE-2019-9512/CVE-2019-9515. 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 --- deps/nghttp2/lib/includes/nghttp2/nghttp2.h | 11 +++++++++++ deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 ++-- deps/nghttp2/lib/nghttp2_option.c | 5 +++++ deps/nghttp2/lib/nghttp2_option.h | 5 +++++ deps/nghttp2/lib/nghttp2_session.c | 9 +++++++-- deps/nghttp2/lib/nghttp2_session.h | 8 ++++++-- 6 files changed, 36 insertions(+), 6 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h index 925a4cbcaf6d6a..313fb23daa7449 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h @@ -2648,6 +2648,17 @@ nghttp2_option_set_max_deflate_dynamic_table_size(nghttp2_option *option, NGHTTP2_EXTERN void nghttp2_option_set_no_closed_streams(nghttp2_option *option, int val); +/** + * @function + * + * This function sets the maximum number of outgoing SETTINGS ACK and + * PING ACK frames retained in :type:`nghttp2_session` object. If + * more than those frames are retained, the peer is considered to be + * misbehaving and session will be closed. The default value is 1000. + */ +NGHTTP2_EXTERN void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, + size_t val); + /** * @function * diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 210cfaa16ae816..45bb0c9102cb05 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.39.1" +#define NGHTTP2_VERSION "1.39.2" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012701 +#define NGHTTP2_VERSION_NUM 0x012702 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_option.c b/deps/nghttp2/lib/nghttp2_option.c index 8946d7dd38cfb8..e53f22d367f84a 100644 --- a/deps/nghttp2/lib/nghttp2_option.c +++ b/deps/nghttp2/lib/nghttp2_option.c @@ -116,3 +116,8 @@ void nghttp2_option_set_no_closed_streams(nghttp2_option *option, int val) { option->opt_set_mask |= NGHTTP2_OPT_NO_CLOSED_STREAMS; option->no_closed_streams = val; } + +void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, size_t val) { + option->opt_set_mask |= NGHTTP2_OPT_MAX_OUTBOUND_ACK; + option->max_outbound_ack = val; +} diff --git a/deps/nghttp2/lib/nghttp2_option.h b/deps/nghttp2/lib/nghttp2_option.h index 29e72aa321007a..1f740aaa6e364e 100644 --- a/deps/nghttp2/lib/nghttp2_option.h +++ b/deps/nghttp2/lib/nghttp2_option.h @@ -66,6 +66,7 @@ typedef enum { NGHTTP2_OPT_MAX_SEND_HEADER_BLOCK_LENGTH = 1 << 8, NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE = 1 << 9, NGHTTP2_OPT_NO_CLOSED_STREAMS = 1 << 10, + NGHTTP2_OPT_MAX_OUTBOUND_ACK = 1 << 11, } nghttp2_option_flag; /** @@ -80,6 +81,10 @@ struct nghttp2_option { * NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE */ size_t max_deflate_dynamic_table_size; + /** + * NGHTTP2_OPT_MAX_OUTBOUND_ACK + */ + size_t max_outbound_ack; /** * Bitwise OR of nghttp2_option_flag to determine that which fields * are specified. diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index 33d987667edaf5..3420cfa2f1c653 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -457,6 +457,7 @@ static int session_new(nghttp2_session **session_ptr, (*session_ptr)->remote_settings.max_concurrent_streams = 100; (*session_ptr)->max_send_header_block_length = NGHTTP2_MAX_HEADERSLEN; + (*session_ptr)->max_outbound_ack = NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM; if (option) { if ((option->opt_set_mask & NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE) && @@ -516,6 +517,10 @@ static int session_new(nghttp2_session **session_ptr, option->no_closed_streams) { (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_NO_CLOSED_STREAMS; } + + if (option->opt_set_mask & NGHTTP2_OPT_MAX_OUTBOUND_ACK) { + (*session_ptr)->max_outbound_ack = option->max_outbound_ack; + } } rv = nghttp2_hd_deflate_init2(&(*session_ptr)->hd_deflater, @@ -6857,7 +6862,7 @@ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, mem = &session->mem; if ((flags & NGHTTP2_FLAG_ACK) && - session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + session->obq_flood_counter_ >= session->max_outbound_ack) { return NGHTTP2_ERR_FLOODED; } @@ -7002,7 +7007,7 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, return NGHTTP2_ERR_INVALID_ARGUMENT; } - if (session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + if (session->obq_flood_counter_ >= session->max_outbound_ack) { return NGHTTP2_ERR_FLOODED; } } diff --git a/deps/nghttp2/lib/nghttp2_session.h b/deps/nghttp2/lib/nghttp2_session.h index bd7a5b35817599..90ead9c0395b4f 100644 --- a/deps/nghttp2/lib/nghttp2_session.h +++ b/deps/nghttp2/lib/nghttp2_session.h @@ -97,7 +97,7 @@ typedef struct { response frames are stacked up, which leads to memory exhaustion. The value selected here is arbitrary, but safe value and if we have these frames in this number, it is considered suspicious. */ -#define NGHTTP2_MAX_OBQ_FLOOD_ITEM 10000 +#define NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM 1000 /* The default value of maximum number of concurrent streams. */ #define NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS 0xffffffffu @@ -258,8 +258,12 @@ struct nghttp2_session { size_t num_idle_streams; /* The number of bytes allocated for nvbuf */ size_t nvbuflen; - /* Counter for detecting flooding in outbound queue */ + /* Counter for detecting flooding in outbound queue. If it exceeds + max_outbound_ack, session will be closed. */ size_t obq_flood_counter_; + /* The maximum number of outgoing SETTINGS ACK and PING ACK in + outbound queue. */ + size_t max_outbound_ack; /* The maximum length of header block to send. Calculated by the same way as nghttp2_hd_deflate_bound() does. */ size_t max_send_header_block_length; From 76a7ada15d4cbad16bd3b48ad839843f2e089b9e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 5 Aug 2019 17:22:30 +0200 Subject: [PATCH 07/19] http2: improve JS-side debug logging DRY up the `debug()` calls, and in particular, avoid building template strings before we know whether we need to. 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 --- lib/internal/http2/core.js | 115 ++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 54 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index a57e63c235398a..93ae66ca890a0a 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -127,6 +127,26 @@ const { StreamPipe } = internalBinding('stream_pipe'); const { _connectionListener: httpConnectionListener } = http; const debug = util.debuglog('http2'); +// TODO(addaleax): See if this can be made more efficient by figuring out +// whether debugging is enabled before we perform any further steps. Currently, +// this seems pretty fast, though. +function debugStream(id, sessionType, message, ...args) { + debug('Http2Stream %s [Http2Session %s]: ' + message, + id, sessionName(sessionType), ...args); +} + +function debugStreamObj(stream, message, ...args) { + debugStream(stream[kID], stream[kSession][kType], ...args); +} + +function debugSession(sessionType, message, ...args) { + debug('Http2Session %s: ' + message, sessionName(sessionType), ...args); +} + +function debugSessionObj(session, message, ...args) { + debugSession(session[kType], message, ...args); +} + const kMaxFrameSize = (2 ** 24) - 1; const kMaxInt = (2 ** 32) - 1; const kMaxStreams = (2 ** 31) - 1; @@ -248,8 +268,7 @@ function onSessionHeaders(handle, id, cat, flags, headers) { const type = session[kType]; session[kUpdateTimer](); - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(type)}]: headers received`); + debugStream(id, type, 'headers received'); const streams = session[kState].streams; const endOfStream = !!(flags & NGHTTP2_FLAG_END_STREAM); @@ -309,8 +328,7 @@ function onSessionHeaders(handle, id, cat, flags, headers) { const originSet = session[kState].originSet = initOriginSet(session); originSet.delete(stream[kOrigin]); } - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(type)}]: emitting stream '${event}' event`); + debugStream(id, type, "emitting stream '%s' event", event); process.nextTick(emit, stream, event, obj, flags, headers); } if (endOfStream) { @@ -351,7 +369,7 @@ function onPing(payload) { if (session.destroyed) return; session[kUpdateTimer](); - debug(`Http2Session ${sessionName(session[kType])}: new ping received`); + debugSessionObj(session, 'new ping received'); session.emit('ping', payload); } @@ -366,8 +384,7 @@ function onStreamClose(code) { if (stream.destroyed) return; - debug(`Http2Stream ${stream[kID]} [Http2Session ` + - `${sessionName(stream[kSession][kType])}]: closed with code ${code}`); + debugStreamObj(stream, 'closed with code %d', code); if (!stream.closed) closeStream(stream, code, kNoRstStream); @@ -403,7 +420,7 @@ function onSettings() { if (session.destroyed) return; session[kUpdateTimer](); - debug(`Http2Session ${sessionName(session[kType])}: new settings received`); + debugSessionObj(session, 'new settings received'); session[kRemoteSettings] = undefined; session.emit('remoteSettings', session.remoteSettings); } @@ -415,9 +432,9 @@ function onPriority(id, parent, weight, exclusive) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Stream ${id} [Http2Session ` + - `${sessionName(session[kType])}]: priority [parent: ${parent}, ` + - `weight: ${weight}, exclusive: ${exclusive}]`); + debugStream(id, session[kType], + 'priority [parent: %d, weight: %d, exclusive: %s]', + parent, weight, exclusive); const emitter = session[kState].streams.get(id) || session; if (!emitter.destroyed) { emitter[kUpdateTimer](); @@ -431,8 +448,8 @@ function onFrameError(id, type, code) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: error sending frame ` + - `type ${type} on stream ${id}, code: ${code}`); + debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d', + type, id, code); const emitter = session[kState].streams.get(id) || session; emitter[kUpdateTimer](); emitter.emit('frameError', type, code, id); @@ -442,8 +459,8 @@ function onAltSvc(stream, origin, alt) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: altsvc received: ` + - `stream: ${stream}, origin: ${origin}, alt: ${alt}`); + debugSessionObj(session, 'altsvc received: stream: %d, origin: %s, alt: %s', + stream, origin, alt); session[kUpdateTimer](); session.emit('altsvc', alt, origin, stream); } @@ -470,8 +487,7 @@ function onOrigin(origins) { const session = this[kOwner]; if (session.destroyed) return; - debug('Http2Session %s: origin received: %j', - sessionName(session[kType]), origins); + debugSessionObj(session, 'origin received: %j', origins); session[kUpdateTimer](); if (!session.encrypted || session.destroyed) return undefined; @@ -491,8 +507,8 @@ function onGoawayData(code, lastStreamID, buf) { const session = this[kOwner]; if (session.destroyed) return; - debug(`Http2Session ${sessionName(session[kType])}: goaway ${code} ` + - `received [last stream id: ${lastStreamID}]`); + debugSessionObj(session, 'goaway %d received [last stream id: %d]', + code, lastStreamID); const state = session[kState]; state.goawayCode = code; @@ -545,8 +561,7 @@ function requestOnConnect(headers, options) { return; } - debug(`Http2Session ${sessionName(session[kType])}: connected, ` + - 'initializing request'); + debugSessionObj(session, 'connected, initializing request'); let streamOptions = 0; if (options.endStream) @@ -641,13 +656,13 @@ function settingsCallback(cb, ack, duration) { this[kState].pendingAck--; this[kLocalSettings] = undefined; if (ack) { - debug(`Http2Session ${sessionName(this[kType])}: settings received`); + debugSessionObj(this, 'settings received'); const settings = this.localSettings; if (typeof cb === 'function') cb(null, settings, duration); this.emit('localSettings', settings); } else { - debug(`Http2Session ${sessionName(this[kType])}: settings canceled`); + debugSessionObj(this, 'settings canceled'); if (typeof cb === 'function') cb(new ERR_HTTP2_SETTINGS_CANCEL()); } @@ -657,7 +672,7 @@ function settingsCallback(cb, ack, duration) { function submitSettings(settings, callback) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: submitting settings`); + debugSessionObj(this, 'submitting settings'); this[kUpdateTimer](); updateSettingsBuffer(settings); if (!this[kHandle].settings(settingsCallback.bind(this, callback))) { @@ -691,7 +706,7 @@ function submitPriority(options) { function submitGoaway(code, lastStreamID, opaqueData) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: submitting goaway`); + debugSessionObj(this, 'submitting goaway'); this[kUpdateTimer](); this[kHandle].goaway(code, lastStreamID, opaqueData); } @@ -821,7 +836,9 @@ function setupHandle(socket, type, options) { process.nextTick(emit, this, 'connect', this, socket); return; } - debug(`Http2Session ${sessionName(type)}: setting up session handle`); + + debugSession(type, 'setting up session handle'); + this[kState].flags |= SESSION_FLAGS_READY; updateOptionsBuffer(options); @@ -983,7 +1000,7 @@ class Http2Session extends EventEmitter { setupFn(); } - debug(`Http2Session ${sessionName(type)}: created`); + debugSession(type, 'created'); } // Returns undefined if the socket is not yet connected, true if the @@ -1156,7 +1173,7 @@ class Http2Session extends EventEmitter { if (callback && typeof callback !== 'function') throw new ERR_INVALID_CALLBACK(); - debug(`Http2Session ${sessionName(this[kType])}: sending settings`); + debugSessionObj(this, 'sending settings'); this[kState].pendingAck++; @@ -1197,7 +1214,7 @@ class Http2Session extends EventEmitter { destroy(error = NGHTTP2_NO_ERROR, code) { if (this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: destroying`); + debugSessionObj(this, 'destroying'); if (typeof error === 'number') { code = error; @@ -1258,7 +1275,7 @@ class Http2Session extends EventEmitter { close(callback) { if (this.closed || this.destroyed) return; - debug(`Http2Session ${sessionName(this[kType])}: marking session closed`); + debugSessionObj(this, 'marking session closed'); this[kState].flags |= SESSION_FLAGS_CLOSED; if (typeof callback === 'function') this.once('close', callback); @@ -1430,7 +1447,7 @@ class ClientHttp2Session extends Http2Session { // Submits a new HTTP2 request to the connected peer. Returns the // associated Http2Stream instance. request(headers, options) { - debug(`Http2Session ${sessionName(this[kType])}: initiating request`); + debugSessionObj(this, 'initiating request'); if (this.destroyed) throw new ERR_HTTP2_INVALID_SESSION(); @@ -1827,8 +1844,7 @@ class Http2Stream extends Duplex { if (this.pending) { this.once('ready', () => this._final(cb)); } else if (handle !== undefined) { - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(this[kSession][kType])}]: _final shutting down`); + debugStreamObj(this, '_final shutting down'); const req = new ShutdownWrap(); req.oncomplete = afterShutdown; req.callback = cb; @@ -1887,9 +1903,7 @@ class Http2Stream extends Duplex { assertIsObject(headers, 'headers'); headers = Object.assign(Object.create(null), headers); - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: sending trailers`); + debugStreamObj(this, 'sending trailers'); this[kUpdateTimer](); @@ -1944,8 +1958,8 @@ class Http2Stream extends Duplex { const handle = this[kHandle]; const id = this[kID]; - debug(`Http2Stream ${this[kID] || ''} [Http2Session ` + - `${sessionName(session[kType])}]: destroying stream`); + debugStream(this[kID] || 'pending', session[kType], 'destroying stream'); + const state = this[kState]; const code = err != null ? NGHTTP2_INTERNAL_ERROR : (state.rstCode || NGHTTP2_NO_ERROR); @@ -2256,8 +2270,7 @@ class ServerHttp2Stream extends Http2Stream { const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating push stream`); + debugStreamObj(this, 'initiating push stream'); this[kUpdateTimer](); @@ -2339,9 +2352,7 @@ class ServerHttp2Stream extends Http2Stream { assertIsObject(options, 'options'); options = Object.assign({}, options); - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response`); + debugStreamObj(this, 'initiating response'); this[kUpdateTimer](); options.endStream = !!options.endStream; @@ -2420,8 +2431,7 @@ class ServerHttp2Stream extends Http2Stream { validateNumber(fd, 'fd'); - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response from fd`); + debugStreamObj(this, 'initiating response from fd'); this[kUpdateTimer](); this.ownsFd = false; @@ -2481,8 +2491,7 @@ class ServerHttp2Stream extends Http2Stream { } const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: initiating response from file`); + debugStreamObj(this, 'initiating response from file'); this[kUpdateTimer](); this.ownsFd = true; @@ -2515,9 +2524,7 @@ class ServerHttp2Stream extends Http2Stream { assertIsObject(headers, 'headers'); headers = Object.assign(Object.create(null), headers); - const session = this[kSession]; - debug(`Http2Stream ${this[kID]} [Http2Session ` + - `${sessionName(session[kType])}]: sending additional headers`); + debugStreamObj(this, 'sending additional headers'); if (headers[HTTP2_HEADER_STATUS] != null) { const statusCode = headers[HTTP2_HEADER_STATUS] |= 0; @@ -2608,8 +2615,7 @@ function socketOnError(error) { // we can do and the other side is fully within its rights to do so. if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null) return session.destroy(); - debug(`Http2Session ${sessionName(session[kType])}: socket error [` + - `${error.message}]`); + debugSessionObj(this, 'socket error [%s]', error.message); session.destroy(error); } } @@ -2654,7 +2660,8 @@ function connectionListener(socket) { return httpConnectionListener.call(this, socket); } // Let event handler deal with the socket - debug(`Unknown protocol from ${socket.remoteAddress}:${socket.remotePort}`); + debug('Unknown protocol from %s:%s', + socket.remoteAddress, socket.remotePort); if (!this.emit('unknownProtocol', socket)) { // We don't know what to do, so let's just tell the other side what's // going on in a format that they *might* understand. @@ -2779,7 +2786,7 @@ function setupCompat(ev) { function socketOnClose() { const session = this[kSession]; if (session !== undefined) { - debug(`Http2Session ${sessionName(session[kType])}: socket closed`); + debugSessionObj(session, 'socket closed'); const err = session.connecting ? new ERR_SOCKET_CLOSED() : null; const state = session[kState]; state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL)); From 2eb914ff5f1f3ddcbe91c50c5b0ae3fe565e2bba Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 00:51:51 +0200 Subject: [PATCH 08/19] http2: only call into JS when necessary for session events For some JS events, it only makes sense to call into JS when there are listeners for the event in question. The overhead is noticeable if a lot of these events are emitted during the lifetime of a session. To reduce this overhead, keep track of whether any/how many JS listeners are present, and if there are none, skip calls into JS altogether. This is part of performance improvements to mitigate CVE-2019-9513. 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 --- lib/internal/http2/core.js | 119 ++++++++++++++++++++++++++++++++++--- src/env.h | 1 + src/node_http2.cc | 29 ++++++++- src/node_http2.h | 20 +++++++ 4 files changed, 161 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 93ae66ca890a0a..d2f1c22ac705df 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -167,6 +167,7 @@ const kID = Symbol('id'); const kInit = Symbol('init'); const kInfoHeaders = Symbol('sent-info-headers'); const kLocalSettings = Symbol('local-settings'); +const kNativeFields = Symbol('kNativeFields'); const kOptions = Symbol('options'); const kOwner = owner_symbol; const kOrigin = Symbol('origin'); @@ -188,7 +189,15 @@ const { paddingBuffer, PADDING_BUF_FRAME_LENGTH, PADDING_BUF_MAX_PAYLOAD_LENGTH, - PADDING_BUF_RETURN_VALUE + PADDING_BUF_RETURN_VALUE, + kBitfield, + kSessionPriorityListenerCount, + kSessionFrameErrorListenerCount, + kSessionUint8FieldCount, + kSessionHasRemoteSettingsListeners, + kSessionRemoteSettingsIsUpToDate, + kSessionHasPingListeners, + kSessionHasAltsvcListeners, } = binding; const { @@ -364,6 +373,76 @@ function submitRstStream(code) { } } +// Keep track of the number/presence of JS event listeners. Knowing that there +// are no listeners allows the C++ code to skip calling into JS for an event. +function sessionListenerAdded(name) { + switch (name) { + case 'ping': + this[kNativeFields][kBitfield] |= 1 << kSessionHasPingListeners; + break; + case 'altsvc': + this[kNativeFields][kBitfield] |= 1 << kSessionHasAltsvcListeners; + break; + case 'remoteSettings': + this[kNativeFields][kBitfield] |= 1 << kSessionHasRemoteSettingsListeners; + break; + case 'priority': + this[kNativeFields][kSessionPriorityListenerCount]++; + break; + case 'frameError': + this[kNativeFields][kSessionFrameErrorListenerCount]++; + break; + } +} + +function sessionListenerRemoved(name) { + switch (name) { + case 'ping': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= ~(1 << kSessionHasPingListeners); + break; + case 'altsvc': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= ~(1 << kSessionHasAltsvcListeners); + break; + case 'remoteSettings': + if (this.listenerCount(name) > 0) return; + this[kNativeFields][kBitfield] &= + ~(1 << kSessionHasRemoteSettingsListeners); + break; + case 'priority': + this[kNativeFields][kSessionPriorityListenerCount]--; + break; + case 'frameError': + this[kNativeFields][kSessionFrameErrorListenerCount]--; + break; + } +} + +// Also keep track of listeners for the Http2Stream instances, as some events +// are emitted on those objects. +function streamListenerAdded(name) { + switch (name) { + case 'priority': + this[kSession][kNativeFields][kSessionPriorityListenerCount]++; + break; + case 'frameError': + this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++; + break; + } +} + +function streamListenerRemoved(name) { + switch (name) { + case 'priority': + this[kSession][kNativeFields][kSessionPriorityListenerCount]--; + break; + case 'frameError': + this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--; + break; + } +} + function onPing(payload) { const session = this[kOwner]; if (session.destroyed) @@ -421,7 +500,6 @@ function onSettings() { return; session[kUpdateTimer](); debugSessionObj(session, 'new settings received'); - session[kRemoteSettings] = undefined; session.emit('remoteSettings', session.remoteSettings); } @@ -863,6 +941,10 @@ function setupHandle(socket, type, options) { handle.consume(socket._handle._externalStream); this[kHandle] = handle; + if (this[kNativeFields]) + handle.fields.set(this[kNativeFields]); + else + this[kNativeFields] = handle.fields; if (socket.encrypted) { this[kAlpnProtocol] = socket.alpnProtocol; @@ -904,6 +986,7 @@ function finishSessionDestroy(session, error) { session[kProxySocket] = undefined; session[kSocket] = undefined; session[kHandle] = undefined; + session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); socket[kSession] = undefined; socket[kServer] = undefined; @@ -982,6 +1065,7 @@ class Http2Session extends EventEmitter { this[kProxySocket] = null; this[kSocket] = socket; this[kTimeout] = null; + this[kHandle] = undefined; // Do not use nagle's algorithm if (typeof socket.setNoDelay === 'function') @@ -1000,6 +1084,11 @@ class Http2Session extends EventEmitter { setupFn(); } + if (!this[kNativeFields]) + this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); + this.on('newListener', sessionListenerAdded); + this.on('removeListener', sessionListenerRemoved); + debugSession(type, 'created'); } @@ -1154,13 +1243,18 @@ class Http2Session extends EventEmitter { // The settings currently in effect for the remote peer. get remoteSettings() { - const settings = this[kRemoteSettings]; - if (settings !== undefined) - return settings; + if (this[kNativeFields][kBitfield] & + (1 << kSessionRemoteSettingsIsUpToDate)) { + const settings = this[kRemoteSettings]; + if (settings !== undefined) { + return settings; + } + } if (this.destroyed || this.connecting) return {}; + this[kNativeFields][kBitfield] |= (1 << kSessionRemoteSettingsIsUpToDate); return this[kRemoteSettings] = getSettings(this[kHandle], true); // Remote } @@ -1343,6 +1437,12 @@ class ServerHttp2Session extends Http2Session { constructor(options, socket, server) { super(NGHTTP2_SESSION_SERVER, options, socket); this[kServer] = server; + // This is a bit inaccurate because it does not reflect changes to + // number of listeners made after the session was created. This should + // not be an issue in practice. Additionally, the 'priority' event on + // server instances (or any other object) is fully undocumented. + this[kNativeFields][kSessionPriorityListenerCount] = + server.listenerCount('priority'); } get server() { @@ -1660,6 +1760,9 @@ class Http2Stream extends Duplex { }; this.on('pause', streamOnPause); + + this.on('newListener', streamListenerAdded); + this.on('removeListener', streamListenerRemoved); } [kUpdateTimer]() { @@ -2633,7 +2736,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) { } function sessionOnError(error) { - if (this[kServer]) + if (this[kServer] !== undefined) this[kServer].emit('sessionError', error, this); } @@ -2682,8 +2785,10 @@ function connectionListener(socket) { const session = new ServerHttp2Session(options, socket, this); session.on('stream', sessionOnStream); - session.on('priority', sessionOnPriority); session.on('error', sessionOnError); + // Don't count our own internal listener. + session.on('priority', sessionOnPriority); + session[kNativeFields][kSessionPriorityListenerCount]--; if (this.timeout) session.setTimeout(this.timeout, sessionOnTimeout); diff --git a/src/env.h b/src/env.h index d7003616ece13c..7f77face35057f 100644 --- a/src/env.h +++ b/src/env.h @@ -173,6 +173,7 @@ struct PackageConfig { V(family_string, "family") \ V(fatal_exception_string, "_fatalException") \ V(fd_string, "fd") \ + V(fields_string, "fields") \ V(file_string, "file") \ V(fingerprint256_string, "fingerprint256") \ V(fingerprint_string, "fingerprint") \ diff --git a/src/node_http2.cc b/src/node_http2.cc index c7ccaf78690f09..6882c9d957c436 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -24,6 +24,7 @@ using v8::ObjectTemplate; using v8::String; using v8::Uint32; using v8::Uint32Array; +using v8::Uint8Array; using v8::Undefined; using node::performance::PerformanceEntry; @@ -631,6 +632,15 @@ Http2Session::Http2Session(Environment* env, outgoing_storage_.reserve(4096); outgoing_buffers_.reserve(32); + + { + // Make the js_fields_ property accessible to JS land. + Local ab = + ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount); + Local uint8_arr = + Uint8Array::New(ab, 0, kSessionUint8FieldCount); + USE(wrap->Set(env->context(), env->fields_string(), uint8_arr)); + } } Http2Session::~Http2Session() { @@ -1032,7 +1042,8 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, // Do not report if the frame was not sent due to the session closing if (error_code == NGHTTP2_ERR_SESSION_CLOSING || error_code == NGHTTP2_ERR_STREAM_CLOSED || - error_code == NGHTTP2_ERR_STREAM_CLOSING) { + error_code == NGHTTP2_ERR_STREAM_CLOSING || + session->js_fields_[kSessionFrameErrorListenerCount] == 0) { return 0; } @@ -1337,6 +1348,7 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // are considered advisory only, so this has no real effect other than to // simply let user code know that the priority has changed. void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { + if (js_fields_[kSessionPriorityListenerCount] == 0) return; Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1399,6 +1411,7 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete ALTSVC frame has been received. void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { + if (!(js_fields_[kBitfield] & (1 << kSessionHasAltsvcListeners))) return; Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1484,6 +1497,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { return; } + if (!(js_fields_[kBitfield] & (1 << kSessionHasPingListeners))) return; // Notify the session that a ping occurred arg = Buffer::Copy(env(), reinterpret_cast(frame->ping.opaque_data), @@ -1495,6 +1509,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (!ack) { + js_fields_[kBitfield] &= ~(1 << kSessionRemoteSettingsIsUpToDate); + if (!(js_fields_[kBitfield] & (1 << kSessionHasRemoteSettingsListeners))) + return; // This is not a SETTINGS acknowledgement, notify and return MakeCallback(env()->onsettings_string(), 0, nullptr); return; @@ -2980,6 +2997,16 @@ void Initialize(Local target, NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH); NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE); + NODE_DEFINE_CONSTANT(target, kBitfield); + NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount); + NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount); + NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount); + + NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners); + NODE_DEFINE_CONSTANT(target, kSessionRemoteSettingsIsUpToDate); + NODE_DEFINE_CONSTANT(target, kSessionHasPingListeners); + NODE_DEFINE_CONSTANT(target, kSessionHasAltsvcListeners); + // Method to fetch the nghttp2 string description of an nghttp2 error code env->SetMethod(target, "nghttp2ErrorString", HttpErrorString); diff --git a/src/node_http2.h b/src/node_http2.h index 91c0673d295910..89c0b7d38de3ea 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -675,6 +675,23 @@ class Http2Stream::Provider::Stream : public Http2Stream::Provider { void* user_data); }; +// Indices for js_fields_, which serves as a way to communicate data with JS +// land fast. In particular, we store information about the number/presence +// of certain event listeners in JS, and skip calls from C++ into JS if they +// are missing. +enum SessionUint8Fields { + kBitfield, // See below + kSessionPriorityListenerCount, + kSessionFrameErrorListenerCount, + kSessionUint8FieldCount +}; + +enum SessionBitfieldFlags { + kSessionHasRemoteSettingsListeners, + kSessionRemoteSettingsIsUpToDate, + kSessionHasPingListeners, + kSessionHasAltsvcListeners +}; class Http2Session : public AsyncWrap, public StreamListener { public: @@ -961,6 +978,9 @@ class Http2Session : public AsyncWrap, public StreamListener { // The underlying nghttp2_session handle nghttp2_session* session_; + // JS-accessible numeric fields, as indexed by SessionUint8Fields. + uint8_t js_fields_[kSessionUint8FieldCount] = {}; + // The session type: client or server nghttp2_session_type session_type_; From 7f11465572888340b4c7b399c1f46598d1c4ea50 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 01:30:35 +0200 Subject: [PATCH 09/19] 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 }); From 05dada46eea59c0bfdabe4f54d64cda2f315cec9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 22:27:48 +0200 Subject: [PATCH 10/19] http2: limit number of rejected stream openings Limit the number of streams that are rejected upon creation. Since each such rejection is associated with an `NGHTTP2_ENHANCE_YOUR_CALM` error that should tell the peer to not open any more streams, continuing to open streams should be read as a sign of a misbehaving peer. The limit is currently set to 100 but could be changed or made configurable. This is intended to mitigate CVE-2019-9514. 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 | 8 ++++++++ src/node_http2.h | 5 +++++ src/node_revert.h | 5 ++++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7d3b117f44107b..e0cedc49183bb9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -6,6 +6,8 @@ #include "node_http2_state.h" #include "node_internals.h" #include "node_perf.h" +#include "node_revert.h" +#include "util-inl.h" #include @@ -921,11 +923,17 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, if (UNLIKELY(!session->CanAddStream() || Http2Stream::New(session, id, frame->headers.cat) == nullptr)) { + if (session->rejected_stream_count_++ > 100 && + !IsReverted(SECURITY_REVERT_CVE_2019_9514)) { + return NGHTTP2_ERR_CALLBACK_FAILURE; + } // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } + + session->rejected_stream_count_ = 0; } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } diff --git a/src/node_http2.h b/src/node_http2.h index 8424ffb89712e7..ba8d06893a8032 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1017,6 +1017,11 @@ class Http2Session : public AsyncWrap, public StreamListener { std::vector outgoing_buffers_; std::vector outgoing_storage_; std::vector pending_rst_streams_; + // Count streams that have been rejected while being opened. Exceeding a fixed + // limit will result in the session being destroyed, as an indication of a + // misbehaving peer. This counter is reset once new streams are being + // accepted again. + int32_t rejected_stream_count_ = 0; void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/src/node_revert.h b/src/node_revert.h index c5963afeafd027..e98e583ec3c94e 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -15,8 +15,11 @@ **/ namespace node { -#define SECURITY_REVERSIONS(XX) +#define SECURITY_REVERSIONS(XX) \ + XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") + // TODO(addaleax): Remove all of the above before Node.js 13 as the comment + // at the start of the file indicates. enum reversion { #define V(code, ...) SECURITY_REVERT_##code, From 477461a51f64ec6969654d98018281b0ba2a5464 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Aug 2019 22:55:16 +0200 Subject: [PATCH 11/19] http2: limit number of invalid incoming frames Limit the number of invalid input frames, as they may be pointing towards a misbehaving peer. The limit is currently set to 1000 but could be changed or made configurable. This is intended to mitigate CVE-2019-9514. [This commit differs from the v12.x one due to the lack of https://github.com/libuv/libuv/commit/ee24ce900e5714c950b248da2b. See the comment in the test for more details.] 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 | 4 ++ src/node_http2.h | 2 + test/parallel/test-http2-reset-flood.js | 91 +++++++++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 test/parallel/test-http2-reset-flood.js diff --git a/src/node_http2.cc b/src/node_http2.cc index e0cedc49183bb9..0a4fefb45a6bc8 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1018,6 +1018,10 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, Http2Session* session = static_cast(user_data); Debug(session, "invalid frame received, code: %d", lib_error_code); + if (session->invalid_frame_count_++ > 1000 && + !IsReverted(SECURITY_REVERT_CVE_2019_9514)) { + return 1; + } // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error if (nghttp2_is_fatal(lib_error_code) || diff --git a/src/node_http2.h b/src/node_http2.h index ba8d06893a8032..da2acbca2ca051 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1022,6 +1022,8 @@ class Http2Session : public AsyncWrap, public StreamListener { // misbehaving peer. This counter is reset once new streams are being // accepted again. int32_t rejected_stream_count_ = 0; + // Also use the invalid frame count as a measure for rejecting input frames. + int32_t invalid_frame_count_ = 0; void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/test/parallel/test-http2-reset-flood.js b/test/parallel/test-http2-reset-flood.js new file mode 100644 index 00000000000000..268fd56155876d --- /dev/null +++ b/test/parallel/test-http2-reset-flood.js @@ -0,0 +1,91 @@ +// Flags: --experimental-worker +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const { Worker, parentPort } = require('worker_threads'); + +// Verify that creating a number of invalid HTTP/2 streams will eventually +// result in the peer closing the session. +// This test uses separate threads for client and server to avoid +// the two event loops intermixing, as we are writing in a busy loop here. + +if (process.env.HAS_STARTED_WORKER) { + const server = http2.createServer(); + server.on('stream', (stream) => { + stream.respond({ + 'content-type': 'text/plain', + ':status': 200 + }); + stream.end('Hello, world!\n'); + }); + server.listen(0, () => parentPort.postMessage(server.address().port)); + return; +} + +process.env.HAS_STARTED_WORKER = 1; +const worker = new Worker(__filename).on('message', common.mustCall((port) => { + const h2header = Buffer.alloc(9); + const conn = net.connect(port); + + conn.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'); + + h2header[3] = 4; // Send a settings frame. + conn.write(Buffer.from(h2header)); + + let inbuf = Buffer.alloc(0); + let state = 'settingsHeader'; + let settingsFrameLength; + conn.on('data', (chunk) => { + inbuf = Buffer.concat([inbuf, chunk]); + switch (state) { + case 'settingsHeader': + if (inbuf.length < 9) return; + settingsFrameLength = inbuf.readIntBE(0, 3); + inbuf = inbuf.slice(9); + state = 'readingSettings'; + // Fallthrough + case 'readingSettings': + if (inbuf.length < settingsFrameLength) return; + inbuf = inbuf.slice(settingsFrameLength); + h2header[3] = 4; // Send a settings ACK. + h2header[4] = 1; + conn.write(Buffer.from(h2header)); + state = 'ignoreInput'; + writeRequests(); + } + }); + + let gotError = false; + + let i = 1; + function writeRequests() { + for (; !gotError; i += 2) { + h2header[3] = 1; // HEADERS + h2header[4] = 0x5; // END_HEADERS|END_STREAM + h2header.writeIntBE(1, 0, 3); // Length: 1 + h2header.writeIntBE(i, 5, 4); // Stream ID + // 0x88 = :status: 200 + conn.write(Buffer.concat([h2header, Buffer.from([0x88])])); + + if (i % 1000 === 1) { + // Delay writing a bit so we get the chance to actually observe + // an error. This is not necessary on master/v12.x, because there + // conn.write() can fail directly when writing to a connection + // that was closed by the remote peer due to + // https://github.com/libuv/libuv/commit/ee24ce900e5714c950b248da2b + i += 2; + return setImmediate(writeRequests); + } + } + } + + conn.once('error', common.mustCall(() => { + gotError = true; + worker.terminate(); + conn.destroy(); + })); +})); From f4242e24f9f4fb185909f040cbd2dd889d79439b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:10:54 +0200 Subject: [PATCH 12/19] http2: handle 0-length headers better Ignore headers with 0-length names and track memory for headers the way we track it for other HTTP/2 session memory too. This is intended to mitigate CVE-2019-9516. 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 | 13 ++++++++-- src/node_revert.h | 1 + .../parallel/test-http2-zero-length-header.js | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-zero-length-header.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 0a4fefb45a6bc8..7d10eaf8017404 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1318,6 +1318,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { return; std::vector headers(stream->move_headers()); + DecrementCurrentSessionMemory(stream->current_headers_length_); + stream->current_headers_length_ = 0; Local name_str; Local value_str; @@ -1975,6 +1977,7 @@ Http2Stream::~Http2Stream() { if (session_ == nullptr) return; Debug(this, "tearing down stream"); + session_->DecrementCurrentSessionMemory(current_headers_length_); session_->RemoveStream(this); session_ = nullptr; } @@ -1989,6 +1992,7 @@ std::string Http2Stream::diagnostic_name() const { void Http2Stream::StartHeaders(nghttp2_headers_category category) { Debug(this, "starting headers, category: %d", id_, category); CHECK(!this->IsDestroyed()); + session_->DecrementCurrentSessionMemory(current_headers_length_); current_headers_length_ = 0; current_headers_.clear(); current_headers_category_ = category; @@ -2260,8 +2264,12 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, CHECK(!this->IsDestroyed()); if (this->statistics_.first_header == 0) this->statistics_.first_header = uv_hrtime(); - size_t length = nghttp2_rcbuf_get_buf(name).len + - nghttp2_rcbuf_get_buf(value).len + 32; + size_t name_len = nghttp2_rcbuf_get_buf(name).len; + if (name_len == 0 && !IsReverted(SECURITY_REVERT_CVE_2019_9516)) { + return true; // Ignore headers with empty names. + } + size_t value_len = nghttp2_rcbuf_get_buf(value).len; + size_t length = name_len + value_len + 32; // A header can only be added if we have not exceeded the maximum number // of headers and the session has memory available for it. if (!session_->IsAvailableSessionMemory(length) || @@ -2277,6 +2285,7 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf_incref(name); nghttp2_rcbuf_incref(value); current_headers_length_ += length; + session_->IncrementCurrentSessionMemory(length); return true; } diff --git a/src/node_revert.h b/src/node_revert.h index e98e583ec3c94e..50ada582de48e3 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -17,6 +17,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ + XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") // TODO(addaleax): Remove all of the above before Node.js 13 as the comment // at the start of the file indicates. diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js new file mode 100644 index 00000000000000..7b142d75f003b6 --- /dev/null +++ b/test/parallel/test-http2-zero-length-header.js @@ -0,0 +1,25 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('http2'); + +const server = http2.createServer(); +server.on('stream', (stream, headers) => { + assert.deepStrictEqual(headers, { + ':scheme': 'http', + ':authority': `localhost:${server.address().port}`, + ':method': 'GET', + ':path': '/', + 'bar': '', + '__proto__': null + }); + stream.session.destroy(); + server.close(); +}); +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}/`); + client.request({ ':path': '/', '': 'foo', 'bar': '' }).end(); +})); From 460f896c631f1be52b0ab6c9f30e0b66f601b2a1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:18:13 +0200 Subject: [PATCH 13/19] http2: shrink default `vector::reserve()` allocations Allocating memory upfront comes with overhead, and in particular, `std::vector` implementations do not necessarily return memory to the system when one might expect that (e.g. after shrinking the vector). 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7d10eaf8017404..e03366dd1a0bad 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -632,7 +632,7 @@ Http2Session::Http2Session(Environment* env, // fails. CHECK_EQ(fn(&session_, callbacks, this, *opts, *allocator_info), 0); - outgoing_storage_.reserve(4096); + outgoing_storage_.reserve(1024); outgoing_buffers_.reserve(32); { @@ -1947,7 +1947,7 @@ Http2Stream::Http2Stream(Http2Session* session, if (max_header_pairs_ == 0) { max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; } - current_headers_.reserve(max_header_pairs_); + current_headers_.reserve(std::min(max_header_pairs_, 12u)); // Limit the number of header octets max_header_length_ = From 17357d37a9eba4ac1cbafe8628bf12cc900bc642 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 23:37:58 +0200 Subject: [PATCH 14/19] http2: consider 0-length non-end DATA frames an error This is intended to mitigate CVE-2019-9518. 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 | 12 ++++++++---- src/node_http2.h | 2 +- src/node_revert.h | 1 + 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index e03366dd1a0bad..c513d286e75d72 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -980,8 +980,7 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle, frame->hd.type); switch (frame->hd.type) { case NGHTTP2_DATA: - session->HandleDataFrame(frame); - break; + return session->HandleDataFrame(frame); case NGHTTP2_PUSH_PROMISE: // Intentional fall-through, handled just like headers frames case NGHTTP2_HEADERS: @@ -1398,13 +1397,18 @@ void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete DATA frame has been received. // If we know that this was the last DATA frame (because the END_STREAM flag // is set), then we'll terminate the readable side of the StreamBase. -void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { +int Http2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); Debug(this, "handling data frame for stream %d", id); Http2Stream* stream = FindStream(id); - if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) + if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { stream->EmitRead(UV_EOF); + } else if (frame->hd.length == 0 && + !IsReverted(SECURITY_REVERT_CVE_2019_9518)) { + return 1; // Consider 0-length frame without END_STREAM an error. + } + return 0; } diff --git a/src/node_http2.h b/src/node_http2.h index da2acbca2ca051..6fb8e09751727c 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -889,7 +889,7 @@ class Http2Session : public AsyncWrap, public StreamListener { size_t maxPayloadLen); // Frame Handler - void HandleDataFrame(const nghttp2_frame* frame); + int HandleDataFrame(const nghttp2_frame* frame); void HandleGoawayFrame(const nghttp2_frame* frame); void HandleHeadersFrame(const nghttp2_frame* frame); void HandlePriorityFrame(const nghttp2_frame* frame); diff --git a/src/node_revert.h b/src/node_revert.h index 50ada582de48e3..9646588740a6fd 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -18,6 +18,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ + XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") // TODO(addaleax): Remove all of the above before Node.js 13 as the comment // at the start of the file indicates. From 0ce699c7b120ae7c672f4ffd0dc1562db3dae0a7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 11 Aug 2019 01:31:20 +0200 Subject: [PATCH 15/19] http2: stop reading from socket if writes are in progress If a write to the underlying socket finishes asynchronously, that means that we cannot write any more data at that point without waiting for it to finish. If this happens, we should also not be producing any more input. This is part of mitigating CVE-2019-9511/CVE-2019-9517. 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 | 17 ++++++++++++++++- src/node_http2.h | 2 ++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index c513d286e75d72..e8bf89dd077355 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1574,9 +1574,18 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { Debug(this, "write finished with status %d", status); + CHECK_NE(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); + flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; + // Inform all pending writes about their completion. ClearOutgoing(status); + if ((flags_ & SESSION_STATE_READING_STOPPED) && + nghttp2_session_want_read(session_)) { + flags_ &= ~SESSION_STATE_READING_STOPPED; + stream_->ReadStart(); + } + if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); @@ -1616,10 +1625,13 @@ void Http2Session::MaybeScheduleWrite() { } void Http2Session::MaybeStopReading() { + if (flags_ & SESSION_STATE_READING_STOPPED) return; int want_read = nghttp2_session_want_read(session_); Debug(this, "wants read? %d", want_read); - if (want_read == 0) + if (want_read == 0 || (flags_ & SESSION_STATE_WRITE_IN_PROGRESS)) { + flags_ |= SESSION_STATE_READING_STOPPED; stream_->ReadStop(); + } } // Unset the sending state, finish up all current writes, and reset @@ -1746,8 +1758,11 @@ uint8_t Http2Session::SendPendingData() { chunks_sent_since_last_write_++; + CHECK_EQ(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); + flags_ |= SESSION_STATE_WRITE_IN_PROGRESS; StreamWriteResult res = underlying_stream()->Write(*bufs, count); if (!res.async) { + flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; ClearOutgoing(res.err); } diff --git a/src/node_http2.h b/src/node_http2.h index 6fb8e09751727c..8b393458b05198 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -334,6 +334,8 @@ enum session_state_flags { SESSION_STATE_CLOSED = 0x4, SESSION_STATE_CLOSING = 0x8, SESSION_STATE_SENDING = 0x10, + SESSION_STATE_WRITE_IN_PROGRESS = 0x20, + SESSION_STATE_READING_STOPPED = 0x40, }; typedef uint32_t(*get_setting)(nghttp2_session* session, From c152449012e21dbf1c3e8bd2081600b9f3858549 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 11 Aug 2019 02:22:00 +0200 Subject: [PATCH 16/19] http2: pause input processing if sending output If we are waiting for the ability to send more output, we should not process more input. This commit a) makes us send output earlier, during processing of input, if we accumulate a lot and b) allows interrupting the call into nghttp2 that processes input data and resuming it at a later time, if we do find ourselves in a position where we are waiting to be able to send more output. This is part of mitigating CVE-2019-9511/CVE-2019-9517. 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 | 131 ++++++++++++------ src/node_http2.h | 14 +- ...est-http2-large-write-multiple-requests.js | 39 ++++++ 3 files changed, 140 insertions(+), 44 deletions(-) create mode 100644 test/parallel/test-http2-large-write-multiple-requests.js diff --git a/src/node_http2.cc b/src/node_http2.cc index e8bf89dd077355..6fa552a3156026 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -869,31 +869,52 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, // various callback functions. Each of these will typically result in a call // out to JavaScript so this particular function is rather hot and can be // quite expensive. This is a potential performance optimization target later. -ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) { - size_t total = 0; - // Note that nghttp2_session_mem_recv is a synchronous operation that - // will trigger a number of other callbacks. Those will, in turn have +ssize_t Http2Session::ConsumeHTTP2Data() { + CHECK_NOT_NULL(stream_buf_.base); + CHECK_LT(stream_buf_offset_, stream_buf_.len); + size_t read_len = stream_buf_.len - stream_buf_offset_; + // multiple side effects. - for (size_t n = 0; n < nbufs; n++) { - Debug(this, "receiving %d bytes [wants data? %d]", - bufs[n].len, - nghttp2_session_want_read(session_)); - ssize_t ret = - nghttp2_session_mem_recv(session_, - reinterpret_cast(bufs[n].base), - bufs[n].len); - CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - - if (ret < 0) - return ret; + Debug(this, "receiving %d bytes [wants data? %d]", + read_len, + nghttp2_session_want_read(session_)); + flags_ &= ~SESSION_STATE_NGHTTP2_RECV_PAUSED; + ssize_t ret = + nghttp2_session_mem_recv(session_, + reinterpret_cast(stream_buf_.base) + + stream_buf_offset_, + read_len); + CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - total += ret; + if (flags_ & SESSION_STATE_NGHTTP2_RECV_PAUSED) { + CHECK_NE(flags_ & SESSION_STATE_READING_STOPPED, 0); + + CHECK_GT(ret, 0); + CHECK_LE(static_cast(ret), read_len); + + if (static_cast(ret) < read_len) { + // Mark the remainder of the data as available for later consumption. + stream_buf_offset_ += ret; + return ret; + } } + + // We are done processing the current input chunk. + DecrementCurrentSessionMemory(stream_buf_.len); + stream_buf_offset_ = 0; + stream_buf_ab_.Reset(); + free(stream_buf_allocation_.base); + stream_buf_allocation_ = uv_buf_init(nullptr, 0); + stream_buf_ = uv_buf_init(nullptr, 0); + + if (ret < 0) + return ret; + // Send any data that was queued up while processing the received data. if (!IsDestroyed()) { SendPendingData(); } - return total; + return ret; } @@ -1196,8 +1217,18 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, nghttp2_session_consume_stream(handle, id, avail); else stream->inbound_consumed_data_while_paused_ += avail; + + // If we have a gathered a lot of data for output, try sending it now. + if (session->outgoing_length_ > 4096) session->SendPendingData(); } while (len != 0); + // If we are currently waiting for a write operation to finish, we should + // tell nghttp2 that we want to wait before we process more input data. + if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) { + session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED; + return NGHTTP2_ERR_PAUSE; + } + return 0; } @@ -1289,6 +1320,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { size_t offset = buf.base - session->stream_buf_.base; // Verify that the data offset is inside the current read buffer. + CHECK_GE(offset, session->stream_buf_offset_); CHECK_LE(offset, session->stream_buf_.len); CHECK_LE(offset + buf.len, session->stream_buf_.len); @@ -1586,6 +1618,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { stream_->ReadStart(); } + // If there is more incoming data queued up, consume it. + if (stream_buf_offset_ > 0) { + ConsumeHTTP2Data(); + } + if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); @@ -1643,6 +1680,7 @@ void Http2Session::ClearOutgoing(int status) { if (outgoing_buffers_.size() > 0) { outgoing_storage_.clear(); + outgoing_length_ = 0; std::vector current_outgoing_buffers_; current_outgoing_buffers_.swap(outgoing_buffers_); @@ -1669,6 +1707,11 @@ void Http2Session::ClearOutgoing(int status) { } } +void Http2Session::PushOutgoingBuffer(nghttp2_stream_write&& write) { + outgoing_length_ += write.buf.len; + outgoing_buffers_.emplace_back(std::move(write)); +} + // Queue a given block of data for sending. This always creates a copy, // so it is used for the cases in which nghttp2 requests sending of a // small chunk of data. @@ -1681,7 +1724,7 @@ void Http2Session::CopyDataIntoOutgoing(const uint8_t* src, size_t src_length) { // of the outgoing_buffers_ vector may invalidate the pointer. // The correct base pointers will be set later, before writing to the // underlying socket. - outgoing_buffers_.emplace_back(nghttp2_stream_write { + PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(nullptr, src_length) }); } @@ -1804,13 +1847,13 @@ int Http2Session::OnSendData( if (write.buf.len <= length) { // This write does not suffice by itself, so we can consume it completely. length -= write.buf.len; - session->outgoing_buffers_.emplace_back(std::move(write)); + session->PushOutgoingBuffer(std::move(write)); stream->queue_.pop(); continue; } // Slice off `length` bytes of the first write in the queue. - session->outgoing_buffers_.emplace_back(nghttp2_stream_write { + session->PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(write.buf.base, length) }); write.buf.base += length; @@ -1820,7 +1863,7 @@ int Http2Session::OnSendData( if (frame->data.padlen > 0) { // Send padding if that was requested. - session->outgoing_buffers_.emplace_back(nghttp2_stream_write { + session->PushOutgoingBuffer(nghttp2_stream_write { uv_buf_init(const_cast(zero_bytes_256), frame->data.padlen - 1) }); } @@ -1853,8 +1896,6 @@ 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_.base, nullptr); - CHECK(stream_buf_ab_.IsEmpty()); // Only pass data on if nread > 0 if (nread <= 0) { @@ -1865,26 +1906,33 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { 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); + statistics_.data_received += nread; + + if (UNLIKELY(stream_buf_offset_ > 0)) { + // This is a very unlikely case, and should only happen if the ReadStart() + // call in OnStreamAfterWrite() immediately provides data. If that does + // happen, we concatenate the data we received with the already-stored + // pending input data, slicing off the already processed part. + char* new_buf = Malloc(stream_buf_.len - stream_buf_offset_ + nread); + memcpy(new_buf, + stream_buf_.base + stream_buf_offset_, + stream_buf_.len - stream_buf_offset_); + memcpy(new_buf + stream_buf_.len - stream_buf_offset_, + buf.base, + nread); + free(buf.base); + nread = stream_buf_.len - stream_buf_offset_ + nread; + buf = uv_buf_init(new_buf, nread); + stream_buf_offset_ = 0; stream_buf_ab_.Reset(); - free(stream_buf_allocation_.base); - stream_buf_allocation_ = uv_buf_init(nullptr, 0); - stream_buf_ = uv_buf_init(nullptr, 0); - }); + DecrementCurrentSessionMemory(stream_buf_offset_); + } - // Make sure that there was no read previously active. - CHECK_NULL(stream_buf_.base); - CHECK_EQ(stream_buf_.len, 0); + // Shrink to the actual amount of used data. + buf.base = Realloc(buf.base, nread); + IncrementCurrentSessionMemory(nread); // Remember the current buffer, so that OnDataChunkReceived knows the // offset of a DATA frame's data into the socket read buffer. @@ -1903,8 +1951,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // to copy memory. stream_buf_allocation_ = buf; - statistics_.data_received += nread; - ssize_t ret = Write(&stream_buf_, 1); + ssize_t ret = ConsumeHTTP2Data(); if (UNLIKELY(ret < 0)) { Debug(this, "fatal error receiving data: %d", ret); diff --git a/src/node_http2.h b/src/node_http2.h index 8b393458b05198..1526e0b47e5660 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -336,6 +336,7 @@ enum session_state_flags { SESSION_STATE_SENDING = 0x10, SESSION_STATE_WRITE_IN_PROGRESS = 0x20, SESSION_STATE_READING_STOPPED = 0x40, + SESSION_STATE_NGHTTP2_RECV_PAUSED = 0x80 }; typedef uint32_t(*get_setting)(nghttp2_session* session, @@ -775,14 +776,15 @@ class Http2Session : public AsyncWrap, public StreamListener { // Indicates whether there currently exist outgoing buffers for this stream. bool HasWritesOnSocketForStream(Http2Stream* stream); - // Write data to the session - ssize_t Write(const uv_buf_t* bufs, size_t nbufs); + // Write data from stream_buf_ to the session + ssize_t ConsumeHTTP2Data(); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("streams", streams_); tracker->TrackField("outstanding_pings", outstanding_pings_); tracker->TrackField("outstanding_settings", outstanding_settings_); tracker->TrackField("outgoing_buffers", outgoing_buffers_); + tracker->TrackFieldWithSize("stream_buf", stream_buf_.len); tracker->TrackFieldWithSize("outgoing_storage", outgoing_storage_.size()); tracker->TrackFieldWithSize("pending_rst_streams", pending_rst_streams_.size() * sizeof(int32_t)); @@ -845,6 +847,9 @@ class Http2Session : public AsyncWrap, public StreamListener { } void DecrementCurrentSessionMemory(uint64_t amount) { +#ifdef DEBUG + CHECK_LE(amount, current_session_memory_); +#endif current_session_memory_ -= amount; } @@ -1007,8 +1012,11 @@ class Http2Session : public AsyncWrap, public StreamListener { uint32_t chunks_sent_since_last_write_ = 0; uv_buf_t stream_buf_ = uv_buf_init(nullptr, 0); + // When processing input data, either stream_buf_ab_ or stream_buf_allocation_ + // will be set. stream_buf_ab_ is lazily created from stream_buf_allocation_. v8::Global stream_buf_ab_; uv_buf_t stream_buf_allocation_ = uv_buf_init(nullptr, 0); + size_t stream_buf_offset_ = 0; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; std::queue outstanding_pings_; @@ -1018,6 +1026,7 @@ class Http2Session : public AsyncWrap, public StreamListener { std::vector outgoing_buffers_; std::vector outgoing_storage_; + size_t outgoing_length_ = 0; std::vector pending_rst_streams_; // Count streams that have been rejected while being opened. Exceeding a fixed // limit will result in the session being destroyed, as an indication of a @@ -1027,6 +1036,7 @@ class Http2Session : public AsyncWrap, public StreamListener { // Also use the invalid frame count as a measure for rejecting input frames. int32_t invalid_frame_count_ = 0; + void PushOutgoingBuffer(nghttp2_stream_write&& write); void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); diff --git a/test/parallel/test-http2-large-write-multiple-requests.js b/test/parallel/test-http2-large-write-multiple-requests.js new file mode 100644 index 00000000000000..0d65c3479b409d --- /dev/null +++ b/test/parallel/test-http2-large-write-multiple-requests.js @@ -0,0 +1,39 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const http2 = require('http2'); + +const content = fixtures.readSync('person-large.jpg'); + +const server = http2.createServer({ + maxSessionMemory: 1000 +}); +server.on('stream', (stream, headers) => { + stream.respond({ + 'content-type': 'image/jpeg', + ':status': 200 + }); + stream.end(content); +}); +server.unref(); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}/`); + + let finished = 0; + for (let i = 0; i < 100; i++) { + const req = client.request({ ':path': '/' }).end(); + const chunks = []; + req.on('data', (chunk) => { + chunks.push(chunk); + }); + req.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), content); + if (++finished === 100) client.close(); + })); + } +})); From 0acbe05ee2d0e073e52cfe96a9e701dc9891a360 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Aug 2019 23:36:00 +0200 Subject: [PATCH 17/19] http2: allow security revert for Ping/Settings Flood nghttp2 has updated its limit for outstanding Ping/Settings ACKs to 1000. This commit allows reverting to the old default of 10000. The associated CVEs are CVE-2019-9512/CVE-2019-9515. 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 | 3 +++ src/node_revert.h | 1 + 2 files changed, 4 insertions(+) diff --git a/src/node_http2.cc b/src/node_http2.cc index 6fa552a3156026..43d1c2ea9f277a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -152,6 +152,9 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { buffer[IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS]); } + if (IsReverted(SECURITY_REVERT_CVE_2019_9512)) + nghttp2_option_set_max_outbound_ack(options_, 10000); + // The padding strategy sets the mechanism by which we determine how much // additional frame padding to apply to DATA and HEADERS frames. Currently // this is set on a per-session basis, but eventually we may switch to diff --git a/src/node_revert.h b/src/node_revert.h index 9646588740a6fd..8a3d5c0fe4b36f 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -16,6 +16,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ + XX(CVE_2019_9512, "CVE-2019-9512", "HTTP/2 Ping/Settings Flood") \ XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood") \ XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak") \ XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding") \ From d85e4006ab931c656496a143f03473ebc69eea29 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 13 Aug 2019 17:58:50 +0200 Subject: [PATCH 18/19] test: apply test-http2-max-session-memory-leak from v12.x Refs: https://github.com/nodejs/node/pull/27914 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 --- .../test-http2-max-session-memory-leak.js | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 test/parallel/test-http2-max-session-memory-leak.js diff --git a/test/parallel/test-http2-max-session-memory-leak.js b/test/parallel/test-http2-max-session-memory-leak.js new file mode 100644 index 00000000000000..b066ca80bc5eab --- /dev/null +++ b/test/parallel/test-http2-max-session-memory-leak.js @@ -0,0 +1,46 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); + +// Regression test for https://github.com/nodejs/node/issues/27416. +// Check that received data is accounted for correctly in the maxSessionMemory +// mechanism. + +const bodyLength = 8192; +const maxSessionMemory = 1; // 1 MB +const requestCount = 1000; + +const server = http2.createServer({ maxSessionMemory }); +server.on('stream', (stream) => { + stream.respond(); + stream.end(); +}); + +server.listen(common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`, { + maxSessionMemory + }); + + function request() { + return new Promise((resolve, reject) => { + const stream = client.request({ + ':method': 'POST', + 'content-length': bodyLength + }); + stream.on('error', reject); + stream.on('response', resolve); + stream.end('a'.repeat(bodyLength)); + }); + } + + (async () => { + for (let i = 0; i < requestCount; i++) { + await request(); + } + + client.close(); + server.close(); + })().then(common.mustCall()); +})); From 8994fff6e695a35f298cc53827c64c0fd13b0bf9 Mon Sep 17 00:00:00 2001 From: Beth Griggs Date: Thu, 15 Aug 2019 16:53:20 +0100 Subject: [PATCH 19/19] 2019-08-15, Version 10.17.0 'Dubnium' (LTS) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a security release. Notable changes: Node.js, as well as many other implementations of HTTP/2, have been found vulnerable to Denial of Service attacks. See https://github.com/Netflix/security-bulletins/blob/master/advisories/third-party/2019-002.md for more information. Vulnerabilities fixed: * CVE-2019-9511 “Data Dribble”: The attacker requests a large amount of data from a specified resource over multiple streams. They manipulate window size and stream priority to force the server to queue the data in 1-byte chunks. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. * CVE-2019-9512 “Ping Flood”: The attacker sends continual pings to an HTTP/2 peer, causing the peer to build an internal queue of responses. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. * CVE-2019-9513 “Resource Loop”: The attacker creates multiple request streams and continually shuffles the priority of the streams in a way that causes substantial churn to the priority tree. This can consume excess CPU, potentially leading to a denial of service. * CVE-2019-9514 “Reset Flood”: The attacker opens a number of streams and sends an invalid request over each stream that should solicit a stream of RST_STREAM frames from the peer. Depending on how the peer queues the RST_STREAM frames, this can consume excess memory, CPU,or both, potentially leading to a denial of service. * CVE-2019-9515 “Settings Flood”: The attacker sends a stream of SETTINGS frames to the peer. Since the RFC requires that the peer reply with one acknowledgement per SETTINGS frame, an empty SETTINGS frame is almost equivalent in behavior to a ping. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. * CVE-2019-9516 “0-Length Headers Leak”: The attacker sends a stream of headers with a 0-length header name and 0-length header value, optionally Huffman encoded into 1-byte or greater headers. Some implementations allocate memory for these headers and keep the allocation alive until the session dies. This can consume excess memory, potentially leading to a denial of service. * CVE-2019-9517 “Internal Data Buffering”: The attacker opens the HTTP/2 window so the peer can send without constraint; however, they leave the TCP window closed so the peer cannot actually write (many of) the bytes on the wire. The attacker then sends a stream of requests for a large response object. Depending on how the servers queue the responses, this can consume excess memory, CPU, or both, potentially leading to a denial of service. * CVE-2019-9518 “Empty Frames Flood”: The attacker sends a stream of frames with an empty payload and without the end-of-stream flag. These frames can be DATA, HEADERS, CONTINUATION and/or PUSH_PROMISE. The peer spends time processing each frame disproportionate to attack bandwidth. This can consume excess CPU, potentially leading to a denial of service. (Discovered by Piotr Sikora of Google) PR-URL: --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V10.md | 45 +++++++++++++++++++++++++++++++++ src/node_version.h | 6 ++--- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3d15e48b47155..8888942914eef1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,8 @@ release. 12.0.0
-10.16.2
+10.17.0
+10.16.2
10.16.1
10.16.0
10.15.3
diff --git a/doc/changelogs/CHANGELOG_V10.md b/doc/changelogs/CHANGELOG_V10.md index 4f635b4b09cb73..2dc150b4c6517a 100644 --- a/doc/changelogs/CHANGELOG_V10.md +++ b/doc/changelogs/CHANGELOG_V10.md @@ -10,6 +10,7 @@ +10.17.0
10.16.2
10.16.1
10.16.0
@@ -54,6 +55,50 @@ * [io.js](CHANGELOG_IOJS.md) * [Archive](CHANGELOG_ARCHIVE.md) + +## 2019-08-15, Version 10.17.0 'Dubnium' (LTS), @BethGriggs + +### Notable changes + +This is a security release. + +Node.js, as well as many other implementations of HTTP/2, have been found +vulnerable to Denial of Service attacks. +See https://github.com/Netflix/security-bulletins/blob/master/advisories/third-party/2019-002.md +for more information. + +Vulnerabilities fixed: + +* **CVE-2019-9511 “Data Dribble”**: The attacker requests a large amount of data from a specified resource over multiple streams. They manipulate window size and stream priority to force the server to queue the data in 1-byte chunks. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. +* **CVE-2019-9512 “Ping Flood”**: The attacker sends continual pings to an HTTP/2 peer, causing the peer to build an internal queue of responses. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. +* **CVE-2019-9513 “Resource Loop”**: The attacker creates multiple request streams and continually shuffles the priority of the streams in a way that causes substantial churn to the priority tree. This can consume excess CPU, potentially leading to a denial of service. +* **CVE-2019-9514 “Reset Flood”**: The attacker opens a number of streams and sends an invalid request over each stream that should solicit a stream of RST_STREAM frames from the peer. Depending on how the peer queues the RST_STREAM frames, this can consume excess memory, CPU, or both, potentially leading to a denial of service. +* **CVE-2019-9515 “Settings Flood”**: The attacker sends a stream of SETTINGS frames to the peer. Since the RFC requires that the peer reply with one acknowledgement per SETTINGS frame, an empty SETTINGS frame is almost equivalent in behavior to a ping. Depending on how efficiently this data is queued, this can consume excess CPU, memory, or both, potentially leading to a denial of service. +* **CVE-2019-9516 “0-Length Headers Leak”**: The attacker sends a stream of headers with a 0-length header name and 0-length header value, optionally Huffman encoded into 1-byte or greater headers. Some implementations allocate memory for these headers and keep the allocation alive until the session dies. This can consume excess memory, potentially leading to a denial of service. +* **CVE-2019-9517 “Internal Data Buffering”**: The attacker opens the HTTP/2 window so the peer can send without constraint; however, they leave the TCP window closed so the peer cannot actually write (many of) the bytes on the wire. The attacker then sends a stream of requests for a large response object. Depending on how the servers queue the responses, this can consume excess memory, CPU, or both, potentially leading to a denial of service. +* **CVE-2019-9518 “Empty Frames Flood”**: The attacker sends a stream of frames with an empty payload and without the end-of-stream flag. These frames can be DATA, HEADERS, CONTINUATION and/or PUSH_PROMISE. The peer spends time processing each frame disproportionate to attack bandwidth. This can consume excess CPU, potentially leading to a denial of service. (Discovered by Piotr Sikora of Google) + +### Commits + +* [[`74507fae34`](https://github.com/nodejs/node/commit/74507fae34)] - **deps**: update nghttp2 to 1.39.2 (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`a397c881ec`](https://github.com/nodejs/node/commit/a397c881ec)] - **deps**: update nghttp2 to 1.39.1 (gengjiawen) [#28448](https://github.com/nodejs/node/pull/28448) +* [[`fedfa12a33`](https://github.com/nodejs/node/commit/fedfa12a33)] - **deps**: update nghttp2 to 1.38.0 (gengjiawen) [#27295](https://github.com/nodejs/node/pull/27295) +* [[`ab0f2ace36`](https://github.com/nodejs/node/commit/ab0f2ace36)] - **(SEMVER-MINOR)** **deps**: update nghttp2 to 1.37.0 (gengjiawen) [#26990](https://github.com/nodejs/node/pull/26990) +* [[`0acbe05ee2`](https://github.com/nodejs/node/commit/0acbe05ee2)] - **http2**: allow security revert for Ping/Settings Flood (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`c152449012`](https://github.com/nodejs/node/commit/c152449012)] - **http2**: pause input processing if sending output (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`0ce699c7b1`](https://github.com/nodejs/node/commit/0ce699c7b1)] - **http2**: stop reading from socket if writes are in progress (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`17357d37a9`](https://github.com/nodejs/node/commit/17357d37a9)] - **http2**: consider 0-length non-end DATA frames an error (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`460f896c63`](https://github.com/nodejs/node/commit/460f896c63)] - **http2**: shrink default `vector::reserve()` allocations (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`f4242e24f9`](https://github.com/nodejs/node/commit/f4242e24f9)] - **http2**: handle 0-length headers better (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`477461a51f`](https://github.com/nodejs/node/commit/477461a51f)] - **http2**: limit number of invalid incoming frames (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`05dada46ee`](https://github.com/nodejs/node/commit/05dada46ee)] - **http2**: limit number of rejected stream openings (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`7f11465572`](https://github.com/nodejs/node/commit/7f11465572)] - **http2**: do not create ArrayBuffers when no DATA received (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`2eb914ff5f`](https://github.com/nodejs/node/commit/2eb914ff5f)] - **http2**: only call into JS when necessary for session events (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`76a7ada15d`](https://github.com/nodejs/node/commit/76a7ada15d)] - **http2**: improve JS-side debug logging (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) +* [[`00f153da13`](https://github.com/nodejs/node/commit/00f153da13)] - **http2**: improve http2 code a bit (James M Snell) [#23984](https://github.com/nodejs/node/pull/23984) +* [[`a0a14c809f`](https://github.com/nodejs/node/commit/a0a14c809f)] - **src**: pass along errors from http2 object creation (Anna Henningsen) [#25822](https://github.com/nodejs/node/pull/25822) +* [[`d85e4006ab`](https://github.com/nodejs/node/commit/d85e4006ab)] - **test**: apply test-http2-max-session-memory-leak from v12.x (Anna Henningsen) [#29122](https://github.com/nodejs/node/pull/29122) + ## 2019-08-06, Version 10.16.2 'Dubnium' (LTS), @BethGriggs diff --git a/src/node_version.h b/src/node_version.h index ec74dcdbb61691..d5d604c28f509f 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -23,13 +23,13 @@ #define SRC_NODE_VERSION_H_ #define NODE_MAJOR_VERSION 10 -#define NODE_MINOR_VERSION 16 -#define NODE_PATCH_VERSION 3 +#define NODE_MINOR_VERSION 17 +#define NODE_PATCH_VERSION 0 #define NODE_VERSION_IS_LTS 1 #define NODE_VERSION_LTS_CODENAME "Dubnium" -#define NODE_VERSION_IS_RELEASE 0 +#define NODE_VERSION_IS_RELEASE 1 #ifndef NODE_STRINGIFY #define NODE_STRINGIFY(n) NODE_STRINGIFY_HELPER(n)