src: fix Error::ThrowAsJavaScriptException crash

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash. PR-URL: #975 Reviewed-By: Nicola Del Gobbo <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Jun 14, 2021 · 028107f · 028107f
1 parent fed1353
commit 028107f
Show file tree

Hide file tree

Showing 7 changed files with 228 additions and 6 deletions.
diff --git a/doc/setup.md b/doc/setup.md
@@ -81,3 +81,10 @@ targeted node version *does not* have Node-API built-in.
 
 The preprocessor directive `NODE_ADDON_API_DISABLE_DEPRECATED` can be defined at
 compile time before including `napi.h` to skip the definition of deprecated APIs.
+
+By default, throwing an exception on a terminating environment (eg. worker
+threads) will cause a fatal exception, terminating the Node process. This is to
+provide feedback to the user of the runtime error, as it is impossible to pass
+the error to JavaScript when the environment is terminating. In order to bypass
+this behavior such that the Node process will not terminate, define the
+preprocessor directive `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS`.
diff --git a/napi-inl.h b/napi-inl.h
@@ -2397,12 +2397,38 @@ inline const std::string& Error::Message() const NAPI_NOEXCEPT {
 inline void Error::ThrowAsJavaScriptException() const {
   HandleScope scope(_env);
   if (!IsEmpty()) {
-
+#ifdef NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS
+    bool pendingException = false;
+
+    // check if there is already a pending exception. If so don't try to throw a
+    // new one as that is not allowed/possible
+    napi_status status = napi_is_exception_pending(_env, &pendingException);
+
+    if ((status != napi_ok) ||
+        ((status == napi_ok) && (pendingException == false))) {
+      // We intentionally don't use `NAPI_THROW_*` macros here to ensure
+      // that there is no possible recursion as `ThrowAsJavaScriptException`
+      // is part of `NAPI_THROW_*` macro definition for noexcept.
+
+      status = napi_throw(_env, Value());
+
+      if (status == napi_pending_exception) {
+        // The environment must be terminating as we checked earlier and there
+        // was no pending exception. In this case continuing will result
+        // in a fatal error and there is nothing the author has done incorrectly
+        // in their code that is worth flagging through a fatal error
+        return;
+      }
+    } else {
+      status = napi_pending_exception;
+    }
+#else
     // We intentionally don't use `NAPI_THROW_*` macros here to ensure
     // that there is no possible recursion as `ThrowAsJavaScriptException`
     // is part of `NAPI_THROW_*` macro definition for noexcept.
 
     napi_status status = napi_throw(_env, Value());
+#endif
 
 #ifdef NAPI_CPP_EXCEPTIONS
     if (status != napi_ok) {

diff --git a/test/binding-swallowexcept.cc b/test/binding-swallowexcept.cc
@@ -0,0 +1,12 @@
+#include "napi.h"
+
+using namespace Napi;
+
+Object InitError(Env env);
+
+Object Init(Env env, Object exports) {
+  exports.Set("error", InitError(env));
+  return exports;
+}
+
+NODE_API_MODULE(addon, Init)
diff --git a/test/binding.gyp b/test/binding.gyp
@@ -1,7 +1,8 @@
 {
   'target_defaults': {
     'includes': ['../common.gypi'],
-    'sources': [
+    'variables': {
+      'build_sources': [
         'addon.cc',
         'addon_data.cc',
         'arraybuffer.cc',
@@ -67,20 +68,39 @@
         'version_management.cc',
         'thunking_manual.cc',
       ],
+      'build_sources_swallowexcept': [
+        'binding-swallowexcept.cc',
+        'error.cc',
+      ],
       'conditions': [
         ['disable_deprecated!="true"', {
-          'sources': ['object/object_deprecated.cc']
+          'build_sources': ['object/object_deprecated.cc']
         }]
-      ],
+      ]
+    },
   },
   'targets': [
     {
       'target_name': 'binding',
-      'includes': ['../except.gypi']
+      'includes': ['../except.gypi'],
+      'sources': ['>@(build_sources)']
     },
     {
       'target_name': 'binding_noexcept',
-      'includes': ['../noexcept.gypi']
+      'includes': ['../noexcept.gypi'],
+      'sources': ['>@(build_sources)']
+    },
+    {
+      'target_name': 'binding_swallowexcept',
+      'includes': ['../except.gypi'],
+      'sources': [ '>@(build_sources_swallowexcept)'],
+      'defines': ['NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS']
+    },
+    {
+      'target_name': 'binding_swallowexcept_noexcept',
+      'includes': ['../noexcept.gypi'],
+      'sources': ['>@(build_sources_swallowexcept)'],
+      'defines': ['NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS']
     },
   ],
 }
diff --git a/test/error.cc b/test/error.cc
@@ -1,9 +1,54 @@
+#include <future>
 #include "napi.h"
 
 using namespace Napi;
 
 namespace {
 
+std::promise<void> promise_for_child_process_;
+std::promise<void> promise_for_worker_thread_;
+
+void ResetPromises(const CallbackInfo&) {
+  promise_for_child_process_ = std::promise<void>();
+  promise_for_worker_thread_ = std::promise<void>();
+}
+
+void WaitForWorkerThread(const CallbackInfo&) {
+  std::future<void> future = promise_for_worker_thread_.get_future();
+
+  std::future_status status = future.wait_for(std::chrono::seconds(5));
+
+  if (status != std::future_status::ready) {
+    Error::Fatal("WaitForWorkerThread", "status != std::future_status::ready");
+  }
+}
+
+void ReleaseAndWaitForChildProcess(const CallbackInfo& info,
+                                   const uint32_t index) {
+  if (info.Length() < index + 1) {
+    return;
+  }
+
+  if (!info[index].As<Boolean>().Value()) {
+    return;
+  }
+
+  promise_for_worker_thread_.set_value();
+
+  std::future<void> future = promise_for_child_process_.get_future();
+
+  std::future_status status = future.wait_for(std::chrono::seconds(5));
+
+  if (status != std::future_status::ready) {
+    Error::Fatal("ReleaseAndWaitForChildProcess",
+                 "status != std::future_status::ready");
+  }
+}
+
+void ReleaseWorkerThread(const CallbackInfo&) {
+  promise_for_child_process_.set_value();
+}
+
 void DoNotCatch(const CallbackInfo& info) {
   Function thrower = info[0].As<Function>();
   thrower({});
@@ -18,16 +63,22 @@ void ThrowApiError(const CallbackInfo& info) {
 
 void ThrowJSError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   throw Error::New(info.Env(), message);
 }
 
 void ThrowTypeError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   throw TypeError::New(info.Env(), message);
 }
 
 void ThrowRangeError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   throw RangeError::New(info.Env(), message);
 }
 
@@ -83,16 +134,22 @@ void CatchAndRethrowErrorThatEscapesScope(const CallbackInfo& info) {
 
 void ThrowJSError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   Error::New(info.Env(), message).ThrowAsJavaScriptException();
 }
 
 void ThrowTypeError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   TypeError::New(info.Env(), message).ThrowAsJavaScriptException();
 }
 
 void ThrowRangeError(const CallbackInfo& info) {
   std::string message = info[0].As<String>().Utf8Value();
+
+  ReleaseAndWaitForChildProcess(info, 1);
   RangeError::New(info.Env(), message).ThrowAsJavaScriptException();
 }
 
@@ -187,6 +244,8 @@ void ThrowDefaultError(const CallbackInfo& info) {
     Error::Fatal("ThrowDefaultError", "napi_get_named_property");
   }
 
+  ReleaseAndWaitForChildProcess(info, 1);
+
   // The macro creates a `Napi::Error` using the factory that takes only the
   // env, however, it heeds the exception mechanism to be used.
   NAPI_THROW_IF_FAILED_VOID(env, status);
@@ -209,5 +268,8 @@ Object InitError(Env env) {
     Function::New(env, CatchAndRethrowErrorThatEscapesScope);
   exports["throwFatalError"] = Function::New(env, ThrowFatalError);
   exports["throwDefaultError"] = Function::New(env, ThrowDefaultError);
+  exports["resetPromises"] = Function::New(env, ResetPromises);
+  exports["waitForWorkerThread"] = Function::New(env, WaitForWorkerThread);
+  exports["releaseWorkerThread"] = Function::New(env, ReleaseWorkerThread);
   return exports;
 }
diff --git a/test/error_terminating_environment.js b/test/error_terminating_environment.js
@@ -0,0 +1,94 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const assert = require('assert');
+
+// These tests ensure that Error types can be used in a terminating
+// environment without triggering any fatal errors.
+
+if (process.argv[2] === 'runInChildProcess') {
+  const binding_path = process.argv[3];
+  const index_for_test_case = Number(process.argv[4]);
+
+  const binding = require(binding_path);
+
+  // Use C++ promises to ensure the worker thread is terminated right
+  // before running the testable code in the binding.
+
+  binding.error.resetPromises()
+
+  const { Worker } = require('worker_threads');
+
+  const worker = new Worker(
+    __filename,
+    {
+      argv: [
+        'runInWorkerThread',
+        binding_path,
+        index_for_test_case,
+      ]
+    }
+  );
+
+  binding.error.waitForWorkerThread()
+
+  worker.terminate();
+
+  binding.error.releaseWorkerThread()
+
+  return;
+}
+
+if (process.argv[2] === 'runInWorkerThread') {
+  const binding_path = process.argv[3];
+  const index_for_test_case = Number(process.argv[4]);
+
+  const binding = require(binding_path);
+
+  switch (index_for_test_case) {
+    case 0:
+      binding.error.throwJSError('test', true);
+      break;
+    case 1:
+      binding.error.throwTypeError('test', true);
+      break;
+    case 2:
+      binding.error.throwRangeError('test', true);
+      break;
+    case 3:
+      binding.error.throwDefaultError(false, true);
+      break;
+    case 4:
+      binding.error.throwDefaultError(true, true);
+      break;
+    default: assert.fail('Invalid index');
+  }
+
+  assert.fail('This should not be reachable');
+}
+
+test(`./build/${buildType}/binding.node`, true);
+test(`./build/${buildType}/binding_noexcept.node`, true);
+test(`./build/${buildType}/binding_swallowexcept.node`, false);
+test(`./build/${buildType}/binding_swallowexcept_noexcept.node`, false);
+
+function test(bindingPath, process_should_abort) {
+  const number_of_test_cases = 5;
+
+  for (let i = 0; i < number_of_test_cases; ++i) {
+    const child_process = require('./napi_child').spawnSync(
+      process.execPath,
+      [
+        __filename,
+        'runInChildProcess',
+        bindingPath,
+        i,
+      ]
+    );
+
+    if (process_should_abort) {
+      assert(child_process.status !== 0, `Test case ${bindingPath} ${i} failed: Process exited with status code 0.`);
+    } else {
+      assert(child_process.status === 0, `Test case ${bindingPath} ${i} failed: Process status ${child_process.status} is non-zero`);
+    }
+  }
+}
diff --git a/test/index.js b/test/index.js
@@ -110,6 +110,7 @@ if (napiVersion < 6) {
 
 if (majorNodeVersion < 12) {
   testModules.splice(testModules.indexOf('objectwrap_worker_thread'), 1);
+  testModules.splice(testModules.indexOf('error_terminating_environment'), 1);
 }
 
 if (napiVersion < 8) {