http2: only call into JS when necessary for session events

addaleax · BethGriggs · commit dd60d3561a02 · 2019-08-15T17:36:36.000+01:00
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: #29124 PR-URL: #29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git 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
@@ -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
@@ -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<ArrayBuffer> ab =
+        ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount);
+    Local<Uint8Array> 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> 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> 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<const char*>(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<Object> 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
@@ -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_;
 
