From cee0872ce85580a86f0d7c7e53e032cebec89e82 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 15 Apr 2019 10:09:21 +0800 Subject: [PATCH 1/2] 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. --- src/env-inl.h | 10 +++++----- src/node_errors.cc | 43 +++++++++++++++++++++++++++++++++---------- 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index a78ed024d2bbf9..9b8fd1b699e895 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -303,11 +303,11 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { inline Environment* Environment::GetCurrent(v8::Local context) { if (UNLIKELY(context.IsEmpty() || - context->GetNumberOfEmbedderDataFields() < - ContextEmbedderIndex::kContextTag || - context->GetAlignedPointerFromEmbedderData( - ContextEmbedderIndex::kContextTag) != - Environment::kNodeContextTagPtr)) { + context->GetNumberOfEmbedderDataFields() <= + ContextEmbedderIndex::kContextTag || + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::kContextTag) != + Environment::kNodeContextTagPtr)) { return nullptr; } diff --git a/src/node_errors.cc b/src/node_errors.cc index 3c04152974339e..825c3e423f233e 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -6,6 +6,7 @@ #ifdef NODE_REPORT #include "node_report.h" #endif +#include "node_v8_platform-inl.h" namespace node { @@ -170,13 +171,10 @@ void PrintStackTrace(Isolate* isolate, Local stack) { fflush(stderr); } -void PrintCaughtException(Isolate* isolate, - Local context, - const v8::TryCatch& try_catch) { - CHECK(try_catch.HasCaught()); - Local err = try_catch.Exception(); - Local message = try_catch.Message(); - Local stack = message->GetStackTrace(); +void PrintException(Isolate* isolate, + Local context, + Local err, + Local message) { node::Utf8Value reason(isolate, err->ToDetailString(context).ToLocalChecked()); bool added_exception_line = false; @@ -184,7 +182,20 @@ void PrintCaughtException(Isolate* isolate, GetErrorSource(isolate, context, message, &added_exception_line); fprintf(stderr, "%s\n", source.c_str()); fprintf(stderr, "%s\n", *reason); - PrintStackTrace(isolate, stack); + + Local stack = message->GetStackTrace(); + if (stack.IsEmpty()) { + return; + } else { + PrintStackTrace(isolate, stack); + } +} + +void PrintCaughtException(Isolate* isolate, + Local context, + const v8::TryCatch& try_catch) { + CHECK(try_catch.HasCaught()); + PrintException(isolate, context, try_catch.Exception(), try_catch.Message()); } void AppendExceptionLine(Environment* env, @@ -775,8 +786,20 @@ void FatalException(Isolate* isolate, CHECK(!error.IsEmpty()); HandleScope scope(isolate); - Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + CHECK(isolate->InContext()); + Local context = isolate->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + if (env == nullptr) { + // This could happen before Environment is assigned to the context. + PrintException(isolate, context, error, message); + // XXX(joyeecheung): this could also happen to a worker, but since + // we don't have access to Environment yet, there is not much + // we can do at this point but to exit directly. + isolate->TerminateExecution(); + DisposePlatform(); + exit(6); + } + Local process_object = env->process_object(); Local fatal_exception_string = env->fatal_exception_string(); Local fatal_exception_function = From 7933801bed4f24252c4f8b8ee0ae139a27dab2cf Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 15 Apr 2019 20:50:55 +0800 Subject: [PATCH 2/2] fixup! src: handle fatal error when Environment is not assigned to context --- src/env-inl.h | 15 +++++++++------ src/node_errors.cc | 20 ++++++++------------ 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 9b8fd1b699e895..e8943ffaf5af38 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -302,15 +302,18 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { } inline Environment* Environment::GetCurrent(v8::Local context) { - if (UNLIKELY(context.IsEmpty() || - context->GetNumberOfEmbedderDataFields() <= - ContextEmbedderIndex::kContextTag || - context->GetAlignedPointerFromEmbedderData( + if (UNLIKELY(context.IsEmpty())) { + return nullptr; + } + if (UNLIKELY(context->GetNumberOfEmbedderDataFields() <= + ContextEmbedderIndex::kContextTag)) { + return nullptr; + } + if (UNLIKELY(context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kContextTag) != - Environment::kNodeContextTagPtr)) { + Environment::kNodeContextTagPtr)) { return nullptr; } - return static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kEnvironment)); diff --git a/src/node_errors.cc b/src/node_errors.cc index 825c3e423f233e..7f134d236404e7 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -184,11 +184,7 @@ void PrintException(Isolate* isolate, fprintf(stderr, "%s\n", *reason); Local stack = message->GetStackTrace(); - if (stack.IsEmpty()) { - return; - } else { - PrintStackTrace(isolate, stack); - } + if (!stack.IsEmpty()) PrintStackTrace(isolate, stack); } void PrintCaughtException(Isolate* isolate, @@ -790,14 +786,14 @@ void FatalException(Isolate* isolate, Local context = isolate->GetCurrentContext(); Environment* env = Environment::GetCurrent(context); if (env == nullptr) { - // This could happen before Environment is assigned to the context. + // This means that the exception happens before Environment is assigned + // to the context e.g. when there is a SyntaxError in a per-context + // script - which usually indicates that there is a bug because no JS + // error is supposed to be thrown at this point. + // Since we don't have access to Environment here, there is not + // much we can do, so we just print whatever useful and crash. PrintException(isolate, context, error, message); - // XXX(joyeecheung): this could also happen to a worker, but since - // we don't have access to Environment yet, there is not much - // we can do at this point but to exit directly. - isolate->TerminateExecution(); - DisposePlatform(); - exit(6); + Abort(); } Local process_object = env->process_object();