Skip to content

Commit

Permalink
http2: only call into JS when necessary for session events
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
@addaleax @BethGriggs
addaleax authored and BethGriggs committed Aug 15, 2019
1 parent 00f6846 commit dd60d35
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 10 deletions.
119 changes: 112 additions & 7 deletions lib/internal/http2/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -394,7 +473,6 @@ function onSettings() {
return;
session[kUpdateTimer]();
debugSessionObj(session, 'new settings received');
session[kRemoteSettings] = undefined;
session.emit('remoteSettings', session.remoteSettings);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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')
Expand All @@ -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');
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1668,6 +1768,9 @@ class Http2Stream extends Duplex {
};

this.on('pause', streamOnPause);

this.on('newListener', streamListenerAdded);
this.on('removeListener', streamListenerRemoved);
}

[kUpdateTimer]() {
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
33 changes: 30 additions & 3 deletions src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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),
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand Down
20 changes: 20 additions & 0 deletions src/node_http2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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_;

Expand Down

0 comments on commit dd60d35

Please sign in to comment.