From e8361287030fbaa773761bb3798d45903bb160f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sun, 15 Apr 2018 11:16:50 +0200 Subject: [PATCH] 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 Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/fs.js | 16 +++--- lib/fs/promises.js | 12 +++-- lib/internal/fs.js | 45 +--------------- lib/internal/validators.js | 58 +++++++++++++++++++++ lib/internal/vm/module.js | 20 ++----- lib/vm.js | 24 +++------ node.gyp | 1 + test/parallel/test-vm-options-validation.js | 35 +++++++++---- 8 files changed, 112 insertions(+), 99 deletions(-) create mode 100644 lib/internal/validators.js diff --git a/lib/fs.js b/lib/fs.js index d71e31565fbad6..c49d470f0355dc 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 ba6c2b7aa64855..6ccff6933bf106 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 0cfb89a9d3d084..2f7a8d8ced176e 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 00000000000000..556bfb2dc08f5f --- /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 7284c8bd619901..9e945c45c3de8a 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 5a5130d7c9c328..3ab50c81580365 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 abed8ad4c48c10..2c7eb994d247cc 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 e00a14e4b9e603..67a27d58fff805 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); }); }