From 855759757acc416f84cdd0b66dd5c655d57e92cb 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                                   | 31 +++++++++++--------
 src/node_internals.h                          |  4 ++-
 .../message/vm_caught_custom_runtime_error.js | 18 +++++++++++
 .../vm_caught_custom_runtime_error.out        |  3 ++
 4 files changed, 42 insertions(+), 14 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 d05237e2b726b4..0a4b9718c02b2a 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1439,7 +1439,8 @@ ssize_t DecodeWrite(Isolate* isolate,
 
 void AppendExceptionLine(Environment* env,
                          Local<Value> er,
-                         Local<Message> message) {
+                         Local<Message> message,
+                         enum ErrorHandlingMode mode) {
   if (message.IsEmpty())
     return;
 
@@ -1521,19 +1522,23 @@ void AppendExceptionLine(Environment* env,
 
   Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
 
-  // Allocation failed, just print it out
-  if (arrow_str.IsEmpty() || err_obj.IsEmpty() || !err_obj->IsNativeError())
-    goto print;
-
-  err_obj->SetHiddenValue(env->arrow_message_string(), arrow_str);
-  return;
+  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);
 
- print:
-  if (env->printed_error())
+    uv_tty_reset_mode();
+    PrintErrorString("\n%s", arrow);
     return;
-  env->set_printed_error(true);
-  uv_tty_reset_mode();
-  PrintErrorString("\n%s", arrow);
+  }
+
+  err_obj->SetHiddenValue(env->arrow_message_string(), arrow_str);
 }
 
 
@@ -1542,7 +1547,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_internals.h b/src/node_internals.h
index 33ae25d6062938..5c94168c916816 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -126,9 +126,11 @@ constexpr size_t arraysize(const T(&)[N]) { return N; }
 # define NO_RETURN
 #endif
 
+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 = CONTEXTIFY_ERROR);
 
 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 00000000000000..237e8e3a105436
--- /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 00000000000000..9aa1e6c6480e3b
--- /dev/null
+++ b/test/message/vm_caught_custom_runtime_error.out
@@ -0,0 +1,3 @@
+beginning
+received error MyCustomError
+end