GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: harden check for non-module in vm.modules linker by MikeRalphson · Pull Request #47583 · nodejs/node

Conversation

@MikeRalphson

@MikeRalphson MikeRalphson commented Apr 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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:

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();

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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 <[email protected]>
@MikeRalphson
MikeRalphson force-pushed the patch-3 branch from bccbb28 to 1586f93 Compare April 16, 2023 16:29
@RaisinTen

RaisinTen commented Apr 17, 2023

Copy link
Copy Markdown
Member

Should we add a test?

Also, cc @nodejs/vm.

aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MikeRalphson

MikeRalphson commented Apr 17, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RaisinTen

RaisinTen commented Apr 17, 2023

Copy link
Copy Markdown
Member

I think so, yes.

MikeRalphson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 reviewed Apr 17, 2023
View reviewed changes
Comment thread lib/internal/vm/module.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@MikeRalphson @aduh95
vm: use more idiomatic test for undefined
ac3d903
Co-authored-by: Antoine du Hamel <[email protected]>
@MikeRalphson

MikeRalphson commented Apr 18, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MikeRalphson

MikeRalphson commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RaisinTen

RaisinTen commented Apr 19, 2023

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 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MikeRalphson

MikeRalphson commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @RaisinTen!

RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Apr 19, 2023

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.

MikeRalphson and RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Apr 21, 2023
View reviewed changes
@fhinkel

fhinkel commented Feb 3, 2024

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas legendecas mentioned this pull request Mar 20, 2024
vm: harden module type checks #52162
Merged
@legendecas

legendecas commented Mar 22, 2024

Copy link
Copy Markdown
Member

Landed in d1d5da2.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@MikeRalphson @RaisinTen @aduh95 @fhinkel @legendecas @jasnell @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.