Skip to content

http2: stricter settings validation #26811

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
BridgeAR wants to merge 2 commits into from
Closed

http2: stricter settings validation #26811

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b70d2d
http2: remove side effects from validateSettings
BridgeAR Mar 20, 2019
5c1b27f
http2: validate settings input
BridgeAR Mar 20, 2019
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: remove side effects from validateSettings 
The function did not only validate the input so far but it also made
a copy of the input object and returned that copy to the callee
function. That copy was not necessary for all call sites and it was
not obvious that the function did not only validate the input but
that it also returned a copy of it. This makes sure the function does
nothing more than validation and copying is happening in the callee
function when required.
  • Loading branch information
@BridgeAR
BridgeAR committed Mar 20, 2019
commit 4b70d2d6f4619ea3ca12aab65e77aa6d9226dabe
10 changes: 5 additions & 5 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ function pingCallback(cb) {
// 6. enablePush must be a boolean
// All settings are optional and may be left undefined
function validateSettings(settings) {
settings = { ...settings };
if (settings === undefined) return;
assertWithinRange('headerTableSize',
settings.headerTableSize,
0, kMaxInt);
Expand All @@ -813,7 +813,6 @@ function validateSettings(settings) {
Error.captureStackTrace(err, 'validateSettings');
throw err;
}
return settings;
}

// Creates the internal binding.Http2Session handle for an Http2Session
Expand Down Expand Up @@ -1152,15 +1151,15 @@ class Http2Session extends EventEmitter {
if (this.destroyed)
throw new ERR_HTTP2_INVALID_SESSION();
assertIsObject(settings, 'settings');
settings = validateSettings(settings);
validateSettings(settings);

if (callback && typeof callback !== 'function')
throw new ERR_INVALID_CALLBACK();
debug(`Http2Session ${sessionName(this[kType])}: sending settings`);

this[kState].pendingAck++;

const settingsFn = submitSettings.bind(this, settings, callback);
const settingsFn = submitSettings.bind(this, { ...settings }, callback);
if (this.connecting) {
this.once('connect', settingsFn);
return;
Expand Down Expand Up @@ -2830,7 +2829,8 @@ function createServer(options, handler) {
// HTTP2-Settings header frame.
function getPackedSettings(settings) {
assertIsObject(settings, 'settings');
updateSettingsBuffer(validateSettings(settings));
validateSettings(settings);
updateSettingsBuffer({ ...settings });
return binding.packSettings();
}

Expand Down