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

src: handle fatal error when Environment is not assigned to context #27236

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@joyeecheung
Copy link
Member

commented Apr 15, 2019

Previously when an uncaught JS error is thrown before Environment was
assigned to the context (e.g. a SyntaxError in a per-context script),
it triggered an infinite recursion:

  1. The error message listener node::OnMessage() triggered
    node::FatalException()
  2. node::FatalException() attempted to get the Environment
    assigned to the context entered using Environment::GetCurrent()
  3. Environment::GetCurrent() previously incorrectly accepted
    out-of-bound access with the length of the embedder data array
    as index, and called context->GetAlignedPointerFromEmbedderData()
  4. The out-of-bound access in GetAlignedPointerFromEmbedderData()
    triggered a fatal error, which was handled by node::FatalError()
  5. node::FatalError() called Environment::GetCurrent(), then
    we went back to 3.

This patch fixes the incorrect guard in 3. When Environment::GetCurrent()
returns nullptr (when Environment is not yet assigned to the context) in 2,
it now prints the JS stack trace and crashes directly.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
src: handle fatal error when Environment is not assigned to context
Previously when a uncaught JS error is thrown before Environment was
assigned to the context (e.g. a SyntaxError in a per-context script),
it triggered an infinite recursion:

1. The error message listener `node::OnMessage()` triggered
   `node::FatalException()`
2. `node::FatalException()` attempted to get the Environment
   assigned to the context entered using `Environment::GetCurrent()`
3. `Environment::GetCurrent()` previously incorrectly accepted
   out-of-bound access with the length of the embedder data array
   as index, and called `context->GetAlignedPointerFromEmbedderData()`
4. The out-of-bound access in `GetAlignedPointerFromEmbedderData()`
   triggered a fatal error, which was handled by `node::FatalError()`
5. `node::FatalError()` calls `node::FatalException()`, then
   we enter the infinite recursion.

This patch fixes the incorrect guard in 3, and handles error with
best-effort when `Environment::GetCurrent()` returns nullptr
(when Environment is not yet assigned to the context) in 2.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Show resolved Hide resolved src/node_errors.cc Outdated
@bnoordhuis
Copy link
Member

left a comment

LGTM with some comments/questions.

Show resolved Hide resolved src/env-inl.h Outdated
Show resolved Hide resolved src/node_errors.cc Outdated
Show resolved Hide resolved src/node_errors.cc Outdated
@nodejs-github-bot

This comment has been minimized.

@addaleax
Copy link
Member

left a comment

Thank you!

Show resolved Hide resolved src/node_errors.cc
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Landed in cdba9f2 with the comment nit fixed.

joyeecheung added a commit that referenced this pull request Apr 17, 2019

src: handle fatal error when Environment is not assigned to context
Previously when an uncaught JS error is thrown before Environment was
assigned to the context (e.g. a SyntaxError in a per-context script),
it triggered an infinite recursion:

1. The error message listener `node::OnMessage()` triggered
   `node::FatalException()`
2. `node::FatalException()` attempted to get the Environment
   assigned to the context entered using `Environment::GetCurrent()`
3. `Environment::GetCurrent()` previously incorrectly accepted
   out-of-bound access with the length of the embedder data array
   as index, and called `context->GetAlignedPointerFromEmbedderData()`
4. The out-of-bound access in `GetAlignedPointerFromEmbedderData()`
   triggered a fatal error, which was handled by `node::FatalError()`
5. `node::FatalError()` called `Environment::GetCurrent()`, then
   we went back to 3.

This patch fixes the incorrect guard in 3. When
`Environment::GetCurrent()` returns nullptr (when Environment is not
yet assigned to the context) in 2, it now prints the JS stack trace
and crashes directly.

PR-URL: #27236
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@nodejs-github-bot

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.