vm: move options checks from C++ to JS

Also introduces stronger type validations for options passed to vm
functions.

PR-URL: https://github.com/nodejs/node/pull/19398
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

6 files changed, 175 insertions, 79 deletions

diff --git a/test/parallel/test-vm-basic.js b/test/parallel/test-vm-basic.js
index bf1532cacc..2dfc1e9cb7 100644
--- a/test/parallel/test-vm-basic.js
+++ b/test/parallel/test-vm-basic.js
@@ -24,60 +24,85 @@ const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
-// Test 1: vm.runInNewContext
-const sandbox = {};
-let result = vm.runInNewContext(
-  'foo = "bar"; this.typeofProcess = typeof process; typeof Object;',
-  sandbox
-);
-assert.deepStrictEqual(sandbox, {
-  foo: 'bar',
-  typeofProcess: 'undefined',
-});
-assert.strictEqual(result, 'function');
+// vm.runInNewContext
+{
+  const sandbox = {};
+  const result = vm.runInNewContext(
+    'foo = "bar"; this.typeofProcess = typeof process; typeof Object;',
+    sandbox
+  );
+  assert.deepStrictEqual(sandbox, {
+    foo: 'bar',
+    typeofProcess: 'undefined',
+  });
+  assert.strictEqual(result, 'function');
+}
 
-// Test 2: vm.runInContext
-const sandbox2 = { foo: 'bar' };
-const context = vm.createContext(sandbox2);
-result = vm.runInContext(
-  'baz = foo; this.typeofProcess = typeof process; typeof Object;',
-  context
-);
-assert.deepStrictEqual(sandbox2, {
-  foo: 'bar',
-  baz: 'bar',
-  typeofProcess: 'undefined'
-});
-assert.strictEqual(result, 'function');
+// vm.runInContext
+{
+  const sandbox = { foo: 'bar' };
+  const context = vm.createContext(sandbox);
+  const result = vm.runInContext(
+    'baz = foo; this.typeofProcess = typeof process; typeof Object;',
+    context
+  );
+  assert.deepStrictEqual(sandbox, {
+    foo: 'bar',
+    baz: 'bar',
+    typeofProcess: 'undefined'
+  });
+  assert.strictEqual(result, 'function');
+}
+
+// vm.runInThisContext
+{
+  const result = vm.runInThisContext(
+    'vmResult = "foo"; Object.prototype.toString.call(process);'
+  );
+  assert.strictEqual(global.vmResult, 'foo');
+  assert.strictEqual(result, '[object process]');
+  delete global.vmResult;
+}
+
+// vm.runInNewContext
+{
+  const result = vm.runInNewContext(
+    'vmResult = "foo"; typeof process;'
+  );
+  assert.strictEqual(global.vmResult, undefined);
+  assert.strictEqual(result, 'undefined');
+}
+
+// vm.createContext
+{
+  const sandbox = {};
+  const context = vm.createContext(sandbox);
+  assert.strictEqual(sandbox, context);
+}
 
-// Test 3: vm.runInThisContext
-result = vm.runInThisContext(
-  'vmResult = "foo"; Object.prototype.toString.call(process);'
-);
-assert.strictEqual(global.vmResult, 'foo');
-assert.strictEqual(result, '[object process]');
-delete global.vmResult;
+// Run script with filename
+{
+  const script = 'throw new Error("boom")';
+  const filename = 'test-boom-error';
+  const context = vm.createContext();
 
-// Test 4: vm.runInNewContext
-result = vm.runInNewContext(
-  'vmResult = "foo"; typeof process;'
-);
-assert.strictEqual(global.vmResult, undefined);
-assert.strictEqual(result, 'undefined');
+  function checkErr(err) {
+    return err.stack.startsWith('test-boom-error:1');
+  }
 
-// Test 5: vm.createContext
-const sandbox3 = {};
-const context2 = vm.createContext(sandbox3);
-assert.strictEqual(sandbox3, context2);
+  assert.throws(() => vm.runInContext(script, context, filename), checkErr);
+  assert.throws(() => vm.runInNewContext(script, context, filename), checkErr);
+  assert.throws(() => vm.runInThisContext(script, filename), checkErr);
+}
 
-// Test 6: invalid arguments
+// Invalid arguments
 [null, 'string'].forEach((input) => {
   common.expectsError(() => {
     vm.createContext({}, input);
   }, {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "options" argument must be of type object. ' +
+    message: 'The "options" argument must be of type Object. ' +
              `Received type ${typeof input}`
   });
 });
diff --git a/test/parallel/test-vm-cached-data.js b/test/parallel/test-vm-cached-data.js
index 5302d73c2d..e3947d1ae6 100644
--- a/test/parallel/test-vm-cached-data.js
+++ b/test/parallel/test-vm-cached-data.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const vm = require('vm');
 const spawnSync = require('child_process').spawnSync;
@@ -84,8 +84,12 @@ function testRejectSlice() {
 testRejectSlice();
 
 // It should throw on non-Buffer cachedData
-assert.throws(() => {
+common.expectsError(() => {
   new vm.Script('function abc() {}', {
     cachedData: 'ohai'
   });
-}, /^TypeError: options\.cachedData must be a Buffer instance$/);
+}, {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be one of type Buffer or Uint8Array/
+});
diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js
index fd6a66f392..31ed69da93 100644
--- a/test/parallel/test-vm-context.js
+++ b/test/parallel/test-vm-context.js
@@ -64,19 +64,25 @@ try {
 // This is outside of catch block to confirm catch block ran.
 assert.strictEqual(gh1140Exception.toString(), 'Error');
 
-// GH-558, non-context argument segfaults / raises assertion
-const nonContextualSandboxErrorMsg =
-  /^TypeError: contextifiedSandbox argument must be an object\.$/;
-const contextifiedSandboxErrorMsg =
-    /^TypeError: sandbox argument must have been converted to a context\.$/;
+const nonContextualSandboxError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be of type Object/
+};
+const contextifiedSandboxError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError,
+  message: /must be of type vm\.Context/
+};
+
 [
-  [undefined, nonContextualSandboxErrorMsg],
-  [null, nonContextualSandboxErrorMsg], [0, nonContextualSandboxErrorMsg],
-  [0.0, nonContextualSandboxErrorMsg], ['', nonContextualSandboxErrorMsg],
-  [{}, contextifiedSandboxErrorMsg], [[], contextifiedSandboxErrorMsg]
+  [undefined, nonContextualSandboxError],
+  [null, nonContextualSandboxError], [0, nonContextualSandboxError],
+  [0.0, nonContextualSandboxError], ['', nonContextualSandboxError],
+  [{}, contextifiedSandboxError], [[], contextifiedSandboxError]
 ].forEach((e) => {
-  assert.throws(() => { script.runInContext(e[0]); }, e[1]);
-  assert.throws(() => { vm.runInContext('', e[0]); }, e[1]);
+  common.expectsError(() => { script.runInContext(e[0]); }, e[1]);
+  common.expectsError(() => { vm.runInContext('', e[0]); }, e[1]);
 });
 
 // Issue GH-693:
diff --git a/test/parallel/test-vm-is-context.js b/test/parallel/test-vm-is-context.js
index a762622c67..2749a14834 100644
--- a/test/parallel/test-vm-is-context.js
+++ b/test/parallel/test-vm-is-context.js
@@ -35,13 +35,6 @@ for (const valToTest of [
   });
 }
 
-common.expectsError(() => {
-  vm.isContext();
-}, {
-  code: 'ERR_MISSING_ARGS',
-  type: TypeError
-});
-
 assert.strictEqual(vm.isContext({}), false);
 assert.strictEqual(vm.isContext([]), false);
 
diff --git a/test/parallel/test-vm-options-validation.js b/test/parallel/test-vm-options-validation.js
new file mode 100644
index 0000000000..e00a14e4b9
--- /dev/null
+++ b/test/parallel/test-vm-options-validation.js
@@ -0,0 +1,78 @@
+'use strict';
+
+const common = require('../common');
+const vm = require('vm');
+
+const invalidArgType = {
+  type: TypeError,
+  code: 'ERR_INVALID_ARG_TYPE'
+};
+
+const outOfRange = {
+  type: RangeError,
+  code: 'ERR_OUT_OF_RANGE'
+};
+
+common.expectsError(() => {
+  new vm.Script('void 0', 42);
+}, invalidArgType);
+
+[null, {}, [1], 'bad', true, 0.1].forEach((value) => {
+  common.expectsError(() => {
+    new vm.Script('void 0', { lineOffset: value });
+  }, invalidArgType);
+
+  common.expectsError(() => {
+    new vm.Script('void 0', { columnOffset: value });
+  }, invalidArgType);
+});
+
+common.expectsError(() => {
+  new vm.Script('void 0', { lineOffset: Number.MAX_SAFE_INTEGER });
+}, outOfRange);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { columnOffset: Number.MAX_SAFE_INTEGER });
+}, outOfRange);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { filename: 123 });
+}, invalidArgType);
+
+common.expectsError(() => {
+  new vm.Script('void 0', { produceCachedData: 1 });
+}, invalidArgType);
+
+[[0], {}, true, 'bad', 42].forEach((value) => {
+  common.expectsError(() => {
+    new vm.Script('void 0', { cachedData: value });
+  }, invalidArgType);
+});
+
+{
+  const script = new vm.Script('void 0');
+  const sandbox = vm.createContext();
+
+  function assertErrors(options) {
+    common.expectsError(() => {
+      script.runInThisContext(options);
+    }, invalidArgType);
+
+    common.expectsError(() => {
+      script.runInContext(sandbox, options);
+    }, invalidArgType);
+
+    common.expectsError(() => {
+      script.runInNewContext({}, options);
+    }, invalidArgType);
+  }
+
+  [null, 'bad', 42].forEach(assertErrors);
+  [{}, [1], 'bad', null, -1, 0, NaN].forEach((value) => {
+    assertErrors({ timeout: value });
+  });
+  [{}, [1], 'bad', 1, null].forEach((value) => {
+    assertErrors({ displayErrors: value });
+    assertErrors({ breakOnSigint: value });
+  });
+}
diff --git a/test/parallel/test-vm-timeout.js b/test/parallel/test-vm-timeout.js
index 51239ffed5..859992e99b 100644
--- a/test/parallel/test-vm-timeout.js
+++ b/test/parallel/test-vm-timeout.js
@@ -24,25 +24,15 @@ require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
-// Test 1: Timeout of 100ms executing endless loop
+// Timeout of 100ms executing endless loop
 assert.throws(function() {
   vm.runInThisContext('while(true) {}', { timeout: 100 });
 }, /^Error: Script execution timed out\.$/);
 
-// Test 2: Timeout must be >= 0ms
-assert.throws(function() {
-  vm.runInThisContext('', { timeout: -1 });
-}, /^RangeError: timeout must be a positive number$/);
-
-// Test 3: Timeout of 0ms
-assert.throws(function() {
-  vm.runInThisContext('', { timeout: 0 });
-}, /^RangeError: timeout must be a positive number$/);
-
-// Test 4: Timeout of 1000ms, script finishes first
+// Timeout of 1000ms, script finishes first
 vm.runInThisContext('', { timeout: 1000 });
 
-// Test 5: Nested vm timeouts, inner timeout propagates out
+// Nested vm timeouts, inner timeout propagates out
 assert.throws(function() {
   const context = {
     log: console.log,
@@ -54,7 +44,7 @@ assert.throws(function() {
   throw new Error('Test 5 failed');
 }, /Script execution timed out\./);
 
-// Test 6: Nested vm timeouts, outer timeout is shorter and fires first.
+// Nested vm timeouts, outer timeout is shorter and fires first.
 assert.throws(function() {
   const context = {
     runInVM: function(timeout) {
@@ -65,7 +55,7 @@ assert.throws(function() {
   throw new Error('Test 6 failed');
 }, /Script execution timed out\./);
 
-// Test 7: Nested vm timeouts, inner script throws an error.
+// Nested vm timeouts, inner script throws an error.
 assert.throws(function() {
   const context = {
     runInVM: function(timeout) {