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.

PR-URL: https://github.com/nodejs/node/pull/29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 112 insertions, 7 deletions

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index ab0a64e73e..69666bb6ba 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -177,6 +177,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');
@@ -196,7 +197,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 {
@@ -372,6 +381,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)
@@ -430,7 +509,6 @@ function onSettings() {
     return;
   session[kUpdateTimer]();
   debugSessionObj(session, 'new settings received');
-  session[kRemoteSettings] = undefined;
   session.emit('remoteSettings', session.remoteSettings);
 }
 
@@ -854,6 +932,10 @@ function setupHandle(socket, type, options) {
   handle.consume(socket._handle);
 
   this[kHandle] = handle;
+  if (this[kNativeFields])
+    handle.fields.set(this[kNativeFields]);
+  else
+    this[kNativeFields] = handle.fields;
 
   if (socket.encrypted) {
     this[kAlpnProtocol] = socket.alpnProtocol;
@@ -895,6 +977,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;
 
@@ -974,6 +1057,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')
@@ -998,6 +1082,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');
   }
 
@@ -1155,13 +1244,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
   }
 
@@ -1340,6 +1434,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() {
@@ -1652,6 +1752,9 @@ class Http2Stream extends Duplex {
     this[kProxySocket] = null;
 
     this.on('pause', streamOnPause);
+
+    this.on('newListener', streamListenerAdded);
+    this.on('removeListener', streamListenerRemoved);
   }
 
   [kUpdateTimer]() {
@@ -2608,7 +2711,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) {
 }
 
 function sessionOnError(error) {
-  if (this[kServer])
+  if (this[kServer] !== undefined)
     this[kServer].emit('sessionError', error, this);
 }
 
@@ -2657,8 +2760,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);