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.

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

Fix minor undefined behavior in modules-new-test #3708

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fhanau
Copy link
Contributor

@fhanau fhanau commented Mar 12, 2025

When running under high load, a race condition in modules-new-test.c++ could lead to the V8 isolate being destroyed too late, leading to a member access within null pointer during isolate deallocation.

As suggested by @danlapid

When running under high load, a race condition in modules-new-test.c++ could
lead to the V8 isolate being destroyed too late, leading to a member access
within null pointer during isolate deallocation.
@fhanau fhanau requested a review from danlapid March 12, 2025 18:07
@fhanau fhanau requested review from a team as code owners March 12, 2025 18:07
@fhanau fhanau requested a review from ObsidianMinor March 12, 2025 18:07
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Heh, I just spotted this yesterday. To avoid some merge conflicts, if you don't mind, let me incorporate this change into the other PR that I've been in the process of preparing that I'll likely open tomorrow. I'm good with this landing on its own if it must but I can just as easily just pull it in with the other PR also.

@fhanau
Copy link
Contributor Author

fhanau commented Mar 12, 2025

Heh, I just spotted this yesterday. To avoid some merge conflicts, if you don't mind, let me incorporate this change into the other PR that I've been in the process of preparing that I'll likely open tomorrow. I'm good with this landing on its own if it must but I can just as easily just pull it in with the other PR also.

The change itself is not that important – not sure to what extent this is causing downstream tests to be flaky though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants