vm: reject in importModuleDynamically without --experimental-vm-modules · nodejs/node@dda33c2 · GitHub
Skip to content

Commit dda33c2

Browse files
joyeecheungtargos
joyeecheung
authored and
targos
committed
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: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 3999362 commit dda33c2

7 files changed

Lines changed: 81 additions & 7 deletions

File tree

‎doc/api/errors.md‎

Lines changed: 6 additions & 0 deletions

‎doc/api/vm.md‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,9 @@ changes:
9898
when `import()` is called. If this option is not specified, calls to
9999
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
100100
This option is part of the experimental modules API. We do not recommend
101-
using it in a production environment.
101+
using it in a production environment. If `--experimental-vm-modules` isn't
102+
set, this callback will be ignored and calls to `import()` will reject with
103+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
102104
* `specifier` {string} specifier passed to `import()`
103105
* `script` {vm.Script}
104106
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -760,6 +762,9 @@ changes:
760762
* `importModuleDynamically` {Function} Called during evaluation of this module
761763
when `import()` is called. If this option is not specified, calls to
762764
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
765+
If `--experimental-vm-modules` isn't set, this callback will be ignored
766+
and calls to `import()` will reject with
767+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
763768
* `specifier` {string} specifier passed to `import()`
764769
* `module` {vm.Module}
765770
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -1018,7 +1023,9 @@ changes:
10181023
when `import()` is called. If this option is not specified, calls to
10191024
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
10201025
This option is part of the experimental modules API, and should not be
1021-
considered stable.
1026+
considered stable. If `--experimental-vm-modules` isn't
1027+
set, this callback will be ignored and calls to `import()` will reject with
1028+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
10221029
* `specifier` {string} specifier passed to `import()`
10231030
* `function` {Function}
10241031
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -1242,7 +1249,9 @@ changes:
12421249
when `import()` is called. If this option is not specified, calls to
12431250
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
12441251
This option is part of the experimental modules API. We do not recommend
1245-
using it in a production environment.
1252+
using it in a production environment. If `--experimental-vm-modules` isn't
1253+
set, this callback will be ignored and calls to `import()` will reject with
1254+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
12461255
* `specifier` {string} specifier passed to `import()`
12471256
* `script` {vm.Script}
12481257
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -1341,7 +1350,9 @@ changes:
13411350
when `import()` is called. If this option is not specified, calls to
13421351
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
13431352
This option is part of the experimental modules API. We do not recommend
1344-
using it in a production environment.
1353+
using it in a production environment. If `--experimental-vm-modules` isn't
1354+
set, this callback will be ignored and calls to `import()` will reject with
1355+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
13451356
* `specifier` {string} specifier passed to `import()`
13461357
* `script` {vm.Script}
13471358
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -1421,7 +1432,9 @@ changes:
14211432
when `import()` is called. If this option is not specified, calls to
14221433
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
14231434
This option is part of the experimental modules API. We do not recommend
1424-
using it in a production environment.
1435+
using it in a production environment. If `--experimental-vm-modules` isn't
1436+
set, this callback will be ignored and calls to `import()` will reject with
1437+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][].
14251438
* `specifier` {string} specifier passed to `import()`
14261439
* `script` {vm.Script}
14271440
* `importAssertions` {Object} The `"assert"` value passed to the
@@ -1585,6 +1598,7 @@ are not controllable through the timeout either.
15851598
[Source Text Module Record]: https://tc39.es/ecma262/#sec-source-text-module-records
15861599
[Synthetic Module Record]: https://heycam.github.io/webidl/#synthetic-module-records
15871600
[V8 Embedder's Guide]: https://v8.dev/docs/embed#contexts
1601+
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`]: errors.md#err_vm_dynamic_import_callback_missing_flag
15881602
[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`]: errors.md#err_vm_dynamic_import_callback_missing
15891603
[`ERR_VM_MODULE_STATUS`]: errors.md#err_vm_module_status
15901604
[`Error`]: errors.md#class-error

‎lib/internal/errors.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1821,6 +1821,9 @@ E('ERR_VALID_PERFORMANCE_ENTRY_TYPE',
18211821
'At least one valid performance entry type is required', Error);
18221822
E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING',
18231823
'A dynamic import callback was not specified.', TypeError);
1824+
E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG',
1825+
'A dynamic import callback was invoked without --experimental-vm-modules',
1826+
TypeError);
18241827
E('ERR_VM_MODULE_ALREADY_LINKED', 'Module has already been linked', Error);
18251828
E('ERR_VM_MODULE_CANNOT_CREATE_CACHED_DATA',
18261829
'Cached data cannot be created for a module which has been evaluated', Error);

‎lib/internal/modules/esm/utils.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@ const {
1414
} = internalBinding('util');
1515
const {
1616
default_host_defined_options,
17+
vm_dynamic_import_missing_flag,
1718
} = internalBinding('symbols');
1819

1920
const {
21+
ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG,
2022
ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
2123
ERR_INVALID_ARG_VALUE,
2224
} = require('internal/errors').codes;
@@ -132,7 +134,8 @@ const moduleRegistries = new SafeWeakMap();
132134
*/
133135
function registerModule(referrer, registry) {
134136
const idSymbol = referrer[host_defined_option_symbol];
135-
if (idSymbol === default_host_defined_options) {
137+
if (idSymbol === default_host_defined_options ||
138+
idSymbol === vm_dynamic_import_missing_flag) {
136139
// The referrer is compiled without custom callbacks, so there is
137140
// no registry to hold on to. We'll throw
138141
// ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is
@@ -173,6 +176,9 @@ async function importModuleDynamicallyCallback(symbol, specifier, attributes) {
173176
return importModuleDynamically(specifier, callbackReferrer, attributes);
174177
}
175178
}
179+
if (symbol === vm_dynamic_import_missing_flag) {
180+
throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG();
181+
}
176182
throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
177183
}
178184

‎lib/internal/vm.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,19 @@ const {
1515
} = ContextifyScript.prototype;
1616
const {
1717
default_host_defined_options,
18+
vm_dynamic_import_missing_flag,
1819
} = internalBinding('symbols');
1920
const {
2021
validateFunction,
2122
validateObject,
2223
kValidateObjectAllowArray,
2324
} = require('internal/validators');
2425

26+
const {
27+
getOptionValue,
28+
} = require('internal/options');
29+
30+
2531
function isContext(object) {
2632
validateObject(object, 'object', kValidateObjectAllowArray);
2733

@@ -41,6 +47,16 @@ function getHostDefinedOptionId(importModuleDynamically, filename) {
4147
// compilation cache can be hit.
4248
return default_host_defined_options;
4349
}
50+
// We should've thrown here immediately when we introduced
51+
// --experimental-vm-modules and importModuleDynamically, but since
52+
// users are already using this callback to throw a similar error,
53+
// we also defer the error to the time when an actual import() is called
54+
// to avoid breaking them. To ensure that the isolate compilation
55+
// cache can still be hit, use a constant sentinel symbol here.
56+
if (!getOptionValue('--experimental-vm-modules')) {
57+
return vm_dynamic_import_missing_flag;
58+
}
59+
4460
return Symbol(filename);
4561
}
4662

‎src/env_properties.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@
4646
V(owner_symbol, "owner_symbol") \
4747
V(onpskexchange_symbol, "onpskexchange") \
4848
V(resource_symbol, "resource_symbol") \
49-
V(trigger_async_id_symbol, "trigger_async_id_symbol")
49+
V(trigger_async_id_symbol, "trigger_async_id_symbol") \
50+
V(vm_dynamic_import_missing_flag, "vm_dynamic_import_missing_flag")
5051

5152
// Strings are per-isolate primitives but Environment proxies them
5253
// for the sake of convenience. Strings should be ASCII-only.

‎test/parallel/test-vm-dynamic-import-callback-missing-flag.js‎

Lines changed: 28 additions & 0 deletions

0 commit comments

Comments
 (0)