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.

PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@addaleax @targos
addaleax authored and targos committed Aug 15, 2019
1 parent 58d8c9e commit 530004e
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 8 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 @@ -178,6 +178,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');
Expand All @@ -200,7 +201,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 @@ -376,6 +385,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 @@ -434,7 +513,6 @@ function onSettings() {
return;
session[kUpdateTimer]();
debugSessionObj(session, 'new settings received');
session[kRemoteSettings] = undefined;
session.emit('remoteSettings', session.remoteSettings);
}

Expand Down Expand Up @@ -858,6 +936,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;
Expand Down Expand Up @@ -899,6 +981,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 @@ -978,6 +1061,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')
Expand All @@ -1002,6 +1086,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 @@ -1159,13 +1248,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 @@ -1344,6 +1438,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 @@ -1656,6 +1756,9 @@ class Http2Stream extends Duplex {
this[kProxySocket] = null;

this.on('pause', streamOnPause);

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

[kUpdateTimer]() {
Expand Down Expand Up @@ -2612,7 +2715,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 @@ -2661,8 +2764,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 @@ -208,6 +208,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
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") \
Expand Down
29 changes: 28 additions & 1 deletion src/node_http2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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 @@ -639,6 +640,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() {
Expand Down Expand Up @@ -1033,7 +1043,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;
}

Expand Down Expand Up @@ -1316,6 +1327,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();
Expand Down Expand Up @@ -1380,6 +1392,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();
Expand Down Expand Up @@ -1458,6 +1471,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),
Expand All @@ -1469,6 +1483,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()->http2session_on_settings_function(), 0, nullptr);
return;
Expand Down Expand Up @@ -2981,6 +2998,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 @@ -672,6 +672,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:
Expand Down Expand Up @@ -949,6 +966,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_;

Expand Down

0 comments on commit 530004e

Please sign in to comment.