Skip to content

[v14.x] http2: treat non-EOF empty frames like other invalid frames #38673

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 2 commits into from
Closed

[v14.x] http2: treat non-EOF empty frames like other invalid frames #38673

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d32bdd1
http2: fix setting options before handle exists
addaleax Mar 23, 2021
990ef98
http2: treat non-EOF empty frames like other invalid frames
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.

PR-URL: #37875
Fixes: #37849
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
@addaleax @targos
addaleax authored and targos committed Jun 5, 2021
commit d32bdd14c2a764b6920afee84951cbd3886faf55
52 changes: 45 additions & 7 deletions lib/internal/http2/core.js
View file
Open in desktop
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