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

FatalException handler SIGABRT from throw with .stack getter error #25718

Closed
Fishrock123 opened this issue Jan 26, 2019 · 1 comment
Closed
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.

Comments

@Fishrock123
Copy link
Contributor

node -e "throw { get stack() { throw new Error('weird throw but ok') } }"

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
...

As discovered in #25715 but not strictly related to.

This is the line in question from the abort:

env->stack_string()).ToLocalChecked();

Basically, node::FatalException doesn't handle deep errors from .stack getters.

This is a pretty messy problem to fully deal with. It is possible to have a situation of infinitely recursing .stack getter errors, theoretically at least.

That being said... I doubt that any significant number of people will ever actually run into the more problematic cases, or maybe even the simple case.

@Fishrock123 Fishrock123 added confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem. labels Jan 26, 2019
@devsnek
Copy link
Member

devsnek commented Jan 26, 2019

I think it would be reasonable to recurse until the c++ stack blows.

addaleax added a commit to addaleax/node that referenced this issue Jan 30, 2019
Handle situations where accessing `.name` or `.stack` on an object
fails.

Fixes: nodejs#25718
addaleax added a commit that referenced this issue Feb 3, 2019
Handle situations where accessing `.name` or `.stack` on an object
fails.

Fixes: #25718

PR-URL: #25834
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants