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

[v12.x] src: move CHECK in AddIsolateFinishedCallback #38010

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 31, 2021

CHECK(it->second) asserts that we have PerIsolatePlatformData
in the per_isolate_ map, and not just a key with empty value. When
it == per_isolate_.end(), however, it means that we don't have the
isolate and the CHECK(it->second) is guaranteed to fail then!

@indutny indutny requested a review from addaleax March 31, 2021 20:33
@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. v12.x labels Mar 31, 2021
@indutny indutny force-pushed the fix/electron-worker-threads-crash branch from b964433 to d248ed3 Compare March 31, 2021 21:06
src/env.cc Outdated Show resolved Hide resolved
`CHECK(it->second)` asserts that we have `PerIsolatePlatformData`
in the `per_isolate_` map, and not just a key with empty value. When
`it == per_isolate_.end()`, however, it means that we don't have the
isolate and the `CHECK(it->second)` is guaranteed to fail then!
@indutny indutny force-pushed the fix/electron-worker-threads-crash branch from d248ed3 to ad7f207 Compare March 31, 2021 21:57
@indutny indutny changed the title env: fix use-after-free in keep-alive allocators src: move CHECK in AddIsolateFinishedCallback Mar 31, 2021
@indutny
Copy link
Member Author

indutny commented Mar 31, 2021

Thank you @addaleax !

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2021
@jasnell jasnell changed the title src: move CHECK in AddIsolateFinishedCallback [v12.x] src: move CHECK in AddIsolateFinishedCallback Apr 30, 2021
@jasnell
Copy link
Member

jasnell commented Apr 30, 2021

This is ready to go for the next v12.x release but can't land in the staging branch yet.

richardlau pushed a commit that referenced this pull request Jul 23, 2021
`CHECK(it->second)` asserts that we have `PerIsolatePlatformData`
in the `per_isolate_` map, and not just a key with empty value. When
`it == per_isolate_.end()`, however, it means that we don't have the
isolate and the `CHECK(it->second)` is guaranteed to fail then!

PR-URL: #38010
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau
Copy link
Member

Landed in 8a9f949.

@richardlau richardlau closed this Jul 23, 2021
@richardlau richardlau mentioned this pull request Jul 23, 2021
guybedford pushed a commit that referenced this pull request Jul 26, 2021
`CHECK(it->second)` asserts that we have `PerIsolatePlatformData`
in the `per_isolate_` map, and not just a key with empty value. When
`it == per_isolate_.end()`, however, it means that we don't have the
isolate and the `CHECK(it->second)` is guaranteed to fail then!

PR-URL: #38010
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 26, 2021
`CHECK(it->second)` asserts that we have `PerIsolatePlatformData`
in the `per_isolate_` map, and not just a key with empty value. When
`it == per_isolate_.end()`, however, it means that we don't have the
isolate and the `CHECK(it->second)` is guaranteed to fail then!

PR-URL: #38010
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants