Skip to content

Commit c44ee7a

Browse files
addaleaxtargos
addaleax
authored and
targos
committed
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: nodejs#29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 8dae8d1 commit c44ee7a

4 files changed

Lines changed: 161 additions & 8 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ const kID = Symbol('id');
177177
const kInit = Symbol('init');
178178
const kInfoHeaders = Symbol('sent-info-headers');
179179
const kLocalSettings = Symbol('local-settings');
180+
const kNativeFields = Symbol('kNativeFields');
180181
const kOptions = Symbol('options');
181182
const kOwner = owner_symbol;
182183
const kOrigin = Symbol('origin');
@@ -196,7 +197,15 @@ const {
196197
paddingBuffer,
197198
PADDING_BUF_FRAME_LENGTH,
198199
PADDING_BUF_MAX_PAYLOAD_LENGTH,
199-
PADDING_BUF_RETURN_VALUE
200+
PADDING_BUF_RETURN_VALUE,
201+
kBitfield,
202+
kSessionPriorityListenerCount,
203+
kSessionFrameErrorListenerCount,
204+
kSessionUint8FieldCount,
205+
kSessionHasRemoteSettingsListeners,
206+
kSessionRemoteSettingsIsUpToDate,
207+
kSessionHasPingListeners,
208+
kSessionHasAltsvcListeners,
200209
} = binding;
201210

202211
const {
@@ -372,6 +381,76 @@ function submitRstStream(code) {
372381
}
373382
}
374383

384+
// Keep track of the number/presence of JS event listeners. Knowing that there
385+
// are no listeners allows the C++ code to skip calling into JS for an event.
386+
function sessionListenerAdded(name) {
387+
switch (name) {
388+
case 'ping':
389+
this[kNativeFields][kBitfield] |= 1 << kSessionHasPingListeners;
390+
break;
391+
case 'altsvc':
392+
this[kNativeFields][kBitfield] |= 1 << kSessionHasAltsvcListeners;
393+
break;
394+
case 'remoteSettings':
395+
this[kNativeFields][kBitfield] |= 1 << kSessionHasRemoteSettingsListeners;
396+
break;
397+
case 'priority':
398+
this[kNativeFields][kSessionPriorityListenerCount]++;
399+
break;
400+
case 'frameError':
401+
this[kNativeFields][kSessionFrameErrorListenerCount]++;
402+
break;
403+
}
404+
}
405+
406+
function sessionListenerRemoved(name) {
407+
switch (name) {
408+
case 'ping':
409+
if (this.listenerCount(name) > 0) return;
410+
this[kNativeFields][kBitfield] &= ~(1 << kSessionHasPingListeners);
411+
break;
412+
case 'altsvc':
413+
if (this.listenerCount(name) > 0) return;
414+
this[kNativeFields][kBitfield] &= ~(1 << kSessionHasAltsvcListeners);
415+
break;
416+
case 'remoteSettings':
417+
if (this.listenerCount(name) > 0) return;
418+
this[kNativeFields][kBitfield] &=
419+
~(1 << kSessionHasRemoteSettingsListeners);
420+
break;
421+
case 'priority':
422+
this[kNativeFields][kSessionPriorityListenerCount]--;
423+
break;
424+
case 'frameError':
425+
this[kNativeFields][kSessionFrameErrorListenerCount]--;
426+
break;
427+
}
428+
}
429+
430+
// Also keep track of listeners for the Http2Stream instances, as some events
431+
// are emitted on those objects.
432+
function streamListenerAdded(name) {
433+
switch (name) {
434+
case 'priority':
435+
this[kSession][kNativeFields][kSessionPriorityListenerCount]++;
436+
break;
437+
case 'frameError':
438+
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++;
439+
break;
440+
}
441+
}
442+
443+
function streamListenerRemoved(name) {
444+
switch (name) {
445+
case 'priority':
446+
this[kSession][kNativeFields][kSessionPriorityListenerCount]--;
447+
break;
448+
case 'frameError':
449+
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--;
450+
break;
451+
}
452+
}
453+
375454
function onPing(payload) {
376455
const session = this[kOwner];
377456
if (session.destroyed)
@@ -430,7 +509,6 @@ function onSettings() {
430509
return;
431510
session[kUpdateTimer]();
432511
debugSessionObj(session, 'new settings received');
433-
session[kRemoteSettings] = undefined;
434512
session.emit('remoteSettings', session.remoteSettings);
435513
}
436514

@@ -854,6 +932,10 @@ function setupHandle(socket, type, options) {
854932
handle.consume(socket._handle);
855933

856934
this[kHandle] = handle;
935+
if (this[kNativeFields])
936+
handle.fields.set(this[kNativeFields]);
937+
else
938+
this[kNativeFields] = handle.fields;
857939

858940
if (socket.encrypted) {
859941
this[kAlpnProtocol] = socket.alpnProtocol;
@@ -895,6 +977,7 @@ function finishSessionDestroy(session, error) {
895977
session[kProxySocket] = undefined;
896978
session[kSocket] = undefined;
897979
session[kHandle] = undefined;
980+
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
898981
socket[kSession] = undefined;
899982
socket[kServer] = undefined;
900983

@@ -974,6 +1057,7 @@ class Http2Session extends EventEmitter {
9741057
this[kProxySocket] = null;
9751058
this[kSocket] = socket;
9761059
this[kTimeout] = null;
1060+
this[kHandle] = undefined;
9771061

9781062
// Do not use nagle's algorithm
9791063
if (typeof socket.setNoDelay === 'function')
@@ -998,6 +1082,11 @@ class Http2Session extends EventEmitter {
9981082
setupFn();
9991083
}
10001084

1085+
if (!this[kNativeFields])
1086+
this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
1087+
this.on('newListener', sessionListenerAdded);
1088+
this.on('removeListener', sessionListenerRemoved);
1089+
10011090
debugSession(type, 'created');
10021091
}
10031092

@@ -1155,13 +1244,18 @@ class Http2Session extends EventEmitter {
11551244

11561245
// The settings currently in effect for the remote peer.
11571246
get remoteSettings() {
1158-
const settings = this[kRemoteSettings];
1159-
if (settings !== undefined)
1160-
return settings;
1247+
if (this[kNativeFields][kBitfield] &
1248+
(1 << kSessionRemoteSettingsIsUpToDate)) {
1249+
const settings = this[kRemoteSettings];
1250+
if (settings !== undefined) {
1251+
return settings;
1252+
}
1253+
}
11611254

11621255
if (this.destroyed || this.connecting)
11631256
return {};
11641257

1258+
this[kNativeFields][kBitfield] |= (1 << kSessionRemoteSettingsIsUpToDate);
11651259
return this[kRemoteSettings] = getSettings(this[kHandle], true); // Remote
11661260
}
11671261

@@ -1340,6 +1434,12 @@ class ServerHttp2Session extends Http2Session {
13401434
constructor(options, socket, server) {
13411435
super(NGHTTP2_SESSION_SERVER, options, socket);
13421436
this[kServer] = server;
1437+
// This is a bit inaccurate because it does not reflect changes to
1438+
// number of listeners made after the session was created. This should
1439+
// not be an issue in practice. Additionally, the 'priority' event on
1440+
// server instances (or any other object) is fully undocumented.
1441+
this[kNativeFields][kSessionPriorityListenerCount] =
1442+
server.listenerCount('priority');
13431443
}
13441444

13451445
get server() {
@@ -1652,6 +1752,9 @@ class Http2Stream extends Duplex {
16521752
this[kProxySocket] = null;
16531753

16541754
this.on('pause', streamOnPause);
1755+
1756+
this.on('newListener', streamListenerAdded);
1757+
this.on('removeListener', streamListenerRemoved);
16551758
}
16561759

16571760
[kUpdateTimer]() {
@@ -2608,7 +2711,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) {
26082711
}
26092712

26102713
function sessionOnError(error) {
2611-
if (this[kServer])
2714+
if (this[kServer] !== undefined)
26122715
this[kServer].emit('sessionError', error, this);
26132716
}
26142717

@@ -2657,8 +2760,10 @@ function connectionListener(socket) {
26572760
const session = new ServerHttp2Session(options, socket, this);
26582761

26592762
session.on('stream', sessionOnStream);
2660-
session.on('priority', sessionOnPriority);
26612763
session.on('error', sessionOnError);
2764+
// Don't count our own internal listener.
2765+
session.on('priority', sessionOnPriority);
2766+
session[kNativeFields][kSessionPriorityListenerCount]--;
26622767

26632768
if (this.timeout)
26642769
session.setTimeout(this.timeout, sessionOnTimeout);

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ constexpr size_t kFsStatsBufferLength =
230230
V(family_string, "family") \
231231
V(fatal_exception_string, "_fatalException") \
232232
V(fd_string, "fd") \
233+
V(fields_string, "fields") \
233234
V(file_string, "file") \
234235
V(fingerprint256_string, "fingerprint256") \
235236
V(fingerprint_string, "fingerprint") \

‎src/node_http2.cc‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ using v8::ObjectTemplate;
2525
using v8::String;
2626
using v8::Uint32;
2727
using v8::Uint32Array;
28+
using v8::Uint8Array;
2829
using v8::Undefined;
2930

3031
using node::performance::PerformanceEntry;
@@ -639,6 +640,15 @@ Http2Session::Http2Session(Environment* env,
639640

640641
outgoing_storage_.reserve(4096);
641642
outgoing_buffers_.reserve(32);
643+
644+
{
645+
// Make the js_fields_ property accessible to JS land.
646+
Local<ArrayBuffer> ab =
647+
ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount);
648+
Local<Uint8Array> uint8_arr =
649+
Uint8Array::New(ab, 0, kSessionUint8FieldCount);
650+
USE(wrap->Set(env->context(), env->fields_string(), uint8_arr));
651+
}
642652
}
643653

644654
Http2Session::~Http2Session() {
@@ -1033,7 +1043,8 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
10331043
// Do not report if the frame was not sent due to the session closing
10341044
if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
10351045
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
1036-
error_code == NGHTTP2_ERR_STREAM_CLOSING) {
1046+
error_code == NGHTTP2_ERR_STREAM_CLOSING ||
1047+
session->js_fields_[kSessionFrameErrorListenerCount] == 0) {
10371048
return 0;
10381049
}
10391050

@@ -1316,6 +1327,7 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
13161327
// are considered advisory only, so this has no real effect other than to
13171328
// simply let user code know that the priority has changed.
13181329
void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
1330+
if (js_fields_[kSessionPriorityListenerCount] == 0) return;
13191331
Isolate* isolate = env()->isolate();
13201332
HandleScope scope(isolate);
13211333
Local<Context> context = env()->context();
@@ -1380,6 +1392,7 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
13801392

13811393
// Called by OnFrameReceived when a complete ALTSVC frame has been received.
13821394
void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {
1395+
if (!(js_fields_[kBitfield] & (1 << kSessionHasAltsvcListeners))) return;
13831396
Isolate* isolate = env()->isolate();
13841397
HandleScope scope(isolate);
13851398
Local<Context> context = env()->context();
@@ -1458,6 +1471,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14581471
return;
14591472
}
14601473

1474+
if (!(js_fields_[kBitfield] & (1 << kSessionHasPingListeners))) return;
14611475
// Notify the session that a ping occurred
14621476
arg = Buffer::Copy(env(),
14631477
reinterpret_cast<const char*>(frame->ping.opaque_data),
@@ -1469,6 +1483,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14691483
void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
14701484
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
14711485
if (!ack) {
1486+
js_fields_[kBitfield] &= ~(1 << kSessionRemoteSettingsIsUpToDate);
1487+
if (!(js_fields_[kBitfield] & (1 << kSessionHasRemoteSettingsListeners)))
1488+
return;
14721489
// This is not a SETTINGS acknowledgement, notify and return
14731490
MakeCallback(env()->http2session_on_settings_function(), 0, nullptr);
14741491
return;
@@ -2981,6 +2998,16 @@ void Initialize(Local<Object> target,
29812998
NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH);
29822999
NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE);
29833000

3001+
NODE_DEFINE_CONSTANT(target, kBitfield);
3002+
NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount);
3003+
NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount);
3004+
NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount);
3005+
3006+
NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners);
3007+
NODE_DEFINE_CONSTANT(target, kSessionRemoteSettingsIsUpToDate);
3008+
NODE_DEFINE_CONSTANT(target, kSessionHasPingListeners);
3009+
NODE_DEFINE_CONSTANT(target, kSessionHasAltsvcListeners);
3010+
29843011
// Method to fetch the nghttp2 string description of an nghttp2 error code
29853012
env->SetMethod(target, "nghttp2ErrorString", HttpErrorString);
29863013

‎src/node_http2.h‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,23 @@ class Http2Stream::Provider::Stream : public Http2Stream::Provider {
672672
void* user_data);
673673
};
674674

675+
// Indices for js_fields_, which serves as a way to communicate data with JS
676+
// land fast. In particular, we store information about the number/presence
677+
// of certain event listeners in JS, and skip calls from C++ into JS if they
678+
// are missing.
679+
enum SessionUint8Fields {
680+
kBitfield, // See below
681+
kSessionPriorityListenerCount,
682+
kSessionFrameErrorListenerCount,
683+
kSessionUint8FieldCount
684+
};
685+
686+
enum SessionBitfieldFlags {
687+
kSessionHasRemoteSettingsListeners,
688+
kSessionRemoteSettingsIsUpToDate,
689+
kSessionHasPingListeners,
690+
kSessionHasAltsvcListeners
691+
};
675692

676693
class Http2Session : public AsyncWrap, public StreamListener {
677694
public:
@@ -949,6 +966,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
949966
// The underlying nghttp2_session handle
950967
nghttp2_session* session_;
951968

969+
// JS-accessible numeric fields, as indexed by SessionUint8Fields.
970+
uint8_t js_fields_[kSessionUint8FieldCount] = {};
971+
952972
// The session type: client or server
953973
nghttp2_session_type session_type_;
954974

0 commit comments

Comments
 (0)