FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: only call into JS when necessary for session events · nodejs/node@dd60d35 · GitHub

/ node Public

Commit dd60d35

Browse files
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: #29124 PR-URL: #29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 00f6846 commit dd60d35

4 files changed

Lines changed: 163 additions & 10 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ const kInit = Symbol('init');
108108
const kInfoHeaders = Symbol('sent-info-headers');
109109
const kMaybeDestroy = Symbol('maybe-destroy');
110110
const kLocalSettings = Symbol('local-settings');
111+
const kNativeFields = Symbol('kNativeFields');
111112
const kOptions = Symbol('options');
112113
const kOwner = Symbol('owner');
113114
const kOrigin = Symbol('origin');
@@ -130,7 +131,15 @@ const {
130131
paddingBuffer,
131132
PADDING_BUF_FRAME_LENGTH,
132133
PADDING_BUF_MAX_PAYLOAD_LENGTH,
133-
PADDING_BUF_RETURN_VALUE
134+
PADDING_BUF_RETURN_VALUE,
135+
kBitfield,
136+
kSessionPriorityListenerCount,
137+
kSessionFrameErrorListenerCount,
138+
kSessionUint8FieldCount,
139+
kSessionHasRemoteSettingsListeners,
140+
kSessionRemoteSettingsIsUpToDate,
141+
kSessionHasPingListeners,
142+
kSessionHasAltsvcListeners,
134143
} = binding;
135144

136145
const {
@@ -305,6 +314,76 @@ function submitRstStream(code) {
305314
}
306315
}
307316

317+
// Keep track of the number/presence of JS event listeners. Knowing that there
318+
// are no listeners allows the C++ code to skip calling into JS for an event.
319+
function sessionListenerAdded(name) {
320+
switch (name) {
321+
case 'ping':
322+
this[kNativeFields][kBitfield] |= 1 << kSessionHasPingListeners;
323+
break;
324+
case 'altsvc':
325+
this[kNativeFields][kBitfield] |= 1 << kSessionHasAltsvcListeners;
326+
break;
327+
case 'remoteSettings':
328+
this[kNativeFields][kBitfield] |= 1 << kSessionHasRemoteSettingsListeners;
329+
break;
330+
case 'priority':
331+
this[kNativeFields][kSessionPriorityListenerCount]++;
332+
break;
333+
case 'frameError':
334+
this[kNativeFields][kSessionFrameErrorListenerCount]++;
335+
break;
336+
}
337+
}
338+
339+
function sessionListenerRemoved(name) {
340+
switch (name) {
341+
case 'ping':
342+
if (this.listenerCount(name) > 0) return;
343+
this[kNativeFields][kBitfield] &= ~(1 << kSessionHasPingListeners);
344+
break;
345+
case 'altsvc':
346+
if (this.listenerCount(name) > 0) return;
347+
this[kNativeFields][kBitfield] &= ~(1 << kSessionHasAltsvcListeners);
348+
break;
349+
case 'remoteSettings':
350+
if (this.listenerCount(name) > 0) return;
351+
this[kNativeFields][kBitfield] &=
352+
~(1 << kSessionHasRemoteSettingsListeners);
353+
break;
354+
case 'priority':
355+
this[kNativeFields][kSessionPriorityListenerCount]--;
356+
break;
357+
case 'frameError':
358+
this[kNativeFields][kSessionFrameErrorListenerCount]--;
359+
break;
360+
}
361+
}
362+
363+
// Also keep track of listeners for the Http2Stream instances, as some events
364+
// are emitted on those objects.
365+
function streamListenerAdded(name) {
366+
switch (name) {
367+
case 'priority':
368+
this[kSession][kNativeFields][kSessionPriorityListenerCount]++;
369+
break;
370+
case 'frameError':
371+
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++;
372+
break;
373+
}
374+
}
375+
376+
function streamListenerRemoved(name) {
377+
switch (name) {
378+
case 'priority':
379+
this[kSession][kNativeFields][kSessionPriorityListenerCount]--;
380+
break;
381+
case 'frameError':
382+
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--;
383+
break;
384+
}
385+
}
386+
308387
function onPing(payload) {
309388
const session = this[kOwner];
310389
if (session.destroyed)
@@ -394,7 +473,6 @@ function onSettings() {
394473
return;
395474
session[kUpdateTimer]();
396475
debugSessionObj(session, 'new settings received');
397-
session[kRemoteSettings] = undefined;
398476
session.emit('remoteSettings', session.remoteSettings);
399477
}
400478

@@ -845,6 +923,10 @@ function setupHandle(socket, type, options) {
845923
handle.consume(socket._handle._externalStream);
846924

847925
this[kHandle] = handle;
926+
if (this[kNativeFields])
927+
handle.fields.set(this[kNativeFields]);
928+
else
929+
this[kNativeFields] = handle.fields;
848930

849931
if (socket.encrypted) {
850932
this[kAlpnProtocol] = socket.alpnProtocol;
@@ -886,6 +968,7 @@ function finishSessionDestroy(session, error) {
886968
session[kProxySocket] = undefined;
887969
session[kSocket] = undefined;
888970
session[kHandle] = undefined;
971+
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
889972
socket[kSession] = undefined;
890973
socket[kServer] = undefined;
891974

@@ -963,6 +1046,7 @@ class Http2Session extends EventEmitter {
9631046
this[kType] = type;
9641047
this[kProxySocket] = null;
9651048
this[kSocket] = socket;
1049+
this[kHandle] = undefined;
9661050

9671051
// Do not use nagle's algorithm
9681052
if (typeof socket.setNoDelay === 'function')
@@ -981,6 +1065,11 @@ class Http2Session extends EventEmitter {
9811065
setupFn();
9821066
}
9831067

1068+
if (!this[kNativeFields])
1069+
this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
1070+
this.on('newListener', sessionListenerAdded);
1071+
this.on('removeListener', sessionListenerRemoved);
1072+
9841073
debugSession(type, 'created');
9851074
}
9861075

@@ -1136,13 +1225,18 @@ class Http2Session extends EventEmitter {
11361225

11371226
// The settings currently in effect for the remote peer.
11381227
get remoteSettings() {
1139-
const settings = this[kRemoteSettings];
1140-
if (settings !== undefined)
1141-
return settings;
1228+
if (this[kNativeFields][kBitfield] &
1229+
(1 << kSessionRemoteSettingsIsUpToDate)) {
1230+
const settings = this[kRemoteSettings];
1231+
if (settings !== undefined) {
1232+
return settings;
1233+
}
1234+
}
11421235

11431236
if (this.destroyed || this.connecting)
11441237
return {};
11451238

1239+
this[kNativeFields][kBitfield] |= (1 << kSessionRemoteSettingsIsUpToDate);
11461240
return this[kRemoteSettings] = getSettings(this[kHandle], true); // Remote
11471241
}
11481242

@@ -1330,6 +1424,12 @@ class ServerHttp2Session extends Http2Session {
13301424
constructor(options, socket, server) {
13311425
super(NGHTTP2_SESSION_SERVER, options, socket);
13321426
this[kServer] = server;
1427+
// This is a bit inaccurate because it does not reflect changes to
1428+
// number of listeners made after the session was created. This should
1429+
// not be an issue in practice. Additionally, the 'priority' event on
1430+
// server instances (or any other object) is fully undocumented.
1431+
this[kNativeFields][kSessionPriorityListenerCount] =
1432+
server.listenerCount('priority');
13331433
}
13341434

13351435
get server() {
@@ -1668,6 +1768,9 @@ class Http2Stream extends Duplex {
16681768
};
16691769

16701770
this.on('pause', streamOnPause);
1771+
1772+
this.on('newListener', streamListenerAdded);
1773+
this.on('removeListener', streamListenerRemoved);
16711774
}
16721775

16731776
[kUpdateTimer]() {
@@ -2620,7 +2723,7 @@ function sessionOnPriority(stream, parent, weight, exclusive) {
26202723
}
26212724

26222725
function sessionOnError(error) {
2623-
if (this[kServer])
2726+
if (this[kServer] !== undefined)
26242727
this[kServer].emit('sessionError', error, this);
26252728
}
26262729

@@ -2669,8 +2772,10 @@ function connectionListener(socket) {
26692772
const session = new ServerHttp2Session(options, socket, this);
26702773

26712774
session.on('stream', sessionOnStream);
2672-
session.on('priority', sessionOnPriority);
26732775
session.on('error', sessionOnError);
2776+
// Don't count our own internal listener.
2777+
session.on('priority', sessionOnPriority);
2778+
session[kNativeFields][kSessionPriorityListenerCount]--;
26742779

26752780
if (this.timeout)
26762781
session.setTimeout(this.timeout, sessionOnTimeout);

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,7 @@ class ModuleWrap;
161161
V(family_string, "family") \
162162
V(fatal_exception_string, "_fatalException") \
163163
V(fd_string, "fd") \
164+
V(fields_string, "fields") \
164165
V(file_string, "file") \
165166
V(fingerprint_string, "fingerprint") \
166167
V(flags_string, "flags") \

‎src/node_http2.cc‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ using v8::ObjectTemplate;
2020
using v8::String;
2121
using v8::Uint32;
2222
using v8::Uint32Array;
23+
using v8::Uint8Array;
2324
using v8::Undefined;
2425

2526
using node::performance::PerformanceEntry;
@@ -667,6 +668,15 @@ Http2Session::Http2Session(Environment* env,
667668

668669
outgoing_storage_.reserve(4096);
669670
outgoing_buffers_.reserve(32);
671+
672+
{
673+
// Make the js_fields_ property accessible to JS land.
674+
Local<ArrayBuffer> ab =
675+
ArrayBuffer::New(env->isolate(), js_fields_, kSessionUint8FieldCount);
676+
Local<Uint8Array> uint8_arr =
677+
Uint8Array::New(ab, 0, kSessionUint8FieldCount);
678+
/*USE*/(wrap->Set(env->context(), env->fields_string(), uint8_arr));
679+
}
670680
}
671681

672682
void Http2Session::Unconsume() {
@@ -1090,7 +1100,8 @@ inline int Http2Session::OnFrameNotSent(nghttp2_session* handle,
10901100
// Do not report if the frame was not sent due to the session closing
10911101
if (error_code == NGHTTP2_ERR_SESSION_CLOSING ||
10921102
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
1093-
error_code == NGHTTP2_ERR_STREAM_CLOSING) {
1103+
error_code == NGHTTP2_ERR_STREAM_CLOSING ||
1104+
session->js_fields_[kSessionFrameErrorListenerCount] == 0) {
10941105
return 0;
10951106
}
10961107

@@ -1346,7 +1357,8 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
13461357
// received. Notifies JS land about the priority change. Note that priorities
13471358
// are considered advisory only, so this has no real effect other than to
13481359
// simply let user code know that the priority has changed.
1349-
inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
1360+
void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
1361+
if (js_fields_[kSessionPriorityListenerCount] == 0) return;
13501362
Isolate* isolate = env()->isolate();
13511363
HandleScope scope(isolate);
13521364
Local<Context> context = env()->context();
@@ -1413,7 +1425,8 @@ inline void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
14131425
}
14141426

14151427
// Called by OnFrameReceived when a complete ALTSVC frame has been received.
1416-
inline void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {
1428+
void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) {
1429+
if (!(js_fields_[kBitfield] & (1 << kSessionHasAltsvcListeners))) return;
14171430
Isolate* isolate = env()->isolate();
14181431
HandleScope scope(isolate);
14191432
Local<Context> context = env()->context();
@@ -1499,6 +1512,7 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14991512
return;
15001513
}
15011514

1515+
if (!(js_fields_[kBitfield] & (1 << kSessionHasPingListeners))) return;
15021516
// Notify the session that a ping occurred
15031517
arg = Buffer::Copy(env(),
15041518
reinterpret_cast<const char*>(frame->ping.opaque_data),
@@ -1510,6 +1524,9 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
15101524
inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
15111525
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
15121526
if (!ack) {
1527+
js_fields_[kBitfield] &= ~(1 << kSessionRemoteSettingsIsUpToDate);
1528+
if (!(js_fields_[kBitfield] & (1 << kSessionHasRemoteSettingsListeners)))
1529+
return;
15131530
// This is not a SETTINGS acknowledgement, notify and return
15141531
MakeCallback(env()->onsettings_string(), 0, nullptr);
15151532
return;
@@ -3135,6 +3152,16 @@ void Initialize(Local<Object> target,
31353152
NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH);
31363153
NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE);
31373154

3155+
NODE_DEFINE_CONSTANT(target, kBitfield);
3156+
NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount);
3157+
NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount);
3158+
NODE_DEFINE_CONSTANT(target, kSessionUint8FieldCount);
3159+
3160+
NODE_DEFINE_CONSTANT(target, kSessionHasRemoteSettingsListeners);
3161+
NODE_DEFINE_CONSTANT(target, kSessionRemoteSettingsIsUpToDate);
3162+
NODE_DEFINE_CONSTANT(target, kSessionHasPingListeners);
3163+
NODE_DEFINE_CONSTANT(target, kSessionHasAltsvcListeners);
3164+
31383165
// Method to fetch the nghttp2 string description of an nghttp2 error code
31393166
env->SetMethod(target, "nghttp2ErrorString", HttpErrorString);
31403167

‎src/node_http2.h‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,23 @@ class Http2Stream::Provider::Stream : public Http2Stream::Provider {
735735
void* user_data);
736736
};
737737

738+
// Indices for js_fields_, which serves as a way to communicate data with JS
739+
// land fast. In particular, we store information about the number/presence
740+
// of certain event listeners in JS, and skip calls from C++ into JS if they
741+
// are missing.
742+
enum SessionUint8Fields {
743+
kBitfield, // See below
744+
kSessionPriorityListenerCount,
745+
kSessionFrameErrorListenerCount,
746+
kSessionUint8FieldCount
747+
};
748+
749+
enum SessionBitfieldFlags {
750+
kSessionHasRemoteSettingsListeners,
751+
kSessionRemoteSettingsIsUpToDate,
752+
kSessionHasPingListeners,
753+
kSessionHasAltsvcListeners
754+
};
738755

739756
class Http2Session : public AsyncWrap {
740757
public:
@@ -1016,6 +1033,9 @@ class Http2Session : public AsyncWrap {
10161033
// The underlying nghttp2_session handle
10171034
nghttp2_session* session_;
10181035

1036+
// JS-accessible numeric fields, as indexed by SessionUint8Fields.
1037+
uint8_t js_fields_[kSessionUint8FieldCount] = {};
1038+
10191039
// The session type: client or server
10201040
nghttp2_session_type session_type_;
10211041

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL