Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
MikeRalphson wants to merge 2 commits into from
Closed

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

MikeRalphson wants to merge 2 commits into from

Conversation

@MikeRalphson
Copy link
Contributor

@MikeRalphson MikeRalphson commented Apr 16, 2023 •
edited
Loading 

(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:

const vm = require('node:vm');

async function main() {
  const myObj = { myProp: '' };
  const context = vm.createContext(myObj);
  const module = new vm.SourceTextModule('import * as bug from "bug";', { identifier: 'bug', context });
  await module.link(function(a,b,c){return});
}

main();

@nodejs-github-bot nodejs-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
@MikeRalphson
vm: harden check for non-module in vm.modules linker
1586f93 
Signed-off-by: Mike Ralphson <mike.ralphson@gmail.com>
@RaisinTen
Copy link
Member

RaisinTen commented Apr 17, 2023

Should we add a test?

Also, cc @nodejs/vm.

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 17, 2023

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

@RaisinTen
Copy link
Member

RaisinTen commented Apr 17, 2023

I think so, yes.

aduh95
aduh95 reviewed Apr 17, 2023
View reviewed changes
lib/internal/vm/module.js Outdated Show resolved Hide resolved
@MikeRalphson @aduh95
vm: use more idiomatic test for undefined
ac3d903 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 18, 2023

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

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 19, 2023

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

@RaisinTen
Copy link
Member

RaisinTen commented Apr 19, 2023

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

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Apr 19, 2023

Thanks @RaisinTen!

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2023

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
Member

fhinkel commented Feb 3, 2024

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

@legendecas legendecas mentioned this pull request Mar 20, 2024
Merged
@legendecas
Copy link
Member

legendecas commented Mar 22, 2024

Landed in d1d5da2.

@legendecas legendecas closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MikeRalphson @RaisinTen @aduh95 @fhinkel @legendecas @jasnell @nodejs-github-bot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.