Skip to content

Commit d22a56f

Browse files
addaleaxtargos
authored andcommitted
http2: fix setting options before handle exists
Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: nodejs#37875 Fixes: nodejs#37849 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent 43f4668 commit d22a56f

1 file changed

Lines changed: 45 additions & 7 deletions

File tree

lib/internal/http2/core.js

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ const {
1313
ObjectPrototypeHasOwnProperty,
1414
Promise,
1515
ReflectApply,
16+
ReflectGet,
1617
ReflectGetPrototypeOf,
18+
ReflectSet,
1719
Set,
1820
Symbol,
1921
Uint32Array,
@@ -932,6 +934,36 @@ const validateSettings = hideStackFrames((settings) => {
932934
}
933935
});
934936

937+
// Wrap a typed array in a proxy, and allow selectively copying the entries
938+
// that have explicitly been set to another typed array.
939+
function trackAssignmentsTypedArray(typedArray) {
940+
const typedArrayLength = typedArray.length;
941+
const modifiedEntries = new Uint8Array(typedArrayLength);
942+
943+
function copyAssigned(target) {
944+
for (let i = 0; i < typedArrayLength; i++) {
945+
if (modifiedEntries[i]) {
946+
target[i] = typedArray[i];
947+
}
948+
}
949+
}
950+
951+
return new Proxy(typedArray, {
952+
get(obj, prop, receiver) {
953+
if (prop === 'copyAssigned') {
954+
return copyAssigned;
955+
}
956+
return ReflectGet(obj, prop, receiver);
957+
},
958+
set(obj, prop, value) {
959+
if (`${+prop}` === prop) {
960+
modifiedEntries[prop] = 1;
961+
}
962+
return ReflectSet(obj, prop, value);
963+
}
964+
});
965+
}
966+
935967
// Creates the internal binding.Http2Session handle for an Http2Session
936968
// instance. This occurs only after the socket connection has been
937969
// established. Note: the binding.Http2Session will take over ownership
@@ -962,10 +994,13 @@ function setupHandle(socket, type, options) {
962994
handle.consume(socket._handle);
963995

964996
this[kHandle] = handle;
965-
if (this[kNativeFields])
966-
handle.fields.set(this[kNativeFields]);
967-
else
968-
this[kNativeFields] = handle.fields;
997+
if (this[kNativeFields]) {
998+
// If some options have already been set before the handle existed, copy
999+
// those (and only those) that have manually been set over.
1000+
this[kNativeFields].copyAssigned(handle.fields);
1001+
}
1002+
1003+
this[kNativeFields] = handle.fields;
9691004

9701005
if (socket.encrypted) {
9711006
this[kAlpnProtocol] = socket.alpnProtocol;
@@ -1017,7 +1052,8 @@ function cleanupSession(session) {
10171052
session[kProxySocket] = undefined;
10181053
session[kSocket] = undefined;
10191054
session[kHandle] = undefined;
1020-
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
1055+
session[kNativeFields] = trackAssignmentsTypedArray(
1056+
new Uint8Array(kSessionUint8FieldCount));
10211057
if (handle)
10221058
handle.ondone = null;
10231059
if (socket) {
@@ -1183,8 +1219,10 @@ class Http2Session extends EventEmitter {
11831219
setupFn();
11841220
}
11851221

1186-
if (!this[kNativeFields])
1187-
this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
1222+
if (!this[kNativeFields]) {
1223+
this[kNativeFields] = trackAssignmentsTypedArray(
1224+
new Uint8Array(kSessionUint8FieldCount));
1225+
}
11881226
this.on('newListener', sessionListenerAdded);
11891227
this.on('removeListener', sessionListenerRemoved);
11901228

0 commit comments

Comments
 (0)