From aea56474984799111a1d1820fd319ffb60a7b5df Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:00:47 +0100
Subject: [PATCH 1/9] lib: rename validateMode to parseMode

The function did not only validate the mode but it returns a new
value depending on the input. Thus `validate` did not seem to be an
appropriate name.
---
 lib/fs.js                                | 18 +++++++++---------
 lib/internal/fs/promises.js              | 10 +++++-----
 lib/internal/process/main_thread_only.js |  4 ++--
 lib/internal/validators.js               | 12 ++++++------
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index cfe856eaebd41c..aa32d0db2810e8 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -80,7 +80,7 @@ const {
 } = require('internal/constants');
 const {
   isUint32,
-  validateMode,
+  parseMode,
   validateInteger,
   validateInt32,
   validateUint32
@@ -426,7 +426,7 @@ function open(path, flags, mode, callback) {
   }
   const flagsNumber = stringToFlags(flags);
   if (arguments.length >= 4) {
-    mode = validateMode(mode, 'mode', 0o666);
+    mode = parseMode(mode, 'mode', 0o666);
   }
   callback = makeCallback(callback);
 
@@ -444,7 +444,7 @@ function openSync(path, flags, mode) {
   path = toPathIfFileURL(path);
   validatePath(path);
   const flagsNumber = stringToFlags(flags || 'r');
-  mode = validateMode(mode, 'mode', 0o666);
+  mode = parseMode(mode, 'mode', 0o666);
 
   const ctx = { path };
   const result = binding.open(pathModule.toNamespacedPath(path),
@@ -754,7 +754,7 @@ function mkdir(path, options, callback) {
   const req = new FSReqCallback();
   req.oncomplete = callback;
   binding.mkdir(pathModule.toNamespacedPath(path),
-                validateMode(mode, 'mode', 0o777), recursive, req);
+                parseMode(mode, 'mode', 0o777), recursive, req);
 }
 
 function mkdirSync(path, options) {
@@ -773,7 +773,7 @@ function mkdirSync(path, options) {
 
   const ctx = { path };
   binding.mkdir(pathModule.toNamespacedPath(path),
-                validateMode(mode, 'mode', 0o777), recursive, undefined,
+                parseMode(mode, 'mode', 0o777), recursive, undefined,
                 ctx);
   handleErrorFromBinding(ctx);
 }
@@ -1010,7 +1010,7 @@ function unlinkSync(path) {
 
 function fchmod(fd, mode, callback) {
   validateInt32(fd, 'fd', 0);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -1020,7 +1020,7 @@ function fchmod(fd, mode, callback) {
 
 function fchmodSync(fd, mode) {
   validateInt32(fd, 'fd', 0);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
   const ctx = {};
   binding.fchmod(fd, mode, undefined, ctx);
   handleErrorFromBinding(ctx);
@@ -1061,7 +1061,7 @@ function lchmodSync(path, mode) {
 function chmod(path, mode, callback) {
   path = toPathIfFileURL(path);
   validatePath(path);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -1072,7 +1072,7 @@ function chmod(path, mode, callback) {
 function chmodSync(path, mode) {
   path = toPathIfFileURL(path);
   validatePath(path);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
 
   const ctx = { path };
   binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 33c510ed5ca702..4a0c43a3937a6d 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -33,7 +33,7 @@ const {
   validatePath
 } = require('internal/fs/utils');
 const {
-  validateMode,
+  parseMode,
   validateInteger,
   validateUint32
 } = require('internal/validators');
@@ -198,7 +198,7 @@ async function open(path, flags, mode) {
   validatePath(path);
   if (arguments.length < 2) flags = 'r';
   const flagsNumber = stringToFlags(flags);
-  mode = validateMode(mode, 'mode', 0o666);
+  mode = parseMode(mode, 'mode', 0o666);
   return new FileHandle(
     await binding.openFileHandle(pathModule.toNamespacedPath(path),
                                  flagsNumber, mode, kUsePromises));
@@ -309,7 +309,7 @@ async function mkdir(path, options) {
     throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);
 
   return binding.mkdir(pathModule.toNamespacedPath(path),
-                       validateMode(mode, 'mode', 0o777), recursive,
+                       parseMode(mode, 'mode', 0o777), recursive,
                        kUsePromises);
 }
 
@@ -386,14 +386,14 @@ async function unlink(path) {
 
 async function fchmod(handle, mode) {
   validateFileHandle(handle);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
   return binding.fchmod(handle.fd, mode, kUsePromises);
 }
 
 async function chmod(path, mode) {
   path = toPathIfFileURL(path);
   validatePath(path);
-  mode = validateMode(mode, 'mode');
+  mode = parseMode(mode, 'mode');
   return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises);
 }
 
diff --git a/lib/internal/process/main_thread_only.js b/lib/internal/process/main_thread_only.js
index 24a7b02520fa57..358e2b37b0f834 100644
--- a/lib/internal/process/main_thread_only.js
+++ b/lib/internal/process/main_thread_only.js
@@ -11,7 +11,7 @@ const {
   }
 } = require('internal/errors');
 const {
-  validateMode,
+  parseMode,
   validateUint32,
   validateString
 } = require('internal/validators');
@@ -27,7 +27,7 @@ function wrapProcessMethods(binding) {
 
   function umask(mask) {
     if (mask !== undefined) {
-      mask = validateMode(mask, 'mask');
+      mask = parseMode(mask, 'mask');
     }
     return binding.umask(mask);
   }
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index a80917ee7edde4..1c351f68cb56ba 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -21,10 +21,10 @@ const octalReg = /^[0-7]+$/;
 const modeDesc = 'must be a 32-bit unsigned integer or an octal string';
 
 /**
- * Validate values that will be converted into mode_t (the S_* constants).
- * Only valid numbers and octal strings are allowed. They could be converted
- * to 32-bit unsigned integers or non-negative signed integers in the C++
- * land, but any value higher than 0o777 will result in platform-specific
+ * Parse and validate values that will be converted into mode_t (the S_*
+ * constants). Only valid numbers and octal strings are allowed. They could be
+ * converted to 32-bit unsigned integers or non-negative signed integers in the
+ * C++ land, but any value higher than 0o777 will result in platform-specific
  * behaviors.
  *
  * @param {*} value Values to be validated
@@ -32,7 +32,7 @@ const modeDesc = 'must be a 32-bit unsigned integer or an octal string';
  * @param {number} def If specified, will be returned for invalid values
  * @returns {number}
  */
-function validateMode(value, name, def) {
+function parseMode(value, name, def) {
   if (isUint32(value)) {
     return value;
   }
@@ -115,7 +115,7 @@ function validateNumber(value, name) {
 module.exports = {
   isInt32,
   isUint32,
-  validateMode,
+  parseMode,
   validateInteger,
   validateInt32,
   validateUint32,

From c1d5d17a7f608f5971794c8a2390457eb48273a4 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:05:33 +0100
Subject: [PATCH 2/9] lib: remove return values from validation functions

This makes sure the validation functions do not cause any side
effects. Validation functions should ideally only validate the input
without any other effect. Since the input value must be known from
the callee, there is no reason to return the input value.
---
 lib/internal/crypto/scrypt.js | 35 +++++++++++++++++++++++------------
 lib/internal/validators.js    |  5 -----
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/lib/internal/crypto/scrypt.js b/lib/internal/crypto/scrypt.js
index d364c1385995a8..e3cc130c747592 100644
--- a/lib/internal/crypto/scrypt.js
+++ b/lib/internal/crypto/scrypt.js
@@ -76,31 +76,42 @@ function check(password, salt, keylen, options) {
 
   password = validateArrayBufferView(password, 'password');
   salt = validateArrayBufferView(salt, 'salt');
-  keylen = validateUint32(keylen, 'keylen');
+  validateUint32(keylen, 'keylen');
 
   let { N, r, p, maxmem } = defaults;
   if (options && options !== defaults) {
     let has_N, has_r, has_p;
-    if (has_N = (options.N !== undefined))
-      N = validateUint32(options.N, 'N');
+    if (has_N = (options.N !== undefined)) {
+      validateUint32(options.N, 'N');
+      N = options.N;
+    }
     if (options.cost !== undefined) {
       if (has_N) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      N = validateUint32(options.cost, 'cost');
+      validateUint32(options.cost, 'cost');
+      N = options.cost;
+    }
+    if (has_r = (options.r !== undefined)) {
+      validateUint32(options.r, 'r');
+      r = options.r;
     }
-    if (has_r = (options.r !== undefined))
-      r = validateUint32(options.r, 'r');
     if (options.blockSize !== undefined) {
       if (has_r) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      r = validateUint32(options.blockSize, 'blockSize');
+      validateUint32(options.blockSize, 'blockSize');
+      r = options.blockSize;
+    }
+    if (has_p = (options.p !== undefined)) {
+      validateUint32(options.p, 'p');
+      p = options.p;
     }
-    if (has_p = (options.p !== undefined))
-      p = validateUint32(options.p, 'p');
     if (options.parallelization !== undefined) {
       if (has_p) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      p = validateUint32(options.parallelization, 'parallelization');
+      validateUint32(options.parallelization, 'parallelization');
+      p = options.parallelization;
+    }
+    if (options.maxmem !== undefined) {
+      validateUint32(options.maxmem, 'maxmem');
+      maxmem = options.maxmem;
     }
-    if (options.maxmem !== undefined)
-      maxmem = validateUint32(options.maxmem, 'maxmem');
     if (N === 0) N = defaults.N;
     if (r === 0) r = defaults.r;
     if (p === 0) p = defaults.p;
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index 1c351f68cb56ba..e4d937bf19c45e 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -60,7 +60,6 @@ const validateInteger = hideStackFrames((value, name) => {
     throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
   if (!Number.isSafeInteger(value))
     throw new ERR_OUT_OF_RANGE(name, 'an integer', value);
-  return value;
 });
 
 const validateInt32 = hideStackFrames(
@@ -78,7 +77,6 @@ const validateInt32 = hideStackFrames(
     if (value < min || value > max) {
       throw new ERR_OUT_OF_RANGE(name, `>= ${min} && <= ${max}`, value);
     }
-    return value;
   }
 );
 
@@ -97,9 +95,6 @@ const validateUint32 = hideStackFrames((value, name, positive) => {
   if (positive && value === 0) {
     throw new ERR_OUT_OF_RANGE(name, '>= 1 && < 4294967296', value);
   }
-  // TODO(BridgeAR): Remove return values from validation functions and
-  // especially reduce side effects caused by validation functions.
-  return value;
 });
 
 function validateString(value, name) {

From 5f76b83310fd359824c75f3d14a3de7e19e9e82a Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:12:48 +0100
Subject: [PATCH 3/9] timers: rename validateTimerDuration to getTimerDuration

The function did not only validate the timer but it caused side
effects like a warning and potentially returned a different value
than the input value. Thus the name `validate` did not seem to be
appropriate.
---
 lib/_http_client.js                 | 6 +++---
 lib/internal/stream_base_commons.js | 4 ++--
 lib/internal/timers.js              | 4 ++--
 lib/timers.js                       | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 6436d22f6e6540..63119d78becb27 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -45,7 +45,7 @@ const {
   ERR_INVALID_PROTOCOL,
   ERR_UNESCAPED_CHARACTERS
 } = require('internal/errors').codes;
-const { validateTimerDuration } = require('internal/timers');
+const { getTimerDuration } = require('internal/timers');
 const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
 const {
   DTRACE_HTTP_CLIENT_REQUEST,
@@ -146,7 +146,7 @@ function ClientRequest(input, options, cb) {
   this.socketPath = options.socketPath;
 
   if (options.timeout !== undefined)
-    this.timeout = validateTimerDuration(options.timeout, 'timeout');
+    this.timeout = getTimerDuration(options.timeout, 'timeout');
 
   var method = options.method;
   var methodIsString = (typeof method === 'string');
@@ -743,7 +743,7 @@ ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
   }
 
   listenSocketTimeout(this);
-  msecs = validateTimerDuration(msecs, 'msecs');
+  msecs = getTimerDuration(msecs, 'msecs');
   if (callback) this.once('timeout', callback);
 
   if (this.socket) {
diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js
index 67abba09925a8d..3f53e3903adddd 100644
--- a/lib/internal/stream_base_commons.js
+++ b/lib/internal/stream_base_commons.js
@@ -21,7 +21,7 @@ const { owner_symbol } = require('internal/async_hooks').symbols;
 const {
   kTimeout,
   setUnrefTimeout,
-  validateTimerDuration
+  getTimerDuration
 } = require('internal/timers');
 
 const kMaybeDestroy = Symbol('kMaybeDestroy');
@@ -205,7 +205,7 @@ function setStreamTimeout(msecs, callback) {
   this.timeout = msecs;
 
   // Type checking identical to timers.enroll()
-  msecs = validateTimerDuration(msecs, 'msecs');
+  msecs = getTimerDuration(msecs, 'msecs');
 
   // Attempt to clear an existing timer in both cases -
   //  even if it will be rescheduled we don't want to leak an existing timer.
diff --git a/lib/internal/timers.js b/lib/internal/timers.js
index c8d8e5a6c330d8..d485c3ff65a8a5 100644
--- a/lib/internal/timers.js
+++ b/lib/internal/timers.js
@@ -359,7 +359,7 @@ function setUnrefTimeout(callback, after) {
 }
 
 // Type checking used by timers.enroll() and Socket#setTimeout()
-function validateTimerDuration(msecs, name) {
+function getTimerDuration(msecs, name) {
   validateNumber(msecs, name);
   if (msecs < 0 || !isFinite(msecs)) {
     throw new ERR_OUT_OF_RANGE(name, 'a non-negative finite number', msecs);
@@ -586,7 +586,7 @@ module.exports = {
   kRefed,
   initAsyncResource,
   setUnrefTimeout,
-  validateTimerDuration,
+  getTimerDuration,
   immediateQueue,
   getTimerCallbacks,
   immediateInfoFields: {
diff --git a/lib/timers.js b/lib/timers.js
index dff7eb4994f793..8dc8b8877bed98 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -36,7 +36,7 @@ const {
   },
   kRefed,
   initAsyncResource,
-  validateTimerDuration,
+  getTimerDuration,
   timerListMap,
   timerListQueue,
   immediateQueue,
@@ -102,7 +102,7 @@ function unenroll(item) {
 // This function does not start the timer, see `active()`.
 // Using existing objects as timers slightly reduces object overhead.
 function enroll(item, msecs) {
-  msecs = validateTimerDuration(msecs, 'msecs');
+  msecs = getTimerDuration(msecs, 'msecs');
 
   // If this item was already in a list somewhere
   // then we should unenroll it from that

From 0ec9c67da6f6b28b93600a1bc18b5d71361ea6b1 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:15:06 +0100
Subject: [PATCH 4/9] child_process: rename _validateStdtio to getValidStdio

The name indicated only validation while it did much more and it
returned a different value to the callee function. The underscore
was also not necessary as the function is internal one way or the
other.
---
 lib/child_process.js                               |  4 ++--
 lib/internal/child_process.js                      |  6 +++---
 test/parallel/test-child-process-validate-stdio.js | 12 ++++++------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/child_process.js b/lib/child_process.js
index 8f16b707ea8ac6..683b4aec319140 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -41,7 +41,7 @@ const {
 const { validateString, isInt32 } = require('internal/validators');
 const child_process = require('internal/child_process');
 const {
-  _validateStdio,
+  getValidStdio,
   setupChannel,
   ChildProcess
 } = child_process;
@@ -577,7 +577,7 @@ function spawnSync(file, args, options) {
   // Validate and translate the kill signal, if present.
   options.killSignal = sanitizeKillSignal(options.killSignal);
 
-  options.stdio = _validateStdio(options.stdio || 'pipe', true).stdio;
+  options.stdio = getValidStdio(options.stdio || 'pipe', true).stdio;
 
   if (options.input) {
     var stdin = options.stdio[0] = { ...options.stdio[0] };
diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 0894fd1bcf0d9e..dddfd7cbeceb73 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -315,7 +315,7 @@ ChildProcess.prototype.spawn = function(options) {
   // If no `stdio` option was given - use default
   var stdio = options.stdio || 'pipe';
 
-  stdio = _validateStdio(stdio, false);
+  stdio = getValidStdio(stdio, false);
 
   ipc = stdio.ipc;
   ipcFd = stdio.ipcFd;
@@ -873,7 +873,7 @@ function isInternal(message) {
 
 function nop() { }
 
-function _validateStdio(stdio, sync) {
+function getValidStdio(stdio, sync) {
   var ipc;
   var ipcFd;
 
@@ -1028,6 +1028,6 @@ function spawnSync(opts) {
 module.exports = {
   ChildProcess,
   setupChannel,
-  _validateStdio,
+  getValidStdio,
   spawnSync
 };
diff --git a/test/parallel/test-child-process-validate-stdio.js b/test/parallel/test-child-process-validate-stdio.js
index 3566c8f9520f30..23b3fe2c95a877 100644
--- a/test/parallel/test-child-process-validate-stdio.js
+++ b/test/parallel/test-child-process-validate-stdio.js
@@ -3,21 +3,21 @@
 
 const common = require('../common');
 const assert = require('assert');
-const _validateStdio = require('internal/child_process')._validateStdio;
+const getValidStdio = require('internal/child_process').getValidStdio;
 
 const expectedError =
   common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError }, 2);
 
 // Should throw if string and not ignore, pipe, or inherit
-assert.throws(() => _validateStdio('foo'), expectedError);
+assert.throws(() => getValidStdio('foo'), expectedError);
 
 // Should throw if not a string or array
-assert.throws(() => _validateStdio(600), expectedError);
+assert.throws(() => getValidStdio(600), expectedError);
 
 // Should populate stdio with undefined if len < 3
 {
   const stdio1 = [];
-  const result = _validateStdio(stdio1, false);
+  const result = getValidStdio(stdio1, false);
   assert.strictEqual(stdio1.length, 3);
   assert.strictEqual(result.hasOwnProperty('stdio'), true);
   assert.strictEqual(result.hasOwnProperty('ipc'), true);
@@ -26,14 +26,14 @@ assert.throws(() => _validateStdio(600), expectedError);
 
 // Should throw if stdio has ipc and sync is true
 const stdio2 = ['ipc', 'ipc', 'ipc'];
-common.expectsError(() => _validateStdio(stdio2, true),
+common.expectsError(() => getValidStdio(stdio2, true),
                     { code: 'ERR_IPC_SYNC_FORK', type: Error }
 );
 
 
 if (common.isMainThread) {
   const stdio3 = [process.stdin, process.stdout, process.stderr];
-  const result = _validateStdio(stdio3, false);
+  const result = getValidStdio(stdio3, false);
   assert.deepStrictEqual(result, {
     stdio: [
       { type: 'fd', fd: 0 },

From f1a2bd53fa0cbcb3993c4d3b808cd977e8e9f7ff Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:18:16 +0100
Subject: [PATCH 5/9] 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.
---
 lib/internal/http2/core.js | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 824d7d604c9a09..69cc72c7ae5905 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -784,7 +784,7 @@ function pingCallback(cb) {
 // 6. enablePush must be a boolean
 // All settings are optional and may be left undefined
 const validateSettings = hideStackFrames((settings) => {
-  settings = { ...settings };
+  if (settings === undefined) return;
   assertWithinRange('headerTableSize',
                     settings.headerTableSize,
                     0, kMaxInt);
@@ -805,7 +805,6 @@ const validateSettings = hideStackFrames((settings) => {
     throw new ERR_HTTP2_INVALID_SETTING_VALUE('enablePush',
                                               settings.enablePush);
   }
-  return settings;
 });
 
 // Creates the internal binding.Http2Session handle for an Http2Session
@@ -1144,7 +1143,7 @@ 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();
@@ -1152,7 +1151,7 @@ class Http2Session extends EventEmitter {
 
     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;
@@ -2817,7 +2816,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();
 }
 

From 641e65d5e42f663e0db496b3efbfe87d05dec973 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 12:24:35 +0100
Subject: [PATCH 6/9] url: refactor validateHostname

This function did not only validate the input but it returned a new
value in case the hostname was valid. This simplifies the function
by always returning the required value, no matter if it is valid or
invalid, so the callee site does not have to check that anymore. On
top the function is renamed to `getHostname` to make clear that the
function does not only validate the input but it also returns a new
value.
---
 lib/url.js | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/url.js b/lib/url.js
index ade0007171ee48..f50d180c7a1585 100644
--- a/lib/url.js
+++ b/lib/url.js
@@ -359,9 +359,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
 
     // validate a little.
     if (!ipv6Hostname) {
-      const result = validateHostname(this, rest, hostname);
-      if (result !== undefined)
-        rest = result;
+      rest = getHostname(this, rest, hostname);
     }
 
     if (this.hostname.length > hostnameMaxLen) {
@@ -462,7 +460,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
   return this;
 };
 
-function validateHostname(self, rest, hostname) {
+function getHostname(self, rest, hostname) {
   for (var i = 0; i < hostname.length; ++i) {
     const code = hostname.charCodeAt(i);
     const isValid = (code >= CHAR_LOWERCASE_A && code <= CHAR_LOWERCASE_Z) ||
@@ -477,9 +475,10 @@ function validateHostname(self, rest, hostname) {
     // Invalid host character
     if (!isValid) {
       self.hostname = hostname.slice(0, i);
-      return '/' + hostname.slice(i) + rest;
+      return `/${hostname.slice(i)}${rest}`;
     }
   }
+  return rest;
 }
 
 // Escaped characters. Use empty strings to fill up unused entries.

From 373c45b875991c6b6582b9d29a966586451e2e30 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 13:04:15 +0100
Subject: [PATCH 7/9] http2: rename function for clarity

The function does not only validate the input but it causes side
effects by adding default options to the input object in case the
option is not set.
---
 lib/internal/http2/core.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 69cc72c7ae5905..3c4e6b55944fde 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -607,7 +607,7 @@ function requestOnConnect(headers, options) {
 // 4. if specified, options.silent must be a boolean
 //
 // Also sets the default priority options if they are not set.
-const validatePriorityOptions = hideStackFrames((options) => {
+const setAndValidatePriorityOptions = hideStackFrames((options) => {
   if (options.weight === undefined) {
     options.weight = NGHTTP2_DEFAULT_WEIGHT;
   } else if (typeof options.weight !== 'number') {
@@ -1451,7 +1451,7 @@ class ClientHttp2Session extends Http2Session {
         throw new ERR_HTTP2_CONNECT_PATH();
     }
 
-    validatePriorityOptions(options);
+    setAndValidatePriorityOptions(options);
 
     if (options.endStream === undefined) {
       // For some methods, we know that a payload is meaningless, so end the
@@ -1839,7 +1839,7 @@ class Http2Stream extends Duplex {
 
     assertIsObject(options, 'options');
     options = { ...options };
-    validatePriorityOptions(options);
+    setAndValidatePriorityOptions(options);
 
     const priorityFn = submitPriority.bind(this, options);
 

From 047c840c0212823882d20e8a0989a8d621fa8ddd Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 13:05:12 +0100
Subject: [PATCH 8/9] crypto: remove obsolete encoding check

This renames the parameters for clarity and removes the check for
undefined encoding. That will always default to `utf8`.
---
 lib/internal/crypto/util.js | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/internal/crypto/util.js b/lib/internal/crypto/util.js
index 6746f2a66c818a..c8a1f9e927ec6b 100644
--- a/lib/internal/crypto/util.js
+++ b/lib/internal/crypto/util.js
@@ -55,13 +55,13 @@ function getDefaultEncoding() {
 // This is here because many functions accepted binary strings without
 // any explicit encoding in older versions of node, and we don't want
 // to break them unnecessarily.
-function toBuf(str, encoding) {
-  if (typeof str === 'string') {
-    if (encoding === 'buffer' || !encoding)
+function toBuf(val, encoding) {
+  if (typeof val === 'string') {
+    if (encoding === 'buffer')
       encoding = 'utf8';
-    return Buffer.from(str, encoding);
+    return Buffer.from(val, encoding);
   }
-  return str;
+  return val;
 }
 
 const getCiphers = cachedResult(() => filterDuplicateStrings(_getCiphers()));

From 595fd9654fb0a4d057f7ebe75b3a98473d37194a Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Wed, 20 Mar 2019 13:15:48 +0100
Subject: [PATCH 9/9] lib: consolidate arrayBufferView validation

There are lots of places that validate for arrayBufferView and we
have multiple functions that do the same thing. Instead, move the
validation into `internal/validators` so all files can use that
instead.

There are more functions throughout the code that do the same but
it takes some more work to fully consolidate all of those.
---
 lib/fs.js                          |  2 +-
 lib/internal/crypto/certificate.js | 11 ++++-------
 lib/internal/fs/promises.js        |  2 +-
 lib/internal/fs/utils.js           | 10 ----------
 lib/internal/validators.js         | 15 +++++++++++++++
 5 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index aa32d0db2810e8..8aeaffb8ec17c3 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -69,7 +69,6 @@ const {
   stringToFlags,
   stringToSymlinkType,
   toUnixTimestamp,
-  validateBuffer,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
   validatePath
@@ -81,6 +80,7 @@ const {
 const {
   isUint32,
   parseMode,
+  validateBuffer,
   validateInteger,
   validateInt32,
   validateUint32
diff --git a/lib/internal/crypto/certificate.js b/lib/internal/crypto/certificate.js
index 29372ca497c935..d828bef0f6ae2d 100644
--- a/lib/internal/crypto/certificate.js
+++ b/lib/internal/crypto/certificate.js
@@ -5,6 +5,9 @@ const {
   certExportPublicKey,
   certVerifySpkac
 } = internalBinding('crypto');
+const {
+  validateBuffer
+} = require('internal/validators');
 
 const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
 const { isArrayBufferView } = require('internal/util/types');
@@ -14,13 +17,7 @@ const {
 } = require('internal/crypto/util');
 
 function verifySpkac(spkac) {
-  if (!isArrayBufferView(spkac)) {
-    throw new ERR_INVALID_ARG_TYPE(
-      'spkac',
-      ['Buffer', 'TypedArray', 'DataView'],
-      spkac
-    );
-  }
+  validateBuffer(spkac, 'spkac');
   return certVerifySpkac(spkac);
 }
 
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 4a0c43a3937a6d..93c8fc0f889d88 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -27,13 +27,13 @@ const {
   stringToFlags,
   stringToSymlinkType,
   toUnixTimestamp,
-  validateBuffer,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
   validatePath
 } = require('internal/fs/utils');
 const {
   parseMode,
+  validateBuffer,
   validateInteger,
   validateUint32
 } = require('internal/validators');
diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
index eacc4606619445..99e820d2944b69 100644
--- a/lib/internal/fs/utils.js
+++ b/lib/internal/fs/utils.js
@@ -14,7 +14,6 @@ const {
 } = require('internal/errors');
 const {
   isUint8Array,
-  isArrayBufferView,
   isDate
 } = require('internal/util/types');
 const { once } = require('internal/util');
@@ -393,14 +392,6 @@ function toUnixTimestamp(time, name = 'time') {
   throw new ERR_INVALID_ARG_TYPE(name, ['Date', 'Time in seconds'], time);
 }
 
-const validateBuffer = hideStackFrames((buffer) => {
-  if (!isArrayBufferView(buffer)) {
-    throw new ERR_INVALID_ARG_TYPE('buffer',
-                                   ['Buffer', 'TypedArray', 'DataView'],
-                                   buffer);
-  }
-});
-
 const validateOffsetLengthRead = hideStackFrames(
   (offset, length, bufferLength) => {
     if (offset < 0 || offset >= bufferLength) {
@@ -453,7 +444,6 @@ module.exports = {
   stringToSymlinkType,
   Stats,
   toUnixTimestamp,
-  validateBuffer,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
   validatePath
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index e4d937bf19c45e..3cc9c2820a1b09 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -8,6 +8,9 @@ const {
     ERR_OUT_OF_RANGE
   }
 } = require('internal/errors');
+const {
+  isArrayBufferView
+} = require('internal/util/types');
 
 function isInt32(value) {
   return value === (value | 0);
@@ -107,10 +110,22 @@ function validateNumber(value, name) {
     throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
 }
 
+// TODO(BridgeAR): We have multiple validation functions that call
+// `require('internal/utils').toBuf()` before validating for array buffer views.
+// Those should likely also be consolidated in here.
+const validateBuffer = hideStackFrames((buffer, name = 'buffer') => {
+  if (!isArrayBufferView(buffer)) {
+    throw new ERR_INVALID_ARG_TYPE(name,
+                                   ['Buffer', 'TypedArray', 'DataView'],
+                                   buffer);
+  }
+});
+
 module.exports = {
   isInt32,
   isUint32,
   parseMode,
+  validateBuffer,
   validateInteger,
   validateInt32,
   validateUint32,