From cdba9f23ec301f834ac686df7e6adcc3ecf59db6 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Mon, 15 Apr 2019 10:09:21 +0800
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/27236
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 src/env-inl.h      | 17 ++++++++++-------
 src/node_errors.cc | 39 +++++++++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/env-inl.h b/src/env-inl.h
index a78ed024d2b..e8943ffaf5a 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<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
index 83dbfc611fe..560409d96b6 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 {
 
@@ -171,13 +172,10 @@ 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;
@@ -185,7 +183,16 @@ 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<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 =