From dd60d3561a029f862a988d90a0e5ec261e8c3e5d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Aug 2019 00:51:51 +0200 Subject: [PATCH] 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/29124 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 | 33 +++++++++- src/node_http2.h | 20 +++++++ 4 files changed, 163 insertions(+), 10 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 85625f9ef30d5a..70974ecb74a7e8 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -108,6 +108,7 @@ const kInit = Symbol('init'); const kInfoHeaders = Symbol('sent-info-headers'); const kMaybeDestroy = Symbol('maybe-destroy'); const kLocalSettings = Symbol('local-settings'); +const kNativeFields = Symbol('kNativeFields'); const kOptions = Symbol('options'); const kOwner = Symbol('owner'); const kOrigin = Symbol('origin'); @@ -130,7 +131,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 { @@ -305,6 +314,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) @@ -394,7 +473,6 @@ function onSettings() { return; session[kUpdateTimer](); debugSessionObj(session, 'new settings received'); - session[kRemoteSettings] = undefined; session.emit('remoteSettings', session.remoteSettings); } @@ -845,6 +923,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; @@ -886,6 +968,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; @@ -963,6 +1046,7 @@ class Http2Session extends EventEmitter { this[kType] = type; this[kProxySocket] = null; this[kSocket] = socket; + this[kHandle] = undefined; // Do not use nagle's algorithm if (typeof socket.setNoDelay === 'function') @@ -981,6 +1065,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'); } @@ -1136,13 +1225,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 } @@ -1330,6 +1424,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() { @@ -1668,6 +1768,9 @@ class Http2Stream extends Duplex { }; this.on('pause', streamOnPause); + + this.on('newListener', streamListenerAdded); + this.on('removeListener', streamListenerRemoved); } [kUpdateTimer]() { @@ -2620,7 +2723,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) { } function sessionOnError(error) { - if (this[kServer]) + if (this[kServer] !== undefined) this[kServer].emit('sessionError', error, this); } @@ -2669,8 +2772,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 b47d55d87b3e69..fc2af3dfd02558 100644 --- a/src/env.h +++ b/src/env.h @@ -161,6 +161,7 @@ class ModuleWrap; V(family_string, "family") \ V(fatal_exception_string, "_fatalException") \ V(fd_string, "fd") \ + V(fields_string, "fields") \ V(file_string, "file") \ V(fingerprint_string, "fingerprint") \ V(flags_string, "flags") \ diff --git a/src/node_http2.cc b/src/node_http2.cc index 973bc36b303523..bc5584797f9faa 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -20,6 +20,7 @@ using v8::ObjectTemplate; using v8::String; using v8::Uint32; using v8::Uint32Array; +using v8::Uint8Array; using v8::Undefined; using node::performance::PerformanceEntry; @@ -667,6 +668,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)); + } } void Http2Session::Unconsume() { @@ -1090,7 +1100,8 @@ inline 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; } @@ -1346,7 +1357,8 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // received. Notifies JS land about the priority change. Note that priorities // are considered advisory only, so this has no real effect other than to // simply let user code know that the priority has changed. -inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { +void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { + if (js_fields_[kSessionPriorityListenerCount] == 0) return; Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1413,7 +1425,8 @@ inline void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { } // Called by OnFrameReceived when a complete ALTSVC frame has been received. -inline void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { +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(); @@ -1499,6 +1512,7 @@ inline 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), @@ -1510,6 +1524,9 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { inline 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; @@ -3135,6 +3152,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 a4e37562f3f313..9e4a83d0a50b39 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -735,6 +735,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: @@ -1016,6 +1033,9 @@ class Http2Session : public AsyncWrap { // 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_;