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

vm: fix crash on fatal error in debug context #1229

Merged
merged 1 commit into from
Mar 22, 2015

Conversation

bnoordhuis
Copy link
Member

Ensure that the debug context has an Environment assigned in case
a fatal error is raised.

The fatal exception handler in node.cc is not equipped to deal with
contexts that don't have one and can't easily be taught that due to
a deficiency in the V8 API: there is no way for the embedder to tell
if the data index is in use.

Fixes: #1190

R=@indutny?

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/356/

@Fishrock123 Fishrock123 added the vm Issues and PRs related to the vm subsystem. label Mar 21, 2015
// can't easily be taught that due to a deficiency in the V8 API:
// there is no way for the embedder to tell if the data index is
// in use.
struct ScopedEnvironment {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just assign it? Is there any point in rolling it back?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons:

  1. The environment doesn't really belong on the debug context. This is a hack to stop it from crashing until we have better ways to deal with this.
  2. It introduces a potential use-after-free because the pointer stays around when the non-debug context is disposed.

@indutny
Copy link
Member

indutny commented Mar 21, 2015

LGTM, with a small comment.

@indutny
Copy link
Member

indutny commented Mar 22, 2015

LGTM, please land it ;)

Ensure that the debug context has an Environment assigned in case
a fatal error is raised.

The fatal exception handler in node.cc is not equipped to deal with
contexts that don't have one and can't easily be taught that due to
a deficiency in the V8 API: there is no way for the embedder to tell
if the data index is in use.

Fixes: nodejs#1190
PR-URL: nodejs#1229
Reviewed-By: Fedor Indutny <[email protected]>
@bnoordhuis bnoordhuis closed this Mar 22, 2015
@bnoordhuis bnoordhuis deleted the fix-issue-1190 branch March 22, 2015 19:10
@bnoordhuis bnoordhuis merged commit cf081a4 into nodejs:v1.x Mar 22, 2015
@rvagg rvagg mentioned this pull request Mar 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants