Skip to content

http2: treat non-EOF empty frames like other invalid frames #37875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
addaleax wants to merge 4 commits into from
Closed

http2: treat non-EOF empty frames like other invalid frames #37875

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd15502
http2: fix setting options before handle exists
addaleax Mar 23, 2021
515cab2
http2: treat non-EOF empty frames like other invalid frames
addaleax Mar 23, 2021
752cb85
fixup! http2: fix setting options before handle exists
addaleax Mar 23, 2021
a087881
fixup! http2: fix setting options before handle exists
addaleax Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
  • Loading branch information
@addaleax
addaleax committed Mar 23, 2021
commit fd15502a10033d1a13d66dadd3485e23ec7e5450
52 changes: 44 additions & 8 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@ const {
Promise,
PromisePrototypeCatch,
ReflectApply,
ReflectGet,
ReflectGetPrototypeOf,
ReflectSet,
RegExpPrototypeTest,
SafeArrayIterator,
SafeMap,
SafeSet,
StringPrototypeSlice,
Symbol,
TypedArrayPrototypeSet,
Uint32Array,
Uint8Array,
} = primordials;
Expand Down Expand Up @@ -960,6 +961,35 @@ 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 modifiedEntries = new Uint8Array(typedArray.length);
Comment thread
addaleax marked this conversation as resolved.
Resolved

function copyAssigned(target) {
for (let i = 0; i < typedArray.length; i++) {
Comment thread
addaleax marked this conversation as resolved.
Resolved
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 @@ -990,10 +1020,13 @@ function setupHandle(socket, type, options) {
handle.consume(socket._handle);

this[kHandle] = handle;
if (this[kNativeFields])
TypedArrayPrototypeSet(handle.fields, 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 @@ -1045,7 +1078,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 @@ -1213,8 +1247,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