lib: introduce internal/validators

Create a file to centralize argument validators that are used in
multiple internal modules.
Move validateInt32 and validateUint32 to this file.

PR-URL: https://github.com/nodejs/node/pull/19973
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

8 files changed, 112 insertions, 99 deletions

diff --git a/lib/fs.js b/lib/fs.js
index d71e31565f..c49d470f03 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -51,7 +51,6 @@ const internalUtil = require('internal/util');
 const {
   copyObject,
   getOptions,
-  isUint32,
   modeNum,
   nullCheck,
   preprocessSymlinkDestination,
@@ -61,16 +60,19 @@ const {
   stringToSymlinkType,
   toUnixTimestamp,
   validateBuffer,
-  validateLen,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
-  validatePath,
-  validateUint32
+  validatePath
 } = internalFS;
 const {
   CHAR_FORWARD_SLASH,
   CHAR_BACKWARD_SLASH,
 } = require('internal/constants');
+const {
+  isUint32,
+  validateInt32,
+  validateUint32
+} = require('internal/validators');
 
 Object.defineProperty(exports, 'constants', {
   configurable: false,
@@ -755,8 +757,8 @@ fs.ftruncate = function(fd, len = 0, callback) {
   // TODO(BridgeAR): This does not seem right.
   // There does not seem to be any validation before and if there is any, it
   // should work similar to validateUint32 or not have a upper cap at all.
-  // This applies to all usage of `validateLen`.
-  validateLen(len);
+  // This applies to all usage of `validateInt32(len, 'len')`.
+  validateInt32(len, 'len');
   len = Math.max(0, len);
   const req = new FSReqWrap();
   req.oncomplete = makeCallback(callback);
@@ -765,7 +767,7 @@ fs.ftruncate = function(fd, len = 0, callback) {
 
 fs.ftruncateSync = function(fd, len = 0) {
   validateUint32(fd, 'fd');
-  validateLen(len);
+  validateInt32(len, 'len');
   len = Math.max(0, len);
   const ctx = {};
   binding.ftruncate(fd, len, undefined, ctx);
diff --git a/lib/fs/promises.js b/lib/fs/promises.js
index ba6c2b7aa6..6ccff6933b 100644
--- a/lib/fs/promises.js
+++ b/lib/fs/promises.js
@@ -24,7 +24,6 @@ const {
   copyObject,
   getOptions,
   getStatsFromBinding,
-  isUint32,
   modeNum,
   nullCheck,
   preprocessSymlinkDestination,
@@ -32,12 +31,15 @@ const {
   stringToSymlinkType,
   toUnixTimestamp,
   validateBuffer,
-  validateLen,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
-  validatePath,
-  validateUint32
+  validatePath
 } = require('internal/fs');
+const {
+  isUint32,
+  validateInt32,
+  validateUint32
+} = require('internal/validators');
 const pathModule = require('path');
 
 const kHandle = Symbol('handle');
@@ -275,7 +277,7 @@ async function truncate(path, len = 0) {
 
 async function ftruncate(handle, len = 0) {
   validateFileHandle(handle);
-  validateLen(len);
+  validateInt32(len, 'len');
   len = Math.max(0, len);
   return binding.ftruncate(handle.fd, len, kUsePromises);
 }
diff --git a/lib/internal/fs.js b/lib/internal/fs.js
index 0cfb89a9d3..2f7a8d8ced 100644
--- a/lib/internal/fs.js
+++ b/lib/internal/fs.js
@@ -70,9 +70,6 @@ function getOptions(options, defaultOptions) {
   return options;
 }
 
-function isInt32(n) { return n === (n | 0); }
-function isUint32(n) { return n === (n >>> 0); }
-
 function modeNum(m, def) {
   if (typeof m === 'number')
     return m;
@@ -341,26 +338,6 @@ function validateBuffer(buffer) {
   }
 }
 
-function validateLen(len) {
-  let err;
-
-  if (!isInt32(len)) {
-    if (typeof len !== 'number') {
-      err = new ERR_INVALID_ARG_TYPE('len', 'number', len);
-    } else if (!Number.isInteger(len)) {
-      err = new ERR_OUT_OF_RANGE('len', 'an integer', len);
-    } else {
-      // 2 ** 31 === 2147483648
-      err = new ERR_OUT_OF_RANGE('len', '> -2147483649 && < 2147483648', len);
-    }
-  }
-
-  if (err !== undefined) {
-    Error.captureStackTrace(err, validateLen);
-    throw err;
-  }
-}
-
 function validateOffsetLengthRead(offset, length, bufferLength) {
   let err;
 
@@ -410,28 +387,10 @@ function validatePath(path, propName = 'path') {
   }
 }
 
-function validateUint32(value, propName) {
-  if (!isUint32(value)) {
-    let err;
-    if (typeof value !== 'number') {
-      err = new ERR_INVALID_ARG_TYPE(propName, 'number', value);
-    } else if (!Number.isInteger(value)) {
-      err = new ERR_OUT_OF_RANGE(propName, 'an integer', value);
-    } else {
-      // 2 ** 32 === 4294967296
-      err = new ERR_OUT_OF_RANGE(propName, '>= 0 && < 4294967296', value);
-    }
-    Error.captureStackTrace(err, validateUint32);
-    throw err;
-  }
-}
-
 module.exports = {
   assertEncoding,
   copyObject,
   getOptions,
-  isInt32,
-  isUint32,
   modeNum,
   nullCheck,
   preprocessSymlinkDestination,
@@ -443,9 +402,7 @@ module.exports = {
   SyncWriteStream,
   toUnixTimestamp,
   validateBuffer,
-  validateLen,
   validateOffsetLengthRead,
   validateOffsetLengthWrite,
-  validatePath,
-  validateUint32
+  validatePath
 };
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
new file mode 100644
index 0000000000..556bfb2dc0
--- /dev/null
+++ b/lib/internal/validators.js
@@ -0,0 +1,58 @@
+'use strict';
+
+const {
+  ERR_INVALID_ARG_TYPE,
+  ERR_OUT_OF_RANGE
+} = require('internal/errors').codes;
+
+function isInt32(value) {
+  return value === (value | 0);
+}
+
+function isUint32(value) {
+  return value === (value >>> 0);
+}
+
+function validateInt32(value, name) {
+  if (!isInt32(value)) {
+    let err;
+    if (typeof value !== 'number') {
+      err = new ERR_INVALID_ARG_TYPE(name, 'number', value);
+    } else if (!Number.isInteger(value)) {
+      err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
+    } else {
+      // 2 ** 31 === 2147483648
+      err = new ERR_OUT_OF_RANGE(name, '> -2147483649 && < 2147483648', value);
+    }
+    Error.captureStackTrace(err, validateInt32);
+    throw err;
+  }
+}
+
+function validateUint32(value, name, positive) {
+  if (!isUint32(value)) {
+    let err;
+    if (typeof value !== 'number') {
+      err = new ERR_INVALID_ARG_TYPE(name, 'number', value);
+    } else if (!Number.isInteger(value)) {
+      err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
+    } else {
+      const min = positive ? 1 : 0;
+      // 2 ** 32 === 4294967296
+      err = new ERR_OUT_OF_RANGE(name, `>= ${min} && < 4294967296`, value);
+    }
+    Error.captureStackTrace(err, validateUint32);
+    throw err;
+  } else if (positive && value === 0) {
+    const err = new ERR_OUT_OF_RANGE(name, '>= 1 && < 4294967296', value);
+    Error.captureStackTrace(err, validateUint32);
+    throw err;
+  }
+}
+
+module.exports = {
+  isInt32,
+  isUint32,
+  validateInt32,
+  validateUint32
+};
diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
index 7284c8bd61..9e945c45c3 100644
--- a/lib/internal/vm/module.js
+++ b/lib/internal/vm/module.js
@@ -6,7 +6,6 @@ const { URL } = require('internal/url');
 const { isContext } = process.binding('contextify');
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_OUT_OF_RANGE,
   ERR_VM_MODULE_ALREADY_LINKED,
   ERR_VM_MODULE_DIFFERENT_CONTEXT,
   ERR_VM_MODULE_LINKING_ERRORED,
@@ -19,6 +18,7 @@ const {
   customInspectSymbol,
 } = require('internal/util');
 const { SafePromise } = require('internal/safe_globals');
+const { validateInt32, validateUint32 } = require('internal/validators');
 
 const {
   ModuleWrap,
@@ -92,8 +92,8 @@ class Module {
       perContextModuleId.set(context, 1);
     }
 
-    validateInteger(lineOffset, 'options.lineOffset');
-    validateInteger(columnOffset, 'options.columnOffset');
+    validateInt32(lineOffset, 'options.lineOffset');
+    validateInt32(columnOffset, 'options.columnOffset');
 
     if (initializeImportMeta !== undefined) {
       if (typeof initializeImportMeta === 'function') {
@@ -203,9 +203,8 @@ class Module {
     let timeout = options.timeout;
     if (timeout === undefined) {
       timeout = -1;
-    } else if (!Number.isInteger(timeout) || timeout <= 0) {
-      throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer',
-                                     timeout);
+    } else {
+      validateUint32(timeout, 'options.timeout', true);
     }
 
     const { breakOnSigint = false } = options;
@@ -243,15 +242,6 @@ class Module {
   }
 }
 
-function validateInteger(prop, propName) {
-  if (!Number.isInteger(prop)) {
-    throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop);
-  }
-  if ((prop >> 0) !== prop) {
-    throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop);
-  }
-}
-
 module.exports = {
   Module,
   initImportMetaMap,
diff --git a/lib/vm.js b/lib/vm.js
index 5a5130d7c9..3ab50c8158 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -28,11 +28,9 @@ const {
   isContext: _isContext,
 } = process.binding('contextify');
 
-const {
-  ERR_INVALID_ARG_TYPE,
-  ERR_OUT_OF_RANGE
-} = require('internal/errors').codes;
+const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
 const { isUint8Array } = require('internal/util/types');
+const { validateInt32, validateUint32 } = require('internal/validators');
 
 class Script extends ContextifyScript {
   constructor(code, options = {}) {
@@ -56,8 +54,8 @@ class Script extends ContextifyScript {
     if (typeof filename !== 'string') {
       throw new ERR_INVALID_ARG_TYPE('options.filename', 'string', filename);
     }
-    validateInteger(lineOffset, 'options.lineOffset');
-    validateInteger(columnOffset, 'options.columnOffset');
+    validateInt32(lineOffset, 'options.lineOffset');
+    validateInt32(columnOffset, 'options.columnOffset');
     if (cachedData !== undefined && !isUint8Array(cachedData)) {
       throw new ERR_INVALID_ARG_TYPE('options.cachedData',
                                      ['Buffer', 'Uint8Array'], cachedData);
@@ -119,15 +117,6 @@ function validateContext(sandbox) {
   }
 }
 
-function validateInteger(prop, propName) {
-  if (!Number.isInteger(prop)) {
-    throw new ERR_INVALID_ARG_TYPE(propName, 'integer', prop);
-  }
-  if ((prop >> 0) !== prop) {
-    throw new ERR_OUT_OF_RANGE(propName, '32-bit integer', prop);
-  }
-}
-
 function validateString(prop, propName) {
   if (prop !== undefined && typeof prop !== 'string')
     throw new ERR_INVALID_ARG_TYPE(propName, 'string', prop);
@@ -151,9 +140,8 @@ function getRunInContextArgs(options = {}) {
   let timeout = options.timeout;
   if (timeout === undefined) {
     timeout = -1;
-  } else if (!Number.isInteger(timeout) || timeout <= 0) {
-    throw new ERR_INVALID_ARG_TYPE('options.timeout', 'a positive integer',
-                                   timeout);
+  } else {
+    validateUint32(timeout, 'options.timeout', true);
   }
 
   const {
diff --git a/node.gyp b/node.gyp
index abed8ad4c4..2c7eb994d2 100644
--- a/node.gyp
+++ b/node.gyp
@@ -146,6 +146,7 @@
       'lib/internal/v8.js',
       'lib/internal/v8_prof_polyfill.js',
       'lib/internal/v8_prof_processor.js',
+      'lib/internal/validators.js',
       'lib/internal/stream_base_commons.js',
       'lib/internal/vm/module.js',
       'lib/internal/streams/lazy_transform.js',
diff --git a/test/parallel/test-vm-options-validation.js b/test/parallel/test-vm-options-validation.js
index e00a14e4b9..67a27d58ff 100644
--- a/test/parallel/test-vm-options-validation.js
+++ b/test/parallel/test-vm-options-validation.js
@@ -17,7 +17,7 @@ common.expectsError(() => {
   new vm.Script('void 0', 42);
 }, invalidArgType);
 
-[null, {}, [1], 'bad', true, 0.1].forEach((value) => {
+[null, {}, [1], 'bad', true].forEach((value) => {
   common.expectsError(() => {
     new vm.Script('void 0', { lineOffset: value });
   }, invalidArgType);
@@ -27,6 +27,16 @@ common.expectsError(() => {
   }, invalidArgType);
 });
 
+[0.1, 2 ** 32].forEach((value) => {
+  common.expectsError(() => {
+    new vm.Script('void 0', { lineOffset: value });
+  }, outOfRange);
+
+  common.expectsError(() => {
+    new vm.Script('void 0', { columnOffset: value });
+  }, outOfRange);
+});
+
 common.expectsError(() => {
   new vm.Script('void 0', { lineOffset: Number.MAX_SAFE_INTEGER });
 }, outOfRange);
@@ -53,26 +63,31 @@ common.expectsError(() => {
   const script = new vm.Script('void 0');
   const sandbox = vm.createContext();
 
-  function assertErrors(options) {
+  function assertErrors(options, errCheck) {
     common.expectsError(() => {
       script.runInThisContext(options);
-    }, invalidArgType);
+    }, errCheck);
 
     common.expectsError(() => {
       script.runInContext(sandbox, options);
-    }, invalidArgType);
+    }, errCheck);
 
     common.expectsError(() => {
       script.runInNewContext({}, options);
-    }, invalidArgType);
+    }, errCheck);
   }
 
-  [null, 'bad', 42].forEach(assertErrors);
-  [{}, [1], 'bad', null, -1, 0, NaN].forEach((value) => {
-    assertErrors({ timeout: value });
+  [null, 'bad', 42].forEach((value) => {
+    assertErrors(value, invalidArgType);
+  });
+  [{}, [1], 'bad', null].forEach((value) => {
+    assertErrors({ timeout: value }, invalidArgType);
+  });
+  [-1, 0, NaN].forEach((value) => {
+    assertErrors({ timeout: value }, outOfRange);
   });
   [{}, [1], 'bad', 1, null].forEach((value) => {
-    assertErrors({ displayErrors: value });
-    assertErrors({ breakOnSigint: value });
+    assertErrors({ displayErrors: value }, invalidArgType);
+    assertErrors({ breakOnSigint: value }, invalidArgType);
   });
 }