Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 2eb914f

Browse filesBrowse files
addaleaxBethGriggs
addaleax
authored and
BethGriggs
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. Backport-PR-URL: #29123 PR-URL: #29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 76a7ada commit 2eb914f
Copy full SHA for 2eb914f

File tree

Expand file treeCollapse file tree

4 files changed

+161
-8
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+161
-8
lines changed
Open diff view settings
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+112-7Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ const kID = Symbol('id');
167167
const kInit = Symbol('init');
168168
const kInfoHeaders = Symbol('sent-info-headers');
169169
const kLocalSettings = Symbol('local-settings');
170+
const kNativeFields = Symbol('kNativeFields');
170171
const kOptions = Symbol('options');
171172
const kOwner = owner_symbol;
172173
const kOrigin = Symbol('origin');
@@ -188,7 +189,15 @@ const {
188189
paddingBuffer,
189190
PADDING_BUF_FRAME_LENGTH,
190191
PADDING_BUF_MAX_PAYLOAD_LENGTH,
191-
PADDING_BUF_RETURN_VALUE
192+
PADDING_BUF_RETURN_VALUE,
193+
kBitfield,
194+
kSessionPriorityListenerCount,
195+
kSessionFrameErrorListenerCount,
196+
kSessionUint8FieldCount,
197+
kSessionHasRemoteSettingsListeners,
198+
kSessionRemoteSettingsIsUpToDate,
199+
kSessionHasPingListeners,
200+
kSessionHasAltsvcListeners,
192201
} = binding;
193202

194203
const {
@@ -364,6 +373,76 @@ function submitRstStream(code) {
364373
}
365374
}
366375

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

@@ -863,6 +941,10 @@ function setupHandle(socket, type, options) {
863941
handle.consume(socket._handle._externalStream);
864942

865943
this[kHandle] = handle;
944+
if (this[kNativeFields])
945+
handle.fields.set(this[kNativeFields]);
946+
else
947+
this[kNativeFields] = handle.fields;
866948

867949
if (socket.encrypted) {
868950
this[kAlpnProtocol] = socket.alpnProtocol;
@@ -904,6 +986,7 @@ function finishSessionDestroy(session, error) {
904986
session[kProxySocket] = undefined;
905987
session[kSocket] = undefined;
906988
session[kHandle] = undefined;
989+
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
907990
socket[kSession] = undefined;
908991
socket[kServer] = undefined;
909992

@@ -982,6 +1065,7 @@ class Http2Session extends EventEmitter {
9821065
this[kProxySocket] = null;
9831066
this[kSocket] = socket;
9841067
this[kTimeout] = null;
1068+
this[kHandle] = undefined;
9851069

9861070
// Do not use nagle's algorithm
9871071
if (typeof socket.setNoDelay === 'function')
@@ -1000,6 +1084,11 @@ class Http2Session extends EventEmitter {
10001084
setupFn();
10011085
}
10021086

1087+
if (!this[kNativeFields])
1088+
this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
1089+
this.on('newListener', sessionListenerAdded);
1090+
this.on('removeListener', sessionListenerRemoved);
1091+
10031092
debugSession(type, 'created');
10041093
}
10051094

@@ -1154,13 +1243,18 @@ class Http2Session extends EventEmitter {
11541243

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

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

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

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

13481448
get server() {
@@ -1660,6 +1760,9 @@ class Http2Stream extends Duplex {
16601760
};
16611761

16621762
this.on('pause', streamOnPause);
1763+
1764+
this.on('newListener', streamListenerAdded);
1765+
this.on('removeListener', streamListenerRemoved);
16631766
}
16641767

16651768
[kUpdateTimer]() {
@@ -2633,7 +2736,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) {
26332736
}
26342737

26352738
function sessionOnError(error) {
2636-
if (this[kServer])
2739+
if (this[kServer] !== undefined)
26372740
this[kServer].emit('sessionError', error, this);
26382741
}
26392742

@@ -2682,8 +2785,10 @@ function connectionListener(socket) {
26822785
const session = new ServerHttp2Session(options, socket, this);
26832786

26842787
session.on('stream', sessionOnStream);
2685-
session.on('priority', sessionOnPriority);
26862788
session.on('error', sessionOnError);
2789+
// Don't count our own internal listener.
2790+
session.on('priority', sessionOnPriority);
2791+
session[kNativeFields][kSessionPriorityListenerCount]--;
26872792

26882793
if (this.timeout)
26892794
session.setTimeout(this.timeout, sessionOnTimeout);
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ struct PackageConfig {
173173
V(family_string, "family") \
174174
V(fatal_exception_string, "_fatalException") \
175175
V(fd_string, "fd") \
176+
V(fields_string, "fields") \
176177
V(file_string, "file") \
177178
V(fingerprint256_string, "fingerprint256") \
178179
V(fingerprint_string, "fingerprint") \
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+28-1Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ using v8::ObjectTemplate;
2424
using v8::String;
2525
using v8::Uint32;
2626
using v8::Uint32Array;
27+
using v8::Uint8Array;
2728
using v8::Undefined;
2829

2930
using node::performance::PerformanceEntry;
@@ -631,6 +632,15 @@ Http2Session::Http2Session(Environment* env,
631632

632633
outgoing_storage_.reserve(4096);
633634
outgoing_buffers_.reserve(32);
635+
636+
{
637+
// Make the js_fields_ property accessible to JS land.
638+
Local<ArrayBuffer> ab =
639+
ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount);
640+
Local<Uint8Array> uint8_arr =
641+
Uint8Array::New(ab, 0, kSessionUint8FieldCount);
642+
USE(wrap->Set(env->context(), env->fields_string(), uint8_arr));
643+
}
634644
}
635645

636646
Http2Session::~Http2Session() {
@@ -1032,7 +1042,8 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
10321042
// Do not report if the frame was not sent due to the session closing
10331043
if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
10341044
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
1035-
error_code == NGHTTP2_ERR_STREAM_CLOSING) {
1045+
error_code == NGHTTP2_ERR_STREAM_CLOSING ||
1046+
session->js_fields_[kSessionFrameErrorListenerCount] == 0) {
10361047
return 0;
10371048
}
10381049

@@ -1337,6 +1348,7 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
13371348
// are considered advisory only, so this has no real effect other than to
13381349
// simply let user code know that the priority has changed.
13391350
void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
1351+
if (js_fields_[kSessionPriorityListenerCount] == 0) return;
13401352
Isolate* isolate = env()->isolate();
13411353
HandleScope scope(isolate);
13421354
Local<Context> context = env()->context();
@@ -1399,6 +1411,7 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
13991411

14001412
// Called by OnFrameReceived when a complete ALTSVC frame has been received.
14011413
void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {
1414+
if (!(js_fields_[kBitfield] & (1 << kSessionHasAltsvcListeners))) return;
14021415
Isolate* isolate = env()->isolate();
14031416
HandleScope scope(isolate);
14041417
Local<Context> context = env()->context();
@@ -1484,6 +1497,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14841497
return;
14851498
}
14861499

1500+
if (!(js_fields_[kBitfield] & (1 << kSessionHasPingListeners))) return;
14871501
// Notify the session that a ping occurred
14881502
arg = Buffer::Copy(env(),
14891503
reinterpret_cast<const char*>(frame->ping.opaque_data),
@@ -1495,6 +1509,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14951509
void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
14961510
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
14971511
if (!ack) {
1512+
js_fields_[kBitfield] &= ~(1 << kSessionRemoteSettingsIsUpToDate);
1513+
if (!(js_fields_[kBitfield] & (1 << kSessionHasRemoteSettingsListeners)))
1514+
return;
14981515
// This is not a SETTINGS acknowledgement, notify and return
14991516
MakeCallback(env()->onsettings_string(), 0, nullptr);
15001517
return;
@@ -2980,6 +2997,16 @@ void Initialize(Local<Object> target,
29802997
NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH);
29812998
NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE);
29822999

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

Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+20Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,23 @@ class Http2Stream::Provider::Stream : public Http2Stream::Provider {
675675
void* user_data);
676676
};
677677

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

679696
class Http2Session : public AsyncWrap, public StreamListener {
680697
public:
@@ -961,6 +978,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
961978
// The underlying nghttp2_session handle
962979
nghttp2_session* session_;
963980

981+
// JS-accessible numeric fields, as indexed by SessionUint8Fields.
982+
uint8_t js_fields_[kSessionUint8FieldCount] = {};
983+
964984
// The session type: client or server
965985
nghttp2_session_type session_type_;
966986

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.