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 · Apr 17, 2019 · cdba9f23ec301f834ac686df7e6adcc3ecf59db6 · cdba9f2
1 parent 83d1ca7
commit cdba9f23ec301f834ac686df7e6adcc3ecf59db6
Showing with 39 additions and 17 deletions.

+10 −7 src/env-inl.h

+29 −10 src/node_errors.cc
diff --git 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<v8::Context> context) {
-  if (UNLIKELY(context.IsEmpty() ||
-      context->GetNumberOfEmbedderDataFields() <
-          ContextEmbedderIndex::kContextTag ||
-      context->GetAlignedPointerFromEmbedderData(
-          ContextEmbedderIndex::kContextTag) !=
-          Environment::kNodeContextTagPtr)) {
+  if (UNLIKELY(context.IsEmpty())) {
+    return nullptr;
+  }
+  if (UNLIKELY(context->GetNumberOfEmbedderDataFields() <=
+               ContextEmbedderIndex::kContextTag)) {
+    return nullptr;
+  }
+  if (UNLIKELY(context->GetAlignedPointerFromEmbedderData(
+                   ContextEmbedderIndex::kContextTag) !=
+               Environment::kNodeContextTagPtr)) {
     return nullptr;
   }
-
   return static_cast<Environment*>(
       context->GetAlignedPointerFromEmbedderData(
           ContextEmbedderIndex::kEnvironment));

diff --git 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 {
 
@@ -171,21 +172,27 @@ void PrintStackTrace(Isolate* isolate, Local<StackTrace> stack) {
   fflush(stderr);
 }
 
-void PrintCaughtException(Isolate* isolate,
-                          Local<Context> context,
-                          const v8::TryCatch& try_catch) {
-  CHECK(try_catch.HasCaught());
-  Local<Value> err = try_catch.Exception();
-  Local<Message> message = try_catch.Message();
-  Local<v8::StackTrace> stack = message->GetStackTrace();
+void PrintException(Isolate* isolate,
+                    Local<Context> context,
+                    Local<Value> err,
+                    Local<Message> message) {
   node::Utf8Value reason(isolate,
                          err->ToDetailString(context).ToLocalChecked());
   bool added_exception_line = false;
   std::string source =
       GetErrorSource(isolate, context, message, &added_exception_line);
   fprintf(stderr, "%s\n", source.c_str());
   fprintf(stderr, "%s\n", *reason);
-  PrintStackTrace(isolate, stack);
+
+  Local<v8::StackTrace> stack = message->GetStackTrace();
+  if (!stack.IsEmpty()) PrintStackTrace(isolate, stack);
+}
+
+void PrintCaughtException(Isolate* isolate,
+                          Local<Context> context,
+                          const v8::TryCatch& try_catch) {
+  CHECK(try_catch.HasCaught());
+  PrintException(isolate, context, try_catch.Exception(), try_catch.Message());
 }
 
 void AppendExceptionLine(Environment* env,
@@ -777,8 +784,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> context = isolate->GetCurrentContext();
+  Environment* env = Environment::GetCurrent(context);
+  if (env == nullptr) {
+    // 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 is useful and crash.
+    PrintException(isolate, context, error, message);
+    Abort();
+  }
+
   Local<Object> process_object = env->process_object();
   Local<String> fatal_exception_string = env->fatal_exception_string();
   Local<Value> fatal_exception_function =