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

src: fix crash when lazy getter is invoked in a vm context #57168

Merged
merged 1 commit into from
Feb 23, 2025
Merged

src: fix crash when lazy getter is invoked in a vm context #57168

merged 1 commit into from
Feb 23, 2025
+48 −3

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Feb 21, 2025 •
edited
Loading

V8 should invoke native functions in their creation context,
preventing dynamic context by the caller. However, the lazy getter has
no JavaScript function representation and has no creation context. It
is not invoked in the original creation context. Fix the null realm by
retrieving the creation context via this argument.

Fixes #57166

@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. util Issues and PRs related to the built-in util module. labels Feb 21, 2025
@legendecas legendecas force-pushed the realm/creation-in-lazy-getter branch from d2261c9 to bc86e5e Compare February 21, 2025 15:24
@legendecas
src: fix crash when lazy getter is invoked in a vm context
cf2e520 
V8 should invoke native functions in their creation context,
preventing dynamic context by the caller. However, the lazy getter has
no JavaScript function representation and has no creation context. It
is not invoked in the original creation context. Fix the null realm by
retrieving the creation context via `this` argument.
@legendecas legendecas force-pushed the realm/creation-in-lazy-getter branch from bc86e5e to cf2e520 Compare February 21, 2025 15:29
@targos
Copy link
Member

targos commented Feb 21, 2025

@nodejs/cpp-reviewers

@codecov Codecov
Copy link

codecov bot commented Feb 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (8fc919d) to head (cf2e520).
Report is 106 commits behind head on main.

Files with missing lines Patch % Lines
src/node_util.cc 45.45% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57168      +/-   ##
==========================================
- Coverage   90.35%   90.35%   -0.01%     
==========================================
  Files         629      629              
  Lines      184308   184373      +65     
  Branches    36015    36019       +4     
==========================================
+ Hits       166532   166584      +52     
- Misses      10909    10916       +7     
- Partials     6867     6873       +6
Files with missing lines Coverage Δ
src/node_errors.h 85.29% <ø> (ø)
src/node_util.cc 82.18% <45.45%> (-1.47%) ⬇️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Feb 23, 2025 •
edited
Loading

citgm check: https://ci.nodejs.org/job/citgm-smoker/nodes=ubuntu2204-64/3560/
https://ci.nodejs.org/job/citgm-smoker/nodes=ubuntu2204-64/3560/testReport/(root)/citgm/commander_v13_1_0/ ✅

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2025
@nodejs-github-bot nodejs-github-bot merged commit 4e1f0cc into nodejs:main Feb 23, 2025
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4e1f0cc

acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@legendecas @acidiney
src: fix crash when lazy getter is invoked in a vm context
1930ad1 
V8 should invoke native functions in their creation context,
preventing dynamic context by the caller. However, the lazy getter has
no JavaScript function representation and has no creation context. It
is not invoked in the original creation context. Fix the null realm by
retrieving the creation context via `this` argument.

PR-URL: nodejs#57168
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@legendecas legendecas deleted the realm/creation-in-lazy-getter branch February 23, 2025 22:23
targos pushed a commit that referenced this pull request Feb 24, 2025
@legendecas @targos
src: fix crash when lazy getter is invoked in a vm context
4564456 
V8 should invoke native functions in their creation context,
preventing dynamic context by the caller. However, the lazy getter has
no JavaScript function representation and has no creation context. It
is not invoked in the original creation context. Fix the null realm by
retrieving the creation context via `this` argument.

PR-URL: #57168
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@github-actions github-actions bot mentioned this pull request Feb 25, 2025
Merged
targos pushed a commit that referenced this pull request Feb 25, 2025
@legendecas @targos
src: fix crash when lazy getter is invoked in a vm context
ef314dc 
V8 should invoke native functions in their creation context,
preventing dynamic context by the caller. However, the lazy getter has
no JavaScript function representation and has no creation context. It
is not invoked in the original creation context. Fix the null realm by
retrieving the creation context via `this` argument.

PR-URL: #57168
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@anonrig anonrig anonrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segmentation fault in Jest
7 participants
@legendecas @targos @nodejs-github-bot @jasnell @addaleax @anonrig @aduh95