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.
nodejs · addaleax · Oct 30, 2018 · Jan 30, 2019 · Mar 29, 2019 · Apr 18, 2019
commit 3b76bb61aab35ddc2fcef73f02b552758d34e445
diff --git 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
@@ -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
@@ -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<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));
+  }
 }
 
 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> 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> 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<const char*>(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<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
@@ -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_;