From 3cac616791984ba2f2a57d2d17f973e4d7b6a598 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 24 Jun 2016 05:50:00 +0200
Subject: [PATCH] vm: don't print out arrow message for custom error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: https://github.com/nodejs/node/issues/7397
PR-URL: https://github.com/nodejs/node/pull/7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/node.cc                                   | 33 +++++++++++--------
 src/node_contextify.cc                        |  2 +-
 src/node_internals.h                          |  4 ++-
 .../message/vm_caught_custom_runtime_error.js | 18 ++++++++++
 .../vm_caught_custom_runtime_error.out        |  3 ++
 5 files changed, 45 insertions(+), 15 deletions(-)
 create mode 100644 test/message/vm_caught_custom_runtime_error.js
 create mode 100644 test/message/vm_caught_custom_runtime_error.out

diff --git a/src/node.cc b/src/node.cc
index 511a4a25fd..66bdc1c13f 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1460,7 +1460,8 @@ bool IsExceptionDecorated(Environment* env, Local<Value> er) {
 
 void AppendExceptionLine(Environment* env,
                          Local<Value> er,
-                         Local<Message> message) {
+                         Local<Message> message,
+                         enum ErrorHandlingMode mode) {
   if (message.IsEmpty())
     return;
 
@@ -1547,20 +1548,26 @@ void AppendExceptionLine(Environment* env,
 
   Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
 
-  if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) {
-    err_obj->SetPrivate(
-        env->context(),
-        env->arrow_message_private_symbol(),
-        arrow_str);
+  const bool can_set_arrow = !arrow_str.IsEmpty() && !err_obj.IsEmpty();
+  // If allocating arrow_str failed, print it out. There's not much else to do.
+  // If it's not an error, but something needs to be printed out because
+  // it's a fatal exception, also print it out from here.
+  // Otherwise, the arrow property will be attached to the object and handled
+  // by the caller.
+  if (!can_set_arrow || (mode == FATAL_ERROR && !err_obj->IsNativeError())) {
+    if (env->printed_error())
+      return;
+    env->set_printed_error(true);
+
+    uv_tty_reset_mode();
+    PrintErrorString("\n%s", arrow);
     return;
   }
 
-  // Allocation failed, just print it out.
-  if (env->printed_error())
-    return;
-  env->set_printed_error(true);
-  uv_tty_reset_mode();
-  PrintErrorString("\n%s", arrow);
+  CHECK(err_obj->SetPrivate(
+            env->context(),
+            env->arrow_message_private_symbol(),
+            arrow_str).FromMaybe(false));
 }
 
 
@@ -1569,7 +1576,7 @@ static void ReportException(Environment* env,
                             Local<Message> message) {
   HandleScope scope(env->isolate());
 
-  AppendExceptionLine(env, er, message);
+  AppendExceptionLine(env, er, message, FATAL_ERROR);
 
   Local<Value> trace_value;
   Local<Value> arrow;
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 07fe998bd4..7ec03c10f7 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -633,7 +633,7 @@ class ContextifyScript : public BaseObject {
     if (IsExceptionDecorated(env, err_obj))
       return;
 
-    AppendExceptionLine(env, exception, try_catch.Message());
+    AppendExceptionLine(env, exception, try_catch.Message(), CONTEXTIFY_ERROR);
     Local<Value> stack = err_obj->Get(env->stack_string());
     auto maybe_value =
         err_obj->GetPrivate(
diff --git a/src/node_internals.h b/src/node_internals.h
index 3f2c0f02cc..ff384231c1 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -135,9 +135,11 @@ constexpr size_t arraysize(const T(&)[N]) { return N; }
 
 bool IsExceptionDecorated(Environment* env, v8::Local<v8::Value> er);
 
+enum ErrorHandlingMode { FATAL_ERROR, CONTEXTIFY_ERROR };
 void AppendExceptionLine(Environment* env,
                          v8::Local<v8::Value> er,
-                         v8::Local<v8::Message> message);
+                         v8::Local<v8::Message> message,
+                         enum ErrorHandlingMode mode);
 
 NO_RETURN void FatalError(const char* location, const char* message);
 
diff --git a/test/message/vm_caught_custom_runtime_error.js b/test/message/vm_caught_custom_runtime_error.js
new file mode 100644
index 0000000000..237e8e3a10
--- /dev/null
+++ b/test/message/vm_caught_custom_runtime_error.js
@@ -0,0 +1,18 @@
+'use strict';
+require('../common');
+const vm = require('vm');
+
+console.error('beginning');
+
+// Regression test for https://github.com/nodejs/node/issues/7397:
+// vm.runInThisContext() should not print out anything to stderr by itself.
+try {
+  vm.runInThisContext(`throw ({
+    name: 'MyCustomError',
+    message: 'This is a custom message'
+  })`, { filename: 'test.vm' });
+} catch (e) {
+  console.error('received error', e.name);
+}
+
+console.error('end');
diff --git a/test/message/vm_caught_custom_runtime_error.out b/test/message/vm_caught_custom_runtime_error.out
new file mode 100644
index 0000000000..9aa1e6c648
--- /dev/null
+++ b/test/message/vm_caught_custom_runtime_error.out
@@ -0,0 +1,3 @@
+beginning
+received error MyCustomError
+end