Skip to content

Commit

Permalink
module: disallow CJS <-> ESM edges in a cycle from require(esm)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
  • Loading branch information
@joyeecheung
joyeecheung authored Apr 8, 2024
1 parent 45f0dd0 commit db17461
Show file tree
Hide file tree
Showing 28 changed files with 499 additions and 47 deletions.
15 changes: 15 additions & 0 deletions doc/api/errors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2489,6 +2489,21 @@ Accessing `Object.prototype.__proto__` has been forbidden using
[`Object.setPrototypeOf`][] should be used to get and set the prototype of an
object.

<a id="ERR_REQUIRE_CYCLE_MODULE"></a>

### `ERR_REQUIRE_CYCLE_MODULE`

> Stability: 1 - Experimental

When trying to `require()` a [ES Module][] under `--experimental-require-module`,
a CommonJS to ESM or ESM to CommonJS edge participates in an immediate cycle.
This is not allowed because ES Modules cannot be evaluated while they are
already being evaluated.

To avoid the cycle, the `require()` call involved in a cycle should not happen
at the top-level of either a ES Module (via `createRequire()`) or a CommonJS
module, and should be done lazily in an inner function.

<a id="ERR_REQUIRE_ASYNC_MODULE"></a>

### `ERR_REQUIRE_ASYNC_MODULE`
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,7 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
'%d is not a valid timestamp', TypeError);
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
E('ERR_REQUIRE_ESM',
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
hideInternalStackFrames(this);
Expand Down
77 changes: 56 additions & 21 deletions lib/internal/modules/cjs/loader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,25 +63,34 @@ const {
Symbol,
} = primordials;

const { kEvaluated } = internalBinding('module_wrap');

// Map used to store CJS parsing data or for ESM loading.
const cjsSourceCache = new SafeWeakMap();
const importedCJSCache = new SafeWeakMap();
/**
* Map of already-loaded CJS modules to use.
*/
const cjsExportsCache = new SafeWeakMap();
const requiredESMSourceCache = new SafeWeakMap();

const kIsMainSymbol = Symbol('kIsMainSymbol');
const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
const kIsExecuting = Symbol('kIsExecuting');
// Set first due to cycle with ESM loader functions.
module.exports = {
cjsExportsCache,
cjsSourceCache,
importedCJSCache,
initializeCJS,
entryPointSource: undefined, // Set below.
Module,
wrapSafe,
kIsMainSymbol,
kIsCachedByESMLoader,
kRequiredModuleSymbol,
kIsExecuting,
};

const kIsMainSymbol = Symbol('kIsMainSymbol');

const { BuiltinModule } = require('internal/bootstrap/realm');
const {
maybeCacheSourceMap,
Expand Down Expand Up @@ -138,6 +147,7 @@ const {
codes: {
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_REQUIRE_CYCLE_MODULE,
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
},
Expand Down Expand Up @@ -942,6 +952,16 @@ const CircularRequirePrototypeWarningProxy = new Proxy({}, {
* @param {Module} module The module instance
*/
function getExportsForCircularRequire(module) {
const requiredESM = module[kRequiredModuleSymbol];
if (requiredESM && requiredESM.getStatus() !== kEvaluated) {
let message = `Cannot require() ES Module ${module.id} in a cycle.`;
const parent = moduleParentCache.get(module);
if (parent) {
message += ` (from ${parent.filename})`;
}
throw new ERR_REQUIRE_CYCLE_MODULE(message);
}

if (module.exports &&
!isProxy(module.exports) &&
ObjectGetPrototypeOf(module.exports) === ObjectPrototype &&
Expand Down Expand Up @@ -1009,11 +1029,21 @@ Module._load = function(request, parent, isMain) {
if (cachedModule !== undefined) {
updateChildren(parent, cachedModule, true);
if (!cachedModule.loaded) {
const parseCachedModule = cjsSourceCache.get(cachedModule);
if (!parseCachedModule || parseCachedModule.loaded) {
// If it's not cached by the ESM loader, the loading request
// comes from required CJS, and we can consider it a circular
// dependency when it's cached.
if (!cachedModule[kIsCachedByESMLoader]) {
return getExportsForCircularRequire(cachedModule);
}
parseCachedModule.loaded = true;
// If it's cached by the ESM loader as a way to indirectly pass
// the module in to avoid creating it twice, the loading request
// come from imported CJS. In that case use the importedCJSCache
// to determine if it's loading or not.
const importedCJSMetadata = importedCJSCache.get(cachedModule);
if (importedCJSMetadata.loading) {
return getExportsForCircularRequire(cachedModule);
}
importedCJSMetadata.loading = true;
} else {
return cachedModule.exports;
}
Expand All @@ -1027,18 +1057,21 @@ Module._load = function(request, parent, isMain) {
// Don't call updateChildren(), Module constructor already does.
const module = cachedModule || new Module(filename, parent);

if (isMain) {
setOwnProperty(process, 'mainModule', module);
setOwnProperty(module.require, 'main', process.mainModule);
module.id = '.';
module[kIsMainSymbol] = true;
} else {
module[kIsMainSymbol] = false;
}
if (!cachedModule) {
if (isMain) {
setOwnProperty(process, 'mainModule', module);
setOwnProperty(module.require, 'main', process.mainModule);
module.id = '.';
module[kIsMainSymbol] = true;
} else {
module[kIsMainSymbol] = false;
}

reportModuleToWatchMode(filename);
reportModuleToWatchMode(filename);
Module._cache[filename] = module;
module[kIsCachedByESMLoader] = false;
}

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

/**
Expand Down Expand Up @@ -1373,7 +1406,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
// Only modules being require()'d really need to avoid TLA.
if (loadAsESM) {
// Pass the source into the .mjs extension handler indirectly through the cache.
cjsSourceCache.set(this, { source: content });
requiredESMSourceCache.set(this, content);
loadESMFromCJS(this, filename);
return;
}
Expand Down Expand Up @@ -1414,13 +1447,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
const module = this;
if (requireDepth === 0) { statCache = new SafeMap(); }
setHasStartedUserCJSExecution();
this[kIsExecuting] = true;
if (inspectorWrapper) {
result = inspectorWrapper(compiledWrapper, thisValue, exports,
require, module, filename, dirname);
} else {
result = ReflectApply(compiledWrapper, thisValue,
[exports, require, module, filename, dirname]);
}
this[kIsExecuting] = false;
if (requireDepth === 0) { statCache = null; }
return result;
};
Expand All @@ -1432,15 +1467,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
* @returns {string}
*/
function getMaybeCachedSource(mod, filename) {
const cached = cjsSourceCache.get(mod);
const cached = importedCJSCache.get(mod);
let content;
if (cached?.source) {
content = cached.source;
cached.source = undefined;
} else {
// TODO(joyeecheung): we can read a buffer instead to speed up
// compilation.
content = fs.readFileSync(filename, 'utf8');
content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8');
}
return content;
}
Expand Down
10 changes: 10 additions & 0 deletions lib/internal/modules/esm/load.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ async function defaultLoad(url, context = kEmptyObject) {

validateAttributes(url, format, importAttributes);

// Use the synchronous commonjs translator which can deal with cycles.
if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
format = 'commonjs-sync';
}

return {
__proto__: null,
format,
Expand Down Expand Up @@ -201,6 +206,11 @@ function defaultLoadSync(url, context = kEmptyObject) {

validateAttributes(url, format, importAttributes);

// Use the synchronous commonjs translator which can deal with cycles.
if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
format = 'commonjs-sync';
}

return {
__proto__: null,
format,
Expand Down
77 changes: 65 additions & 12 deletions lib/internal/modules/esm/loader.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
'use strict';

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

const {
ArrayPrototypeJoin,
Expand All @@ -15,8 +18,11 @@ const {
hardenRegExp,
} = primordials;

const { imported_cjs_symbol } = internalBinding('symbols');

const assert = require('internal/assert');
const {
ERR_REQUIRE_CYCLE_MODULE,
ERR_REQUIRE_ESM,
ERR_NETWORK_IMPORT_DISALLOWED,
ERR_UNKNOWN_MODULE_FORMAT,
Expand All @@ -30,7 +36,10 @@ const {
} = require('internal/modules/esm/utils');
const { kImplicitAssertType } = require('internal/modules/esm/assert');
const { canParse } = internalBinding('url');
const { ModuleWrap } = internalBinding('module_wrap');
const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
const {
urlToFilename,
} = require('internal/modules/helpers');
let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;

/**
Expand Down Expand Up @@ -248,17 +257,36 @@ class ModuleLoader {
/**
* This constructs (creates, instantiates and evaluates) a module graph that
* is require()'d.
* @param {import('../cjs/loader.js').Module} mod CJS module wrapper of the ESM.
* @param {string} filename Resolved filename of the module being require()'d
* @param {string} source Source code. TODO(joyeecheung): pass the raw buffer.
* @param {string} isMain Whether this module is a main module.
* @returns {ModuleNamespaceObject}
* @param {import('../cjs/loader.js').Module|undefined} parent Parent module, if any.
* @returns {{ModuleWrap}}
*/
importSyncForRequire(filename, source, isMain) {
importSyncForRequire(mod, filename, source, isMain, parent) {
const url = pathToFileURL(filename).href;
let job = this.loadCache.get(url, kImplicitAssertType);
// This module is already loaded, check whether it's synchronous and return the
// namespace.
// This module job is already created:
// 1. If it was loaded by `require()` before, at this point the instantiation
// is already completed and we can check the whether it is in a cycle
// (in that case the module status is kEvaluaing), and whether the
// required graph is synchronous.
// 2. If it was loaded by `import` before, only allow it if it's already evaluated
// to forbid cycles.
// TODO(joyeecheung): ensure that imported synchronous graphs are evaluated
// synchronously so that any previously imported synchronous graph is already
// evaluated at this point.
if (job !== undefined) {
mod[kRequiredModuleSymbol] = job.module;
if (job.module.getStatus() !== kEvaluated) {
const parentFilename = urlToFilename(parent?.filename);
let message = `Cannot require() ES Module ${filename} in a cycle.`;
if (parentFilename) {
message += ` (from ${parentFilename})`;
}
throw new ERR_REQUIRE_CYCLE_MODULE(message);
}
return job.module.getNamespaceSync();
}
// TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
Expand All @@ -270,6 +298,7 @@ class ModuleLoader {
const { ModuleJobSync } = require('internal/modules/esm/module_job');
job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk);
this.loadCache.set(url, kImplicitAssertType, job);
mod[kRequiredModuleSymbol] = job.module;
return job.runSync().namespace;
}

Expand Down Expand Up @@ -304,19 +333,29 @@ class ModuleLoader {
const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes;
let job = this.loadCache.get(url, resolvedImportAttributes.type);
if (job !== undefined) {
// This module is previously imported before. We will return the module now and check
// asynchronicity of the entire graph later, after the graph is instantiated.
// This module is being evaluated, which means it's imported in a previous link
// in a cycle.
if (job.module.getStatus() === kEvaluating) {
const parentFilename = urlToFilename(parentURL);
let message = `Cannot import Module ${specifier} in a cycle.`;
if (parentFilename) {
message += ` (from ${parentFilename})`;
}
throw new ERR_REQUIRE_CYCLE_MODULE(message);
}
// Othersie the module could be imported before but the evaluation may be already
// completed (e.g. the require call is lazy) so it's okay. We will return the
// module now and check asynchronicity of the entire graph later, after the
// graph is instantiated.
return job.module;
}

defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
const loadResult = defaultLoadSync(url, { format, importAttributes });
const { responseURL, source } = loadResult;
let { format: finalFormat } = loadResult;
const { format: finalFormat } = loadResult;
this.validateLoadResult(url, finalFormat);
if (finalFormat === 'commonjs') {
finalFormat = 'commonjs-sync';
} else if (finalFormat === 'wasm') {
if (finalFormat === 'wasm') {
assert.fail('WASM is currently unsupported by require(esm)');
}

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

const cjsModule = wrap[imported_cjs_symbol];
if (cjsModule) {
assert(finalFormat === 'commonjs-sync');
// Check if the ESM initiating import CJS is being required by the same CJS module.
if (cjsModule && cjsModule[kIsExecuting]) {
const parentFilename = urlToFilename(parentURL);
let message = `Cannot import CommonJS Module ${specifier} in a cycle.`;
if (parentFilename) {
message += ` (from ${parentFilename})`;
}
throw new ERR_REQUIRE_CYCLE_MODULE(message);
}
}

const inspectBrk = (isMain && getOptionValue('--inspect-brk'));
const { ModuleJobSync } = require('internal/modules/esm/module_job');
job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk);
Expand Down
Loading

0 comments on commit db17461

Please sign in to comment.