Skip to content

fix: harden check for non-module in vm.modules linker - #47583

Closed
MikeRalphson wants to merge 2 commits into
nodejs:mainfrom
MikeRalphson:patch-3
Closed

fix: harden check for non-module in vm.modules linker#47583
MikeRalphson wants to merge 2 commits into
nodejs:mainfrom
MikeRalphson:patch-3

Conversation

@MikeRalphson

@MikeRalphsonMikeRalphson commented Apr 16, 2023

Copy link
Copy Markdown
Contributor
(node:528088) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/vm/module:306
if (module[kWrap] === undefined) {
^
TypeError: Cannot read properties of undefined (reading 'Symbol(kWrap)')
at ModuleWrap.<anonymous> (node:internal/vm/module:306:19)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Node.js v19.9.0

Repro case:

constvm=require('node:vm');asyncfunctionmain(){constmyObj={myProp: ''};constcontext=vm.createContext(myObj);constmodule=newvm.SourceTextModule('import * as bug from "bug";',{identifier: 'bug', context });awaitmodule.link(function(a,b,c){return});}main();

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Apr 16, 2023
Signed-off-by: Mike Ralphson <mike.ralphson@gmail.com>
@RaisinTen

Copy link
Copy Markdown
Member

Should we add a test?

Also, cc @nodejs/vm.

@MikeRalphson

Copy link
Copy Markdown
ContributorAuthor

@RaisinTen should that be a simple test of the exception type?

@RaisinTen

Copy link
Copy Markdown
Member

I think so, yes.

Comment threadlib/internal/vm/module.js Outdated
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MikeRalphson

Copy link
Copy Markdown
ContributorAuthor

Have found the relevant test file and am working up the new test function.

@MikeRalphson

Copy link
Copy Markdown
ContributorAuthor

Is there a way to run just one of, or a subset of, the tests?

@RaisinTen

Copy link
Copy Markdown
Member

Yea, you can run ./node test/parallel/test-rest-of-the-file-name.js to run an individual test

@MikeRalphson

Copy link
Copy Markdown
ContributorAuthor

Thanks @RaisinTen!

@aduh95

Copy link
Copy Markdown
Contributor

You can also use tools/test.py test/parallel/test-rest-of-the-file-name.js or e.g. tools/test.py test/parallel/test-repl-* if you wanted to run all the REPL tests.

@fhinkel

Copy link
Copy Markdown
Member

@MikeRalphson are you still working on the regression test or can somebody else pick this up?

@legendecas

Copy link
Copy Markdown
Member

Landed in d1d5da2.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@MikeRalphson@RaisinTen@aduh95@fhinkel@legendecas@jasnell@nodejs-github-bot