From 788df62787f48444de04ad0acc3205d840bc49de Mon Sep 17 00:00:00 2001 From: rudolftam <78262801+rudolftam@users.noreply.github.com> Date: Sat, 6 Feb 2021 11:42:26 +0200 Subject: [PATCH 1/5] src: fix a crashing issue in Error::ThrowAsJavaScriptException 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. --- napi-inl.h | 5 ++ test/error.cc | 62 +++++++++++++++++++ test/error_terminating_environment.js | 88 +++++++++++++++++++++++++++ test/index.js | 1 + 4 files changed, 156 insertions(+) create mode 100644 test/error_terminating_environment.js diff --git a/napi-inl.h b/napi-inl.h index 9d2f366e1..2ba30e8e9 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2393,6 +2393,11 @@ inline void Error::ThrowAsJavaScriptException() const { napi_status status = napi_throw(_env, Value()); + if (status == napi_pending_exception) { + // The environment could be terminating. + return; + } + #ifdef NAPI_CPP_EXCEPTIONS if (status != napi_ok) { throw Error::New(_env); diff --git a/test/error.cc b/test/error.cc index 44f4f79eb..15858182e 100644 --- a/test/error.cc +++ b/test/error.cc @@ -1,9 +1,54 @@ +#include #include "napi.h" using namespace Napi; namespace { +std::promise promise_for_child_process_; +std::promise promise_for_worker_thread_; + +void ResetPromises(const CallbackInfo&) { + promise_for_child_process_ = std::promise(); + promise_for_worker_thread_ = std::promise(); +} + +void WaitForWorkerThread(const CallbackInfo&) { + std::future 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().Value()) { + return; + } + + promise_for_worker_thread_.set_value(); + + std::future 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(); thrower({}); @@ -18,16 +63,22 @@ void ThrowApiError(const CallbackInfo& info) { void ThrowJSError(const CallbackInfo& info) { std::string message = info[0].As().Utf8Value(); + + ReleaseAndWaitForChildProcess(info, 1); throw Error::New(info.Env(), message); } void ThrowTypeError(const CallbackInfo& info) { std::string message = info[0].As().Utf8Value(); + + ReleaseAndWaitForChildProcess(info, 1); throw TypeError::New(info.Env(), message); } void ThrowRangeError(const CallbackInfo& info) { std::string message = info[0].As().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().Utf8Value(); + + ReleaseAndWaitForChildProcess(info, 1); Error::New(info.Env(), message).ThrowAsJavaScriptException(); } void ThrowTypeError(const CallbackInfo& info) { std::string message = info[0].As().Utf8Value(); + + ReleaseAndWaitForChildProcess(info, 1); TypeError::New(info.Env(), message).ThrowAsJavaScriptException(); } void ThrowRangeError(const CallbackInfo& info) { std::string message = info[0].As().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 new file mode 100644 index 000000000..10c417fb0 --- /dev/null +++ b/test/error_terminating_environment.js @@ -0,0 +1,88 @@ +'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`); +test(`./build/${buildType}/binding_noexcept.node`); + +function test(bindingPath) { + 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, + ] + ); + + assert(child_process.status === 0, `Test case ${i} failed`); + } +} diff --git a/test/index.js b/test/index.js index 46bb749f2..3d940d0d2 100644 --- 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) { From 5382c4a494a3d5f235e8bec40fdf45290c780497 Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Thu, 22 Apr 2021 21:06:07 +0200 Subject: [PATCH 2/5] src,doc: add compile option Introduce the compile option `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` to dictate fix for crashing behavior. --- common.gypi | 1 + doc/setup.md | 7 +++++++ napi-inl.h | 27 ++++++++++++++++++++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/common.gypi b/common.gypi index 9be254f0b..4955dc9bc 100644 --- a/common.gypi +++ b/common.gypi @@ -15,6 +15,7 @@ } }] ], + 'defines': [ 'NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS' ], 'include_dirs': [" Date: Fri, 23 Apr 2021 10:01:32 +0200 Subject: [PATCH 3/5] Address review comments - Move define to src - Correct fix implementation --- common.gypi | 1 - napi-inl.h | 5 ----- test/error.cc | 1 + 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/common.gypi b/common.gypi index 4955dc9bc..9be254f0b 100644 --- a/common.gypi +++ b/common.gypi @@ -15,7 +15,6 @@ } }] ], - 'defines': [ 'NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS' ], 'include_dirs': [" #include "napi.h" From 0187fecdedd67cb78ffaeefa972c9a6338b7ccb7 Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Fri, 23 Apr 2021 11:08:53 +0200 Subject: [PATCH 4/5] Move compile option back to gyp file --- common.gypi | 1 + test/error.cc | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/common.gypi b/common.gypi index 9be254f0b..4955dc9bc 100644 --- a/common.gypi +++ b/common.gypi @@ -15,6 +15,7 @@ } }] ], + 'defines': [ 'NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS' ], 'include_dirs': [" #include "napi.h" From 3f291f71bc1e083f87930f770cd4d0f412b63cc6 Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Tue, 27 Apr 2021 18:03:13 +0200 Subject: [PATCH 5/5] test: Move test to separate target --- common.gypi | 1 - napi-inl.h | 3 ++- test/binding-swallowexcept.cc | 12 +++++++++++ test/binding.gyp | 30 ++++++++++++++++++++++----- test/error_terminating_environment.js | 14 +++++++++---- 5 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 test/binding-swallowexcept.cc diff --git a/common.gypi b/common.gypi index 4955dc9bc..9be254f0b 100644 --- a/common.gypi +++ b/common.gypi @@ -15,7 +15,6 @@ } }] ], - 'defines': [ 'NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS' ], 'include_dirs': ["@(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_terminating_environment.js b/test/error_terminating_environment.js index 10c417fb0..63b42259e 100644 --- a/test/error_terminating_environment.js +++ b/test/error_terminating_environment.js @@ -66,10 +66,12 @@ if (process.argv[2] === 'runInWorkerThread') { assert.fail('This should not be reachable'); } -test(`./build/${buildType}/binding.node`); -test(`./build/${buildType}/binding_noexcept.node`); +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) { +function test(bindingPath, process_should_abort) { const number_of_test_cases = 5; for (let i = 0; i < number_of_test_cases; ++i) { @@ -83,6 +85,10 @@ function test(bindingPath) { ] ); - assert(child_process.status === 0, `Test case ${i} failed`); + 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`); + } } }