module: disallow CJS <-> ESM edges in a cycle from require(esm) #52264

joyeecheung · 2024-03-29T18:26:12Z

This patch disallows CJS <-> ESM cycle 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.

Fixes: #52145

nodejs-github-bot · 2024-03-29T18:26:17Z

Review requested:

@nodejs/loaders
@nodejs/vm

nodejs-github-bot · 2024-03-29T18:34:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57998/

doc/api/errors.md

nodejs-github-bot · 2024-03-31T10:28:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58014/

guybedford · 2024-04-03T00:13:56Z

Just to confirm - can this deal with the case where the require(esm) module is to a parent of a cycle that hasn't been executed yet? That is, the invariant should specifically be that none of the require(esm) module, nor any of its transitive dependencies are currently part of an ESM execution operation.

Consider the graph:

A (esm) -> B (esm)
B (esm) -> C (cjs)
C (cjs) -> Z (esm)
Z (esm) -> A (esm)

Where we import A first, and when the require(esm) gets Z it will be linked but not previously evaluated.

That is, my intuition here would be that a full upfront graph analysis must be done at require(esm) time to ensure acyclic behaviour.

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.

joyeecheung · 2024-04-06T00:35:53Z

Where we import A first, and when the require(esm) gets Z it will be linked but not previously evaluated.

Yes this can be detected. Added a test.

nodejs-github-bot · 2024-04-06T00:41:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58148/

guybedford

Thanks for verifying the test case.

nodejs-github-bot · 2024-04-07T16:50:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58179/

aduh95 · 2024-04-07T17:13:00Z

lib/internal/modules/helpers.js

+function urlToFilename(url) {
+  if (url && StringPrototypeStartsWith(url, 'file://')) {
+    return fileURLToPath(url);
+  }
+  return url;
+}


We could use that util in more places:

node/lib/internal/modules/esm/translators.js

Line 244 in d6b57f6

return StringPrototypeStartsWith(resolvedURL, 'file://') ? fileURLToPath(resolvedURL) : resolvedURL;

node/lib/internal/modules/esm/translators.js

Line 266 in d6b57f6

const filename = StringPrototypeStartsWith(url, 'file://') ? fileURLToPath(url) : url;

Can happen in a follow-up PR.

nodejs-github-bot · 2024-04-08T14:45:56Z

Landed in db17461

JakobJingleheimer

Thanks for this, and especially thanks for the code docs!

Sorry, just realised this review was pending / never got submitted 😞

JakobJingleheimer · 2024-04-03T13:15:40Z

doc/api/errors.md

+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


Suggested change

at the top-level of either a ES Module (via `createRequire()`) or a CommonJS

at the top-level of either an ES Module (via `createRequire()`) or a CommonJS

JakobJingleheimer · 2024-04-09T17:09:35Z

lib/internal/modules/cjs/loader.js

-      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


Suggested change

// come from imported CJS. In that case use the importedCJSCache

// came from imported CJS. In that case use the importedCJSCache

JakobJingleheimer · 2024-04-09T17:14:40Z

lib/internal/modules/esm/loader.js

+   * @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.


nit: we could DRY this up with a @typedef

JakobJingleheimer · 2024-04-09T17:15:48Z

lib/internal/modules/esm/loader.js

+    // 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.


w00t! thanks for this!

JakobJingleheimer · 2024-04-09T17:17:50Z

lib/internal/modules/esm/loader.js

+        }
+        throw new ERR_REQUIRE_CYCLE_MODULE(message);
+      }
+      // Othersie the module could be imported before but the evaluation may be already


Suggested change

// Othersie the module could be imported before but the evaluation may be already

// Otherwise the module could be imported before but the evaluation may be already

JakobJingleheimer · 2024-04-09T17:19:53Z

lib/internal/modules/esm/loader.js

    const { responseURL, source } = loadResult;
-    let { format: finalFormat } = loadResult;
+    const { format: finalFormat } = loadResult;


nit: collapsable

Suggested change

const { responseURL, source } = loadResult;

let { format: finalFormat } = loadResult;

const { format: finalFormat } = loadResult;

const {

format: finalFormat,

responseURL,

source,

} = loadResult;

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>

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024

GeoffreyBooth approved these changes Mar 29, 2024

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

doc/api/errors.md Outdated Show resolved Hide resolved

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2024

joyeecheung mentioned this pull request Mar 31, 2024

Disallow ESM-CJS-ESM-ESM cycles when using require(esm) #52145

Closed

joyeecheung force-pushed the require-esm-cycle branch from f22f103 to f414613 Compare April 6, 2024 00:35

fixup! module: disallow CJS <-> ESM edges in a cycle from require(esm)

971affd

joyeecheung force-pushed the require-esm-cycle branch from f414613 to 971affd Compare April 6, 2024 00:35

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024

guybedford approved these changes Apr 7, 2024

View reviewed changes

aduh95 approved these changes Apr 7, 2024

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot merged commit db17461 into nodejs:main Apr 8, 2024
59 checks passed

JakobJingleheimer reviewed Apr 9, 2024

View reviewed changes

JakobJingleheimer mentioned this pull request Apr 9, 2024

module: tidy code and comments #52437

Merged

RafaelGSS mentioned this pull request Apr 15, 2024

2024-04-24, Version 22.0.0 (Current) #52505

Merged

marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: disallow CJS <-> ESM edges in a cycle from require(esm) #52264

module: disallow CJS <-> ESM edges in a cycle from require(esm) #52264

joyeecheung commented Mar 29, 2024 •

edited

Loading

nodejs-github-bot commented Mar 29, 2024

nodejs-github-bot commented Mar 29, 2024

nodejs-github-bot commented Mar 31, 2024

guybedford commented Apr 3, 2024

joyeecheung commented Apr 6, 2024

nodejs-github-bot commented Apr 6, 2024

guybedford left a comment

nodejs-github-bot commented Apr 7, 2024

aduh95 Apr 7, 2024 •

edited

Loading

nodejs-github-bot commented Apr 8, 2024

JakobJingleheimer left a comment

JakobJingleheimer Apr 3, 2024

JakobJingleheimer Apr 9, 2024

JakobJingleheimer Apr 9, 2024

JakobJingleheimer Apr 9, 2024

JakobJingleheimer Apr 9, 2024

JakobJingleheimer Apr 9, 2024

	at the top-level of either a ES Module (via `createRequire()`) or a CommonJS
	at the top-level of either an ES Module (via `createRequire()`) or a CommonJS

	// come from imported CJS. In that case use the importedCJSCache
	// came from imported CJS. In that case use the importedCJSCache

	// Othersie the module could be imported before but the evaluation may be already
	// Otherwise the module could be imported before but the evaluation may be already

module: disallow CJS <-> ESM edges in a cycle from require(esm) #52264

module: disallow CJS <-> ESM edges in a cycle from require(esm) #52264

Conversation

joyeecheung commented Mar 29, 2024 • edited Loading

nodejs-github-bot commented Mar 29, 2024

nodejs-github-bot commented Mar 29, 2024

nodejs-github-bot commented Mar 31, 2024

guybedford commented Apr 3, 2024

joyeecheung commented Apr 6, 2024

nodejs-github-bot commented Apr 6, 2024

guybedford left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 7, 2024

aduh95 Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 8, 2024

JakobJingleheimer left a comment

Choose a reason for hiding this comment

JakobJingleheimer Apr 3, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 9, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 9, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 9, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 9, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 9, 2024

Choose a reason for hiding this comment

joyeecheung commented Mar 29, 2024 •

edited

Loading

aduh95 Apr 7, 2024 •

edited

Loading