Skip to content

Commit db17461

Browse files
joyeecheungjoyeecheung
authored
module: disallow CJS <-> ESM edges in a cycle from require(esm)
This patch disallows CJS <-> ESM edges when they come from require(esm) requested in ESM evalaution. Drive-by: don't reuse the cache for imported CJS modules to stash source code of required ESM because the former is also used for cycle detection. PR-URL: nodejs#52264 Fixes: nodejs#52145 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 45f0dd0 commit db17461

28 files changed

Lines changed: 499 additions & 47 deletions

‎doc/api/errors.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2489,6 +2489,21 @@ Accessing `Object.prototype.__proto__` has been forbidden using
24892489
[`Object.setPrototypeOf`][] should be used to get and set the prototype of an
24902490
object.
24912491

2492+
<a id="ERR_REQUIRE_CYCLE_MODULE"></a>
2493+
2494+
### `ERR_REQUIRE_CYCLE_MODULE`
2495+
2496+
> Stability: 1 - Experimental
2497+
2498+
When trying to `require()` a [ES Module][] under `--experimental-require-module`,
2499+
a CommonJS to ESM or ESM to CommonJS edge participates in an immediate cycle.
2500+
This is not allowed because ES Modules cannot be evaluated while they are
2501+
already being evaluated.
2502+
2503+
To avoid the cycle, the `require()` call involved in a cycle should not happen
2504+
at the top-level of either a ES Module (via `createRequire()`) or a CommonJS
2505+
module, and should be done lazily in an inner function.
2506+
24922507
<a id="ERR_REQUIRE_ASYNC_MODULE"></a>
24932508

24942509
### `ERR_REQUIRE_ASYNC_MODULE`

‎lib/internal/errors.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1683,6 +1683,7 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
16831683
E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
16841684
'%d is not a valid timestamp', TypeError);
16851685
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
1686+
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
16861687
E('ERR_REQUIRE_ESM',
16871688
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
16881689
hideInternalStackFrames(this);

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -63,25 +63,34 @@ const {
6363
Symbol,
6464
} = primordials;
6565

66+
const { kEvaluated } = internalBinding('module_wrap');
67+
6668
// Map used to store CJS parsing data or for ESM loading.
67-
const cjsSourceCache = new SafeWeakMap();
69+
const importedCJSCache = new SafeWeakMap();
6870
/**
6971
* Map of already-loaded CJS modules to use.
7072
*/
7173
const cjsExportsCache = new SafeWeakMap();
74+
const requiredESMSourceCache = new SafeWeakMap();
7275

76+
const kIsMainSymbol = Symbol('kIsMainSymbol');
77+
const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
78+
const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
79+
const kIsExecuting = Symbol('kIsExecuting');
7380
// Set first due to cycle with ESM loader functions.
7481
module.exports = {
7582
cjsExportsCache,
76-
cjsSourceCache,
83+
importedCJSCache,
7784
initializeCJS,
7885
entryPointSource: undefined, // Set below.
7986
Module,
8087
wrapSafe,
88+
kIsMainSymbol,
89+
kIsCachedByESMLoader,
90+
kRequiredModuleSymbol,
91+
kIsExecuting,
8192
};
8293

83-
const kIsMainSymbol = Symbol('kIsMainSymbol');
84-
8594
const { BuiltinModule } = require('internal/bootstrap/realm');
8695
const {
8796
maybeCacheSourceMap,
@@ -138,6 +147,7 @@ const {
138147
codes: {
139148
ERR_INVALID_ARG_VALUE,
140149
ERR_INVALID_MODULE_SPECIFIER,
150+
ERR_REQUIRE_CYCLE_MODULE,
141151
ERR_REQUIRE_ESM,
142152
ERR_UNKNOWN_BUILTIN_MODULE,
143153
},
@@ -942,6 +952,16 @@ const CircularRequirePrototypeWarningProxy = new Proxy({}, {
942952
* @param {Module} module The module instance
943953
*/
944954
function getExportsForCircularRequire(module) {
955+
const requiredESM = module[kRequiredModuleSymbol];
956+
if (requiredESM && requiredESM.getStatus() !== kEvaluated) {
957+
let message = `Cannot require() ES Module ${module.id} in a cycle.`;
958+
const parent = moduleParentCache.get(module);
959+
if (parent) {
960+
message += ` (from ${parent.filename})`;
961+
}
962+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
963+
}
964+
945965
if (module.exports &&
946966
!isProxy(module.exports) &&
947967
ObjectGetPrototypeOf(module.exports) === ObjectPrototype &&
@@ -1009,11 +1029,21 @@ Module._load = function(request, parent, isMain) {
10091029
if (cachedModule !== undefined) {
10101030
updateChildren(parent, cachedModule, true);
10111031
if (!cachedModule.loaded) {
1012-
const parseCachedModule = cjsSourceCache.get(cachedModule);
1013-
if (!parseCachedModule || parseCachedModule.loaded) {
1032+
// If it's not cached by the ESM loader, the loading request
1033+
// comes from required CJS, and we can consider it a circular
1034+
// dependency when it's cached.
1035+
if (!cachedModule[kIsCachedByESMLoader]) {
10141036
return getExportsForCircularRequire(cachedModule);
10151037
}
1016-
parseCachedModule.loaded = true;
1038+
// If it's cached by the ESM loader as a way to indirectly pass
1039+
// the module in to avoid creating it twice, the loading request
1040+
// come from imported CJS. In that case use the importedCJSCache
1041+
// to determine if it's loading or not.
1042+
const importedCJSMetadata = importedCJSCache.get(cachedModule);
1043+
if (importedCJSMetadata.loading) {
1044+
return getExportsForCircularRequire(cachedModule);
1045+
}
1046+
importedCJSMetadata.loading = true;
10171047
} else {
10181048
return cachedModule.exports;
10191049
}
@@ -1027,18 +1057,21 @@ Module._load = function(request, parent, isMain) {
10271057
// Don't call updateChildren(), Module constructor already does.
10281058
const module = cachedModule || new Module(filename, parent);
10291059

1030-
if (isMain) {
1031-
setOwnProperty(process, 'mainModule', module);
1032-
setOwnProperty(module.require, 'main', process.mainModule);
1033-
module.id = '.';
1034-
module[kIsMainSymbol] = true;
1035-
} else {
1036-
module[kIsMainSymbol] = false;
1037-
}
1060+
if (!cachedModule) {
1061+
if (isMain) {
1062+
setOwnProperty(process, 'mainModule', module);
1063+
setOwnProperty(module.require, 'main', process.mainModule);
1064+
module.id = '.';
1065+
module[kIsMainSymbol] = true;
1066+
} else {
1067+
module[kIsMainSymbol] = false;
1068+
}
10381069

1039-
reportModuleToWatchMode(filename);
1070+
reportModuleToWatchMode(filename);
1071+
Module._cache[filename] = module;
1072+
module[kIsCachedByESMLoader] = false;
1073+
}
10401074

1041-
Module._cache[filename] = module;
10421075
if (parent !== undefined) {
10431076
relativeResolveCache[relResolveCacheIdentifier] = filename;
10441077
}
@@ -1280,7 +1313,7 @@ function loadESMFromCJS(mod, filename) {
12801313
const isMain = mod[kIsMainSymbol];
12811314
// TODO(joyeecheung): we may want to invent optional special handling for default exports here.
12821315
// For now, it's good enough to be identical to what `import()` returns.
1283-
mod.exports = cascadedLoader.importSyncForRequire(filename, source, isMain);
1316+
mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, moduleParentCache.get(mod));
12841317
}
12851318

12861319
/**
@@ -1373,7 +1406,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
13731406
// Only modules being require()'d really need to avoid TLA.
13741407
if (loadAsESM) {
13751408
// Pass the source into the .mjs extension handler indirectly through the cache.
1376-
cjsSourceCache.set(this, { source: content });
1409+
requiredESMSourceCache.set(this, content);
13771410
loadESMFromCJS(this, filename);
13781411
return;
13791412
}
@@ -1414,13 +1447,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
14141447
const module = this;
14151448
if (requireDepth === 0) { statCache = new SafeMap(); }
14161449
setHasStartedUserCJSExecution();
1450+
this[kIsExecuting] = true;
14171451
if (inspectorWrapper) {
14181452
result = inspectorWrapper(compiledWrapper, thisValue, exports,
14191453
require, module, filename, dirname);
14201454
} else {
14211455
result = ReflectApply(compiledWrapper, thisValue,
14221456
[exports, require, module, filename, dirname]);
14231457
}
1458+
this[kIsExecuting] = false;
14241459
if (requireDepth === 0) { statCache = null; }
14251460
return result;
14261461
};
@@ -1432,15 +1467,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
14321467
* @returns {string}
14331468
*/
14341469
function getMaybeCachedSource(mod, filename) {
1435-
const cached = cjsSourceCache.get(mod);
1470+
const cached = importedCJSCache.get(mod);
14361471
let content;
14371472
if (cached?.source) {
14381473
content = cached.source;
14391474
cached.source = undefined;
14401475
} else {
14411476
// TODO(joyeecheung): we can read a buffer instead to speed up
14421477
// compilation.
1443-
content = fs.readFileSync(filename, 'utf8');
1478+
content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8');
14441479
}
14451480
return content;
14461481
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,11 @@ async function defaultLoad(url, context = kEmptyObject) {
152152

153153
validateAttributes(url, format, importAttributes);
154154

155+
// Use the synchronous commonjs translator which can deal with cycles.
156+
if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
157+
format = 'commonjs-sync';
158+
}
159+
155160
return {
156161
__proto__: null,
157162
format,
@@ -201,6 +206,11 @@ function defaultLoadSync(url, context = kEmptyObject) {
201206

202207
validateAttributes(url, format, importAttributes);
203208

209+
// Use the synchronous commonjs translator which can deal with cycles.
210+
if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
211+
format = 'commonjs-sync';
212+
}
213+
204214
return {
205215
__proto__: null,
206216
format,

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

Lines changed: 65 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
'use strict';
22

33
// This is needed to avoid cycles in esm/resolve <-> cjs/loader
4-
require('internal/modules/cjs/loader');
4+
const {
5+
kIsExecuting,
6+
kRequiredModuleSymbol,
7+
} = require('internal/modules/cjs/loader');
58

69
const {
710
ArrayPrototypeJoin,
@@ -15,8 +18,11 @@ const {
1518
hardenRegExp,
1619
} = primordials;
1720

21+
const { imported_cjs_symbol } = internalBinding('symbols');
22+
1823
const assert = require('internal/assert');
1924
const {
25+
ERR_REQUIRE_CYCLE_MODULE,
2026
ERR_REQUIRE_ESM,
2127
ERR_NETWORK_IMPORT_DISALLOWED,
2228
ERR_UNKNOWN_MODULE_FORMAT,
@@ -30,7 +36,10 @@ const {
3036
} = require('internal/modules/esm/utils');
3137
const { kImplicitAssertType } = require('internal/modules/esm/assert');
3238
const { canParse } = internalBinding('url');
33-
const { ModuleWrap } = internalBinding('module_wrap');
39+
const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
40+
const {
41+
urlToFilename,
42+
} = require('internal/modules/helpers');
3443
let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
3544

3645
/**
@@ -248,17 +257,36 @@ class ModuleLoader {
248257
/**
249258
* This constructs (creates, instantiates and evaluates) a module graph that
250259
* is require()'d.
260+
* @param {import('../cjs/loader.js').Module} mod CJS module wrapper of the ESM.
251261
* @param {string} filename Resolved filename of the module being require()'d
252262
* @param {string} source Source code. TODO(joyeecheung): pass the raw buffer.
253263
* @param {string} isMain Whether this module is a main module.
254-
* @returns {ModuleNamespaceObject}
264+
* @param {import('../cjs/loader.js').Module|undefined} parent Parent module, if any.
265+
* @returns {{ModuleWrap}}
255266
*/
256-
importSyncForRequire(filename, source, isMain) {
267+
importSyncForRequire(mod, filename, source, isMain, parent) {
257268
const url = pathToFileURL(filename).href;
258269
let job = this.loadCache.get(url, kImplicitAssertType);
259-
// This module is already loaded, check whether it's synchronous and return the
260-
// namespace.
270+
// This module job is already created:
271+
// 1. If it was loaded by `require()` before, at this point the instantiation
272+
// is already completed and we can check the whether it is in a cycle
273+
// (in that case the module status is kEvaluaing), and whether the
274+
// required graph is synchronous.
275+
// 2. If it was loaded by `import` before, only allow it if it's already evaluated
276+
// to forbid cycles.
277+
// TODO(joyeecheung): ensure that imported synchronous graphs are evaluated
278+
// synchronously so that any previously imported synchronous graph is already
279+
// evaluated at this point.
261280
if (job !== undefined) {
281+
mod[kRequiredModuleSymbol] = job.module;
282+
if (job.module.getStatus() !== kEvaluated) {
283+
const parentFilename = urlToFilename(parent?.filename);
284+
let message = `Cannot require() ES Module ${filename} in a cycle.`;
285+
if (parentFilename) {
286+
message += ` (from ${parentFilename})`;
287+
}
288+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
289+
}
262290
return job.module.getNamespaceSync();
263291
}
264292
// TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
@@ -270,6 +298,7 @@ class ModuleLoader {
270298
const { ModuleJobSync } = require('internal/modules/esm/module_job');
271299
job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk);
272300
this.loadCache.set(url, kImplicitAssertType, job);
301+
mod[kRequiredModuleSymbol] = job.module;
273302
return job.runSync().namespace;
274303
}
275304

@@ -304,19 +333,29 @@ class ModuleLoader {
304333
const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes;
305334
let job = this.loadCache.get(url, resolvedImportAttributes.type);
306335
if (job !== undefined) {
307-
// This module is previously imported before. We will return the module now and check
308-
// asynchronicity of the entire graph later, after the graph is instantiated.
336+
// This module is being evaluated, which means it's imported in a previous link
337+
// in a cycle.
338+
if (job.module.getStatus() === kEvaluating) {
339+
const parentFilename = urlToFilename(parentURL);
340+
let message = `Cannot import Module ${specifier} in a cycle.`;
341+
if (parentFilename) {
342+
message += ` (from ${parentFilename})`;
343+
}
344+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
345+
}
346+
// Othersie the module could be imported before but the evaluation may be already
347+
// completed (e.g. the require call is lazy) so it's okay. We will return the
348+
// module now and check asynchronicity of the entire graph later, after the
349+
// graph is instantiated.
309350
return job.module;
310351
}
311352

312353
defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
313354
const loadResult = defaultLoadSync(url, { format, importAttributes });
314355
const { responseURL, source } = loadResult;
315-
let { format: finalFormat } = loadResult;
356+
const { format: finalFormat } = loadResult;
316357
this.validateLoadResult(url, finalFormat);
317-
if (finalFormat === 'commonjs') {
318-
finalFormat = 'commonjs-sync';
319-
} else if (finalFormat === 'wasm') {
358+
if (finalFormat === 'wasm') {
320359
assert.fail('WASM is currently unsupported by require(esm)');
321360
}
322361

@@ -333,6 +372,20 @@ class ModuleLoader {
333372
process.send({ 'watch:import': [url] });
334373
}
335374

375+
const cjsModule = wrap[imported_cjs_symbol];
376+
if (cjsModule) {
377+
assert(finalFormat === 'commonjs-sync');
378+
// Check if the ESM initiating import CJS is being required by the same CJS module.
379+
if (cjsModule && cjsModule[kIsExecuting]) {
380+
const parentFilename = urlToFilename(parentURL);
381+
let message = `Cannot import CommonJS Module ${specifier} in a cycle.`;
382+
if (parentFilename) {
383+
message += ` (from ${parentFilename})`;
384+
}
385+
throw new ERR_REQUIRE_CYCLE_MODULE(message);
386+
}
387+
}
388+
336389
const inspectBrk = (isMain && getOptionValue('--inspect-brk'));
337390
const { ModuleJobSync } = require('internal/modules/esm/module_job');
338391
job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk);

0 commit comments

Comments
 (0)