Skip to content

vm: fix crash when setting __proto__ on context's globalThis #47939

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 3 commits into from
May 15, 2023
Merged

vm: fix crash when setting __proto__ on context's globalThis #47939

merged 3 commits into from
May 15, 2023

Conversation

F3n67u
Copy link
Member

@F3n67u F3n67u commented May 9, 2023

Add IsUndefined() check after GetOwnPropertyDescriptor to avoid crash when desc_obj->HasOwnProperty()

fix #47798

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels May 9, 2023
dubzzz
dubzzz reviewed May 9, 2023
View reviewed changes
Copy link
Contributor

@dubzzz dubzzz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

From what I understood from the reported bug, the proposed fix seems to be perfect. Just put one comment but I don't know if there are any coding style preferences on node regarding it.

@targos targos added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@F3n67u @VoltrexKeyva
Update test/parallel/test-vm-set-proto-null-on-globalthis.js
4bfdfc5 
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request May 11, 2023
Open
24 tasks
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 12, 2023
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

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

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

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label May 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6bbf2a5 into nodejs:main May 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6bbf2a5

targos pushed a commit that referenced this pull request May 15, 2023
@F3n67u @targos
vm: fix crash when setting __proto__ on context's globalThis
2bd869d 
PR-URL: #47939
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request May 15, 2023
Merged
This was referenced May 16, 2023
Open
Open
Open
Open
Open
Open
Open
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@F3n67u @danielleadams
vm: fix crash when setting __proto__ on context's globalThis
5d2dec1 
PR-URL: #47939
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@F3n67u @MoLow
vm: fix crash when setting __proto__ on context's globalThis
0d0fad8 
PR-URL: nodejs#47939
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dubzzz dubzzz dubzzz left review comments

@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@himself65 himself65 himself65 approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

Setting __proto__ on vm context's globalThis causes a crash (regression in v19.9.0)
9 participants
@F3n67u @nodejs-github-bot @jasnell @addaleax @dubzzz @himself65 @RaisinTen @VoltrexKeyva @targos