Skip to content
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

[v20.x] vm: harden module type checks #53109

Closed
wants to merge 1 commit into from
Closed

[v20.x] vm: harden module type checks #53109

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

Check if the value returned from user linker function is a null-ish value.

validateInternalField should be preferred when checking this argument to guard against null-ish this.

Co-authored-by: Mike Ralphson [email protected]
PR-URL: #52162
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]
Reviewed-By: Yagiz Nizipli [email protected]

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. vm Issues and PRs related to the vm subsystem. labels May 22, 2024
@legendecas legendecas mentioned this pull request May 22, 2024
Merged
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@marco-ippolito marco-ippolito force-pushed the v20.x-staging branch 2 times, most recently from d702971 to 044bcce Compare June 17, 2024 14:26
joyeecheung
joyeecheung reviewed Jun 17, 2024
View reviewed changes
lib/internal/vm/module.js Outdated
this[kDependencySpecifiers] = this[kWrap].getStaticDependencySpecifiers();
}
validateInternalField(this, kDependencySpecifiers, 'SourceTextModule');
this[kDependencySpecifiers] ??= ObjectFreeze(this[kWrap].getStaticDependencySpecifiers());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the freezing (from #49720) semver-major?

marco-ippolito
marco-ippolito requested changes Jun 17, 2024
View reviewed changes
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on #53109 (comment)

@legendecas @MikeRalphson
vm: harden module type checks
5081f84 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <[email protected]>
PR-URL: nodejs#52162
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@legendecas legendecas force-pushed the backport-52162-to-20 branch from f9c85f3 to 5081f84 Compare June 18, 2024 09:10
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Sep 21, 2024

@marco-ippolito I believe the comment has been addressed.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

targos pushed a commit that referenced this pull request Sep 26, 2024
@legendecas @MikeRalphson @targos
vm: harden module type checks
f88bf05 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <[email protected]>
PR-URL: #52162
Backport-PR-URL: #53109
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@targos
Copy link
Member

targos commented Sep 26, 2024

Landed in f88bf05

@targos targos closed this Sep 26, 2024
@legendecas legendecas deleted the backport-52162-to-20 branch September 26, 2024 08:46
targos pushed a commit that referenced this pull request Oct 2, 2024
@legendecas @MikeRalphson @targos
vm: harden module type checks
47be236 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <[email protected]>
PR-URL: #52162
Backport-PR-URL: #53109
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Oct 2, 2024
@legendecas @MikeRalphson @targos
vm: harden module type checks
9e1c229 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <[email protected]>
PR-URL: #52162
Backport-PR-URL: #53109
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@anonrig anonrig anonrig approved these changes

@targos targos targos approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. 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.
6 participants
@legendecas @nodejs-github-bot @targos @anonrig @joyeecheung @marco-ippolito