Skip to content
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.

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

lib: improve validation utils and refactor vm argument validation #31480

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 28 additions & 0 deletions lib/internal/validators.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const {
ArrayIsArray,
NumberIsInteger,
NumberMAX_SAFE_INTEGER,
NumberMIN_SAFE_INTEGER,
Expand Down Expand Up @@ -123,6 +124,30 @@ function validateNumber(value, name) {
throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
}

function validateBoolean(value, name) {
if (typeof value !== 'boolean')
throw new ERR_INVALID_ARG_TYPE(name, 'boolean', value);
}

const validateObject = hideStackFrames(
(value, name, { nullable = false } = {}) => {
if ((!nullable && value === null) ||
ArrayIsArray(value) ||
typeof value !== 'object') {
throw new ERR_INVALID_ARG_TYPE(name, 'Object', value);
}
});

const validateArray = hideStackFrames((value, name, { minLength = 0 } = {}) => {
if (!ArrayIsArray(value)) {
throw new ERR_INVALID_ARG_TYPE(name, 'Array', value);
}
if (value.length < minLength) {
const reason = `must be longer than ${minLength}`;
throw new ERR_INVALID_ARG_VALUE(name, value, reason);
}
});

function validateSignalName(signal, name = 'signal') {
if (typeof signal !== 'string')
throw new ERR_INVALID_ARG_TYPE(name, 'string', signal);
Expand Down Expand Up @@ -159,8 +184,11 @@ module.exports = {
isInt32,
isUint32,
parseFileMode,
validateArray,
validateBoolean,
validateBuffer,
validateEncoding,
validateObject,
validateInteger,
validateInt32,
validateUint32,
Expand Down
126 changes: 40 additions & 86 deletions lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
'use strict';

const {
ArrayIsArray,
ArrayPrototypeForEach,
Symbol,
} = primordials;
Expand All @@ -42,7 +41,11 @@ const {
const {
validateInt32,
validateUint32,
validateString
validateString,
validateArray,
validateBoolean,
validateBuffer,
validateObject,
} = require('internal/validators');
const { kVmBreakFirstLineSymbol } = require('internal/util');
const kParsingContext = Symbol('script parsing context');
Expand Down Expand Up @@ -139,30 +142,14 @@ class Script extends ContextifyScript {
}

function validateContext(contextifiedObject) {
if (typeof contextifiedObject !== 'object' || contextifiedObject === null) {
throw new ERR_INVALID_ARG_TYPE('contextifiedObject', 'Object',
contextifiedObject);
}
if (!_isContext(contextifiedObject)) {
if (!isContext(contextifiedObject)) {
throw new ERR_INVALID_ARG_TYPE('contextifiedObject', 'vm.Context',
contextifiedObject);
}
}

function validateBool(prop, propName) {
if (prop !== undefined && typeof prop !== 'boolean')
throw new ERR_INVALID_ARG_TYPE(propName, 'boolean', prop);
}

function validateObject(prop, propName) {
if (prop !== undefined && (typeof prop !== 'object' || prop === null))
throw new ERR_INVALID_ARG_TYPE(propName, 'Object', prop);
}

function getRunInContextArgs(options = {}) {
if (typeof options !== 'object' || options === null) {
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
}
validateObject(options, 'options');

let timeout = options.timeout;
if (timeout === undefined) {
Expand All @@ -177,14 +164,8 @@ function getRunInContextArgs(options = {}) {
[kVmBreakFirstLineSymbol]: breakFirstLine = false,
} = options;

if (typeof displayErrors !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE('options.displayErrors', 'boolean',
displayErrors);
}
if (typeof breakOnSigint !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE('options.breakOnSigint', 'boolean',
breakOnSigint);
}
validateBoolean(displayErrors, 'options.displayErrors');
validateBoolean(breakOnSigint, 'options.breakOnSigint');

return {
breakOnSigint,
Expand All @@ -193,30 +174,28 @@ function getRunInContextArgs(options = {}) {
}

function getContextOptions(options) {
if (options) {
if (!options)
return {};
const contextOptions = {
name: options.contextName,
origin: options.contextOrigin,
codeGeneration: undefined,
};
if (contextOptions.name !== undefined)
validateString(contextOptions.name, 'options.contextName');
if (contextOptions.origin !== undefined)
validateString(contextOptions.origin, 'options.contextOrigin');
if (options.contextCodeGeneration !== undefined) {
validateObject(options.contextCodeGeneration,
'options.contextCodeGeneration');
const contextOptions = {
name: options.contextName,
origin: options.contextOrigin,
codeGeneration: typeof options.contextCodeGeneration === 'object' ? {
strings: options.contextCodeGeneration.strings,
wasm: options.contextCodeGeneration.wasm,
} : undefined,
};
if (contextOptions.name !== undefined)
validateString(contextOptions.name, 'options.contextName');
if (contextOptions.origin !== undefined)
validateString(contextOptions.origin, 'options.contextOrigin');
if (contextOptions.codeGeneration) {
validateBool(contextOptions.codeGeneration.strings,
'options.contextCodeGeneration.strings');
validateBool(contextOptions.codeGeneration.wasm,
'options.contextCodeGeneration.wasm');
}
return contextOptions;
const { strings, wasm } = options.contextCodeGeneration;
if (strings !== undefined)
validateBoolean(strings, 'options.contextCodeGeneration.strings');
if (wasm !== undefined)
validateBoolean(wasm, 'options.contextCodeGeneration.wasm');
contextOptions.codeGeneration = { strings, wasm };
}
return {};
return contextOptions;
}

function isContext(object) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not changed as currently, it skips Arrays to the _isContext function which in turn rejects them. If I use validateObject it would change the error message for Arrays passed to the isContext from expected to be vm.Context to expected to be an Object and since we have explicit tests for this case I decided to not change this for now.

Expand All @@ -232,9 +211,7 @@ function createContext(contextObject = {}, options = {}) {
return contextObject;
}

if (typeof options !== 'object' || options === null) {
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
}
validateObject(options, 'options');

const {
name = `VM Context ${defaultContextNameIndex++}`,
Expand All @@ -245,14 +222,15 @@ function createContext(contextObject = {}, options = {}) {
validateString(name, 'options.name');
if (origin !== undefined)
validateString(origin, 'options.origin');
validateObject(codeGeneration, 'options.codeGeneration');
if (codeGeneration !== undefined)
validateObject(codeGeneration, 'options.codeGeneration');

let strings = true;
let wasm = true;
if (codeGeneration !== undefined) {
({ strings = true, wasm = true } = codeGeneration);
validateBool(strings, 'options.codeGeneration.strings');
validateBool(wasm, 'options.codeGeneration.wasm');
validateBoolean(strings, 'options.codeGeneration.strings');
validateBoolean(wasm, 'options.codeGeneration.wasm');
}

makeContext(contextObject, name, origin, strings, wasm);
Expand Down Expand Up @@ -314,9 +292,7 @@ function runInThisContext(code, options) {
function compileFunction(code, params, options = {}) {
validateString(code, 'code');
if (params !== undefined) {
if (!ArrayIsArray(params)) {
throw new ERR_INVALID_ARG_TYPE('params', 'Array', params);
}
validateArray(params, 'params');
ArrayPrototypeForEach(params,
(param, i) => validateString(param, `params[${i}]`));
}
Expand All @@ -334,20 +310,9 @@ function compileFunction(code, params, options = {}) {
validateString(filename, 'options.filename');
validateUint32(columnOffset, 'options.columnOffset');
validateUint32(lineOffset, 'options.lineOffset');
if (cachedData !== undefined && !isArrayBufferView(cachedData)) {
throw new ERR_INVALID_ARG_TYPE(
'options.cachedData',
['Buffer', 'TypedArray', 'DataView'],
cachedData
);
}
if (typeof produceCachedData !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE(
'options.produceCachedData',
'boolean',
produceCachedData
);
}
if (cachedData !== undefined)
validateBuffer(cachedData, 'options.cachedData');
validateBoolean(produceCachedData, 'options.produceCachedData');
if (parsingContext !== undefined) {
if (
typeof parsingContext !== 'object' ||
Expand All @@ -361,21 +326,10 @@ function compileFunction(code, params, options = {}) {
);
}
}
if (!ArrayIsArray(contextExtensions)) {
throw new ERR_INVALID_ARG_TYPE(
'options.contextExtensions',
'Array',
contextExtensions
);
}
validateArray(contextExtensions, 'options.contextExtensions');
ArrayPrototypeForEach(contextExtensions, (extension, i) => {
if (typeof extension !== 'object') {
throw new ERR_INVALID_ARG_TYPE(
`options.contextExtensions[${i}]`,
'object',
extension
);
}
const name = `options.contextExtensions[${i}]`;
validateObject(extension, name, { nullable: true });
});

const result = _compileFunction(
Expand Down
91 changes: 76 additions & 15 deletions test/parallel/test-validators.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,87 @@
// Flags: --expose-internals
'use strict';

require('../common');
const assert = require('assert');
const {
validateInteger
validateArray,
validateBoolean,
validateInteger,
validateObject,
} = require('internal/validators');
const { MAX_SAFE_INTEGER, MIN_SAFE_INTEGER } = Number;
const outOfRangeError = {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError'
name: 'RangeError',
};
const invalidArgTypeError = {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
};
const invalidArgValueError = {
code: 'ERR_INVALID_ARG_VALUE',
name: 'TypeError',
};

{
// validateInteger tests.

// validateInteger() defaults to validating safe integers.
validateInteger(MAX_SAFE_INTEGER, 'foo');
validateInteger(MIN_SAFE_INTEGER, 'foo');
assert.throws(() => {
validateInteger(MAX_SAFE_INTEGER + 1, 'foo');
}, outOfRangeError);
assert.throws(() => {
validateInteger(MIN_SAFE_INTEGER - 1, 'foo');
}, outOfRangeError);

// validateInteger() works with unsafe integers.
validateInteger(MAX_SAFE_INTEGER + 1, 'foo', 0, MAX_SAFE_INTEGER + 1);
validateInteger(MIN_SAFE_INTEGER - 1, 'foo', MIN_SAFE_INTEGER - 1);
}

{
// validateArray tests.
validateArray([], 'foo');
validateArray([1, 2, 3], 'foo');

[undefined, null, true, false, 0, 0.0, 42, '', 'string', {}]
.forEach((val) => {
assert.throws(() => {
validateArray(val, 'foo');
}, invalidArgTypeError);
});

validateArray([1], 'foo', { minLength: 1 });
assert.throws(() => {
validateArray([], 'foo', { minLength: 1 });
}, invalidArgValueError);
}

{
// validateBoolean tests.
validateBoolean(true, 'foo');
validateBoolean(false, 'foo');

[undefined, null, 0, 0.0, 42, '', 'string', {}, []].forEach((val) => {
assert.throws(() => {
validateBoolean(val, 'foo');
}, invalidArgTypeError);
});
}

{
// validateObject tests.
validateObject({}, 'foo');
validateObject({ a: 42, b: 'foo' }, 'foo');

[undefined, null, true, false, 0, 0.0, 42, '', 'string', []]
.forEach((val) => {
assert.throws(() => {
validateObject(val, 'foo');
}, invalidArgTypeError);
});

// validateInteger() defaults to validating safe integers.
validateInteger(MAX_SAFE_INTEGER, 'foo');
validateInteger(MIN_SAFE_INTEGER, 'foo');
assert.throws(() => {
validateInteger(MAX_SAFE_INTEGER + 1, 'foo');
}, outOfRangeError);
assert.throws(() => {
validateInteger(MIN_SAFE_INTEGER - 1, 'foo');
}, outOfRangeError);

// validateInteger() works with unsafe integers.
validateInteger(MAX_SAFE_INTEGER + 1, 'foo', 0, MAX_SAFE_INTEGER + 1);
validateInteger(MIN_SAFE_INTEGER - 1, 'foo', MIN_SAFE_INTEGER - 1);
validateObject(null, 'foo', { nullable: true });
}