Skip to content

Commit

Permalink
http2: fix setting options before handle exists
Browse files Browse the repository at this point in the history
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: #37875
Backport-PR-URL: #38673
Fixes: #37849
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
addaleax authored and targos committed Jun 11, 2021
1 parent 52fd42e commit c3bd0fd
Showing 1 changed file with 45 additions and 7 deletions.
52 changes: 45 additions & 7 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ const {
ObjectPrototypeHasOwnProperty,
Promise,
ReflectApply,
ReflectGet,
ReflectGetPrototypeOf,
ReflectSet,
Set,
Symbol,
Uint32Array,
Expand Down Expand Up @@ -933,6 +935,36 @@ const validateSettings = hideStackFrames((settings) => {
}
});

// Wrap a typed array in a proxy, and allow selectively copying the entries
// that have explicitly been set to another typed array.
function trackAssignmentsTypedArray(typedArray) {
const typedArrayLength = typedArray.length;
const modifiedEntries = new Uint8Array(typedArrayLength);

function copyAssigned(target) {
for (let i = 0; i < typedArrayLength; i++) {
if (modifiedEntries[i]) {
target[i] = typedArray[i];
}
}
}

return new Proxy(typedArray, {
get(obj, prop, receiver) {
if (prop === 'copyAssigned') {
return copyAssigned;
}
return ReflectGet(obj, prop, receiver);
},
set(obj, prop, value) {
if (`${+prop}` === prop) {
modifiedEntries[prop] = 1;
}
return ReflectSet(obj, prop, value);
}
});
}

// Creates the internal binding.Http2Session handle for an Http2Session
// instance. This occurs only after the socket connection has been
// established. Note: the binding.Http2Session will take over ownership
Expand Down Expand Up @@ -963,10 +995,13 @@ 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 (this[kNativeFields]) {
// If some options have already been set before the handle existed, copy
// those (and only those) that have manually been set over.
this[kNativeFields].copyAssigned(handle.fields);
}

this[kNativeFields] = handle.fields;

if (socket.encrypted) {
this[kAlpnProtocol] = socket.alpnProtocol;
Expand Down Expand Up @@ -1018,7 +1053,8 @@ function cleanupSession(session) {
session[kProxySocket] = undefined;
session[kSocket] = undefined;
session[kHandle] = undefined;
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
session[kNativeFields] = trackAssignmentsTypedArray(
new Uint8Array(kSessionUint8FieldCount));
if (handle)
handle.ondone = null;
if (socket) {
Expand Down Expand Up @@ -1184,8 +1220,10 @@ class Http2Session extends EventEmitter {
setupFn();
}

if (!this[kNativeFields])
this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
if (!this[kNativeFields]) {
this[kNativeFields] = trackAssignmentsTypedArray(
new Uint8Array(kSessionUint8FieldCount));
}
this.on('newListener', sessionListenerAdded);
this.on('removeListener', sessionListenerRemoved);

Expand Down

0 comments on commit c3bd0fd

Please sign in to comment.