vm: reject in importModuleDynamically without --experimental-vm-modules

Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
joyeecheung · Dec 1, 2023 · a2b3f6e · a2b3f6e
1 parent 222877e
commit a2b3f6e
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 7 deletions.
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -2985,6 +2985,12 @@ An attempt was made to use something that was already closed.
 While using the Performance Timing API (`perf_hooks`), no valid performance
 entry types are found.
 
+<a id="ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG"></a>
+
+### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`
+
+A dynamic import callback was invoked without `--experimental-vm-modules`.
+
 <a id="ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING"></a>
 
 ### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`

diff --git a/doc/api/vm.md b/doc/api/vm.md
@@ -98,7 +98,9 @@ changes:
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     This option is part of the experimental modules API. We do not recommend
-    using it in a production environment.
+    using it in a production environment. If `--experimental-vm-modules` isn't
+    set, this callback will be ignored and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -765,6 +767,9 @@ changes:
   * `importModuleDynamically` {Function} Called during evaluation of this module
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
+    If `--experimental-vm-modules` isn't set, this callback will be ignored
+    and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `module` {vm.Module}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -1022,7 +1027,9 @@ changes:
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     This option is part of the experimental modules API, and should not be
-    considered stable.
+    considered stable.  If `--experimental-vm-modules` isn't
+    set, this callback will be ignored and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `function` {Function}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -1246,7 +1253,9 @@ changes:
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     This option is part of the experimental modules API. We do not recommend
-    using it in a production environment.
+    using it in a production environment.  If `--experimental-vm-modules` isn't
+    set, this callback will be ignored and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -1345,7 +1354,9 @@ changes:
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     This option is part of the experimental modules API. We do not recommend
-    using it in a production environment.
+    using it in a production environment. If `--experimental-vm-modules` isn't
+    set, this callback will be ignored and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -1425,7 +1436,9 @@ changes:
     when `import()` is called. If this option is not specified, calls to
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     This option is part of the experimental modules API. We do not recommend
-    using it in a production environment.
+    using it in a production environment. If `--experimental-vm-modules` isn't
+    set, this callback will be ignored and calls to `import()` will reject with
+    [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAttributes` {Object} The `"assert"` value passed to the
@@ -1589,6 +1602,7 @@ are not controllable through the timeout either.
 [Source Text Module Record]: https://tc39.es/ecma262/#sec-source-text-module-records
 [Synthetic Module Record]: https://heycam.github.io/webidl/#synthetic-module-records
 [V8 Embedder's Guide]: https://v8.dev/docs/embed#contexts
+[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`]: errors.md#err_vm_dynamic_import_callback_missing_flag
 [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`]: errors.md#err_vm_dynamic_import_callback_missing
 [`ERR_VM_MODULE_STATUS`]: errors.md#err_vm_module_status
 [`Error`]: errors.md#class-error

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -1716,6 +1716,9 @@ E('ERR_VALID_PERFORMANCE_ENTRY_TYPE',
   'At least one valid performance entry type is required', Error);
 E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING',
   'A dynamic import callback was not specified.', TypeError);
+E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG',
+  'A dynamic import callback was invoked without --experimental-vm-modules',
+  TypeError);
 E('ERR_VM_MODULE_ALREADY_LINKED', 'Module has already been linked', Error);
 E('ERR_VM_MODULE_CANNOT_CREATE_CACHED_DATA',
   'Cached data cannot be created for a module which has been evaluated', Error);

diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js
@@ -14,9 +14,11 @@ const {
 } = internalBinding('util');
 const {
   default_host_defined_options,
+  vm_dynamic_import_missing_flag,
 } = internalBinding('symbols');
 
 const {
+  ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG,
   ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
   ERR_INVALID_ARG_VALUE,
 } = require('internal/errors').codes;
@@ -132,7 +134,8 @@ const moduleRegistries = new SafeWeakMap();
  */
 function registerModule(referrer, registry) {
   const idSymbol = referrer[host_defined_option_symbol];
-  if (idSymbol === default_host_defined_options) {
+  if (idSymbol === default_host_defined_options ||
+      idSymbol === vm_dynamic_import_missing_flag) {
     // The referrer is compiled without custom callbacks, so there is
     // no registry to hold on to. We'll throw
     // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is
@@ -173,6 +176,9 @@ async function importModuleDynamicallyCallback(symbol, specifier, attributes) {
       return importModuleDynamically(specifier, callbackReferrer, attributes);
     }
   }
+  if (symbol === vm_dynamic_import_missing_flag) {
+    throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG();
+  }
   throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
 }
 

diff --git a/lib/internal/vm.js b/lib/internal/vm.js
@@ -15,12 +15,18 @@ const {
 } = ContextifyScript.prototype;
 const {
   default_host_defined_options,
+  vm_dynamic_import_missing_flag,
 } = internalBinding('symbols');
 const {
   validateFunction,
   validateObject,
 } = require('internal/validators');
 
+const {
+  getOptionValue,
+} = require('internal/options');
+
+
 function isContext(object) {
   validateObject(object, 'object', { __proto__: null, allowArray: true });
 
@@ -40,6 +46,16 @@ function getHostDefinedOptionId(importModuleDynamically, filename) {
     // compilation cache can be hit.
     return default_host_defined_options;
   }
+  // We should've thrown here immediately when we introduced
+  // --experimental-vm-modules and importModuleDynamically, but since
+  // users are already using this callback to throw a similar error,
+  // we also defer the error to the time when an actual import() is called
+  // to avoid breaking them. To ensure that the isolate compilation
+  // cache can still be hit, use a constant sentinel symbol here.
+  if (!getOptionValue('--experimental-vm-modules')) {
+    return vm_dynamic_import_missing_flag;
+  }
+
   return Symbol(filename);
 }
 

diff --git a/src/env_properties.h b/src/env_properties.h
@@ -44,7 +44,8 @@
   V(owner_symbol, "owner_symbol")                                              \
   V(onpskexchange_symbol, "onpskexchange")                                     \
   V(resource_symbol, "resource_symbol")                                        \
-  V(trigger_async_id_symbol, "trigger_async_id_symbol")
+  V(trigger_async_id_symbol, "trigger_async_id_symbol")                        \
+  V(vm_dynamic_import_missing_flag, "vm_dynamic_import_missing_flag")
 
 // Strings are per-isolate primitives but Environment proxies them
 // for the sake of convenience.  Strings should be ASCII-only.

diff --git a/test/parallel/test-vm-dynamic-import-callback-missing-flag.js b/test/parallel/test-vm-dynamic-import-callback-missing-flag.js
@@ -0,0 +1,28 @@
+'use strict';
+
+const common = require('../common');
+const { Script, compileFunction } = require('vm');
+const assert = require('assert');
+
+assert(
+  !process.execArgv.includes('--experimental-vm-modules'),
+  'This test must be run without --experimental-vm-modules');
+
+assert.rejects(async () => {
+  const script = new Script('import("fs")', {
+    importModuleDynamically: common.mustNotCall(),
+  });
+  const imported = script.runInThisContext();
+  await imported;
+}, {
+  code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG'
+}).then(common.mustCall());
+
+assert.rejects(async () => {
+  const imported = compileFunction('return import("fs")', [], {
+    importModuleDynamically: common.mustNotCall(),
+  })();
+  await imported;
+}, {
+  code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG'
+}).then(common.mustCall());