module: disallow CJS <-> ESM edges in a cycle from require(esm) · nodejs/node@51b88fa · GitHub
Skip to content

Commit 51b88fa

Browse files
joyeecheungmarco-ippolito
authored andcommitted
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: #52264 Backport-PR-URL: #53500 Fixes: #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 4dae68c commit 51b88fa

28 files changed

Lines changed: 499 additions & 47 deletions

doc/api/errors.md

Lines changed: 15 additions & 0 deletions

lib/internal/errors.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,6 +1692,7 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
16921692
E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
16931693
'%d is not a valid timestamp', TypeError);
16941694
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
1695+
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
16951696
E('ERR_REQUIRE_ESM',
16961697
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
16971698
hideInternalStackFrames(this);

lib/internal/modules/cjs/loader.js

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -63,24 +63,33 @@ 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
Module,
7986
wrapSafe,
87+
kIsMainSymbol,
88+
kIsCachedByESMLoader,
89+
kRequiredModuleSymbol,
90+
kIsExecuting,
8091
};
8192

82-
const kIsMainSymbol = Symbol('kIsMainSymbol');
83-
8493
const { BuiltinModule } = require('internal/bootstrap/realm');
8594
const {
8695
maybeCacheSourceMap,
@@ -137,6 +146,7 @@ const {
137146
codes: {
138147
ERR_INVALID_ARG_VALUE,
139148
ERR_INVALID_MODULE_SPECIFIER,
149+
ERR_REQUIRE_CYCLE_MODULE,
140150
ERR_REQUIRE_ESM,
141151
ERR_UNKNOWN_BUILTIN_MODULE,
142152
},
@@ -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
/**
@@ -1366,7 +1399,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
13661399
// Only modules being require()'d really need to avoid TLA.
13671400
if (loadAsESM) {
13681401
// Pass the source into the .mjs extension handler indirectly through the cache.
1369-
cjsSourceCache.set(this, { source: content });
1402+
requiredESMSourceCache.set(this, content);
13701403
loadESMFromCJS(this, filename);
13711404
return;
13721405
}
@@ -1407,13 +1440,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
14071440
const module = this;
14081441
if (requireDepth === 0) { statCache = new SafeMap(); }
14091442
setHasStartedUserCJSExecution();
1443+
this[kIsExecuting] = true;
14101444
if (inspectorWrapper) {
14111445
result = inspectorWrapper(compiledWrapper, thisValue, exports,
14121446
require, module, filename, dirname);
14131447
} else {
14141448
result = ReflectApply(compiledWrapper, thisValue,
14151449
[exports, require, module, filename, dirname]);
14161450
}
1451+
this[kIsExecuting] = false;
14171452
if (requireDepth === 0) { statCache = null; }
14181453
return result;
14191454
};
@@ -1425,15 +1460,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
14251460
* @returns {string}
14261461
*/
14271462
function getMaybeCachedSource(mod, filename) {
1428-
const cached = cjsSourceCache.get(mod);
1463+
const cached = importedCJSCache.get(mod);
14291464
let content;
14301465
if (cached?.source) {
14311466
content = cached.source;
14321467
cached.source = undefined;
14331468
} else {
14341469
// TODO(joyeecheung): we can read a buffer instead to speed up
14351470
// compilation.
1436-
content = fs.readFileSync(filename, 'utf8');
1471+
content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8');
14371472
}
14381473
return content;
14391474
}

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

0 commit comments

Comments
 (0)