node-api: emit uncaught-exception on unhandled tsfn callbacks

PR-URL: https://github.com/nodejs/node/pull/36510
Fixes: https://github.com/nodejs/node/issues/36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit is contained in:
legendecas 2020-12-14 19:10:51 +08:00 committed by Michael Dawson
parent 6375b13de8
commit 681fb3a200
13 changed files with 304 additions and 46 deletions

View File

@ -439,6 +439,18 @@ under this flag.
To allow polyfills to be added, `--require` runs before freezing intrinsics.
### `--force-node-api-uncaught-exceptions-policy`
<!-- YAML
added: REPLACEME
-->
Enforces `uncaughtException` event on Node-API asynchronous callbacks.
To prevent from an existing add-on from crashing the process, this flag is not
enabled by default. In the future, this flag will be enabled by default to
enforce the correct behavior.
### `--heapsnapshot-near-heap-limit=max_count`
<!-- YAML
@ -1638,6 +1650,7 @@ Node.js options that are allowed are:
* `--experimental-wasm-modules`
* `--force-context-aware`
* `--force-fips`
* `--force-node-api-uncaught-exceptions-policy`
* `--frozen-intrinsics`
* `--heapsnapshot-near-heap-limit`
* `--heapsnapshot-signal`

View File

@ -57,15 +57,7 @@ struct napi_env__ {
CHECK_EQ(isolate, context->GetIsolate());
napi_clear_last_error(this);
}
virtual ~napi_env__() {
// First we must finalize those references that have `napi_finalizer`
// callbacks. The reason is that addons might store other references which
// they delete during their `napi_finalizer` callbacks. If we deleted such
// references here first, they would be doubly deleted when the
// `napi_finalizer` deleted them subsequently.
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
v8impl::RefTracker::FinalizeAll(&reflist);
}
virtual ~napi_env__() { FinalizeAll(); }
v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
v8impl::Persistent<v8::Context> context_persistent;
@ -102,11 +94,23 @@ struct napi_env__ {
}
}
// This should be overridden to schedule the finalization to a properiate
// timing, like next tick of the event loop.
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
v8::HandleScope handle_scope(isolate);
CallIntoModule([&](napi_env env) { cb(env, data, hint); });
}
void FinalizeAll() {
// First we must finalize those references that have `napi_finalizer`
// callbacks. The reason is that addons might store other references which
// they delete during their `napi_finalizer` callbacks. If we deleted such
// references here first, they would be doubly deleted when the
// `napi_finalizer` deleted them subsequently.
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
v8impl::RefTracker::FinalizeAll(&reflist);
}
v8impl::Persistent<v8::Value> last_exception;
// We store references in two different lists, depending on whether they have

View File

@ -9,6 +9,7 @@
#include "node_buffer.h"
#include "node_errors.h"
#include "node_internals.h"
#include "node_process.h"
#include "node_url.h"
#include "threadpoolwork-inl.h"
#include "tracing/traced_value.h"
@ -23,6 +24,11 @@ node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
CHECK_NOT_NULL(node_env());
}
node_napi_env__::~node_napi_env__() {
destructing = true;
FinalizeAll();
}
bool node_napi_env__::can_call_into_js() const {
return node_env()->can_call_into_js();
}
@ -35,19 +41,64 @@ v8::Maybe<bool> node_napi_env__::mark_arraybuffer_as_untransferable(
}
void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
CallFinalizer<true>(cb, data, hint);
}
template <bool enforceUncaughtExceptionPolicy>
void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
if (destructing) {
// we can not defer finalizers when the environment is being destructed.
v8::HandleScope handle_scope(isolate);
v8::Context::Scope context_scope(context());
CallbackIntoModule<enforceUncaughtExceptionPolicy>(
[&](napi_env env) { cb(env, data, hint); });
return;
}
// we need to keep the env live until the finalizer has been run
// EnvRefHolder provides an exception safe wrapper to Ref and then
// Unref once the lambda is freed
EnvRefHolder liveEnv(static_cast<napi_env>(this));
node_env()->SetImmediate(
[=, liveEnv = std::move(liveEnv)](node::Environment* node_env) {
napi_env env = liveEnv.env();
node_napi_env__* env = static_cast<node_napi_env__*>(liveEnv.env());
v8::HandleScope handle_scope(env->isolate);
v8::Context::Scope context_scope(env->context());
env->CallIntoModule([&](napi_env env) { cb(env, data, hint); });
env->CallbackIntoModule<enforceUncaughtExceptionPolicy>(
[&](napi_env env) { cb(env, data, hint); });
});
}
void node_napi_env__::trigger_fatal_exception(v8::Local<v8::Value> local_err) {
v8::Local<v8::Message> local_msg =
v8::Exception::CreateMessage(isolate, local_err);
node::errors::TriggerUncaughtException(isolate, local_err, local_msg);
}
// option enforceUncaughtExceptionPolicy is added for not breaking existing
// running n-api add-ons, and should be deprecated in the next major Node.js
// release.
template <bool enforceUncaughtExceptionPolicy, typename T>
void node_napi_env__::CallbackIntoModule(T&& call) {
CallIntoModule(call, [](napi_env env_, v8::Local<v8::Value> local_err) {
node_napi_env__* env = static_cast<node_napi_env__*>(env_);
node::Environment* node_env = env->node_env();
if (!node_env->options()->force_node_api_uncaught_exceptions_policy &&
!enforceUncaughtExceptionPolicy) {
ProcessEmitDeprecationWarning(
node_env,
"Uncaught N-API callback exception detected, please run node "
"with option --force-node-api-uncaught-exceptions-policy=true"
"to handle those exceptions properly.",
"DEP0XXX");
return;
}
// If there was an unhandled exception in the complete callback,
// report it as a fatal exception. (There is no JavaScript on the
// callstack that can possibly handle it.)
env->trigger_fatal_exception(local_err);
});
}
namespace v8impl {
namespace {
@ -60,20 +111,10 @@ class BufferFinalizer : private Finalizer {
static_cast<BufferFinalizer*>(hint)};
finalizer->_finalize_data = data;
node::Environment* node_env =
static_cast<node_napi_env>(finalizer->_env)->node_env();
node_env->SetImmediate(
[finalizer = std::move(finalizer)](node::Environment* env) {
if (finalizer->_finalize_callback == nullptr) return;
v8::HandleScope handle_scope(finalizer->_env->isolate);
v8::Context::Scope context_scope(finalizer->_env->context());
finalizer->_env->CallIntoModule([&](napi_env env) {
finalizer->_finalize_callback(
env, finalizer->_finalize_data, finalizer->_finalize_hint);
});
});
if (finalizer->_finalize_callback == nullptr) return;
finalizer->_env->CallFinalizer(finalizer->_finalize_callback,
finalizer->_finalize_data,
finalizer->_finalize_hint);
}
struct Deleter {
@ -102,13 +143,6 @@ static inline napi_env NewEnv(v8::Local<v8::Context> context,
return result;
}
static inline void trigger_fatal_exception(napi_env env,
v8::Local<v8::Value> local_err) {
v8::Local<v8::Message> local_msg =
v8::Exception::CreateMessage(env->isolate, local_err);
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
}
class ThreadSafeFunction : public node::AsyncResource {
public:
ThreadSafeFunction(v8::Local<v8::Function> func,
@ -325,7 +359,7 @@ class ThreadSafeFunction : public node::AsyncResource {
v8::Local<v8::Function>::New(env->isolate, ref);
js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
}
env->CallIntoModule(
env->CallbackIntoModule<false>(
[&](napi_env env) { call_js_cb(env, js_callback, context, data); });
}
@ -336,7 +370,9 @@ class ThreadSafeFunction : public node::AsyncResource {
v8::HandleScope scope(env->isolate);
if (finalize_cb) {
CallbackScope cb_scope(this);
env->CallIntoModule(
// Do not use CallFinalizer since it will defer the invocation, which
// would lead to accessing a deleted ThreadSafeFunction.
env->CallbackIntoModule<false>(
[&](napi_env env) { finalize_cb(env, finalize_data, context); });
}
EmptyQueueAndDelete();
@ -719,7 +755,7 @@ napi_status NAPI_CDECL napi_fatal_exception(napi_env env, napi_value err) {
CHECK_ARG(env, err);
v8::Local<v8::Value> local_err = v8impl::V8LocalValueFromJsValue(err);
v8impl::trigger_fatal_exception(env, local_err);
static_cast<node_napi_env>(env)->trigger_fatal_exception(local_err);
return napi_clear_last_error(env);
}
@ -1064,16 +1100,9 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork {
CallbackScope callback_scope(this);
_env->CallIntoModule(
[&](napi_env env) {
_complete(env, ConvertUVErrorCode(status), _data);
},
[](napi_env env, v8::Local<v8::Value> local_err) {
// If there was an unhandled exception in the complete callback,
// report it as a fatal exception. (There is no JavaScript on the
// callstack that can possibly handle it.)
v8impl::trigger_fatal_exception(env, local_err);
});
_env->CallbackIntoModule<true>([&](napi_env env) {
_complete(env, ConvertUVErrorCode(status), _data);
});
// Note: Don't access `work` after this point because it was
// likely deleted by the complete callback.

View File

@ -11,11 +11,18 @@
struct node_napi_env__ : public napi_env__ {
node_napi_env__(v8::Local<v8::Context> context,
const std::string& module_filename);
~node_napi_env__();
bool can_call_into_js() const override;
v8::Maybe<bool> mark_arraybuffer_as_untransferable(
v8::Local<v8::ArrayBuffer> ab) const override;
void CallFinalizer(napi_finalize cb, void* data, void* hint) override;
template <bool enforceUncaughtExceptionPolicy>
void CallFinalizer(napi_finalize cb, void* data, void* hint);
void trigger_fatal_exception(v8::Local<v8::Value> local_err);
template <bool enforceUncaughtExceptionPolicy, typename T>
void CallbackIntoModule(T&& call);
inline node::Environment* node_env() const {
return node::Environment::GetCurrent(context());
@ -23,6 +30,7 @@ struct node_napi_env__ : public napi_env__ {
inline const char* GetFilename() const { return filename.c_str(); }
std::string filename;
bool destructing = false;
};
using node_napi_env = node_napi_env__*;

View File

@ -432,6 +432,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::force_async_hooks_checks,
kAllowedInEnvironment,
true);
AddOption(
"--force-node-api-uncaught-exceptions-policy",
"enforces 'uncaughtException' event on Node API asynchronous callbacks",
&EnvironmentOptions::force_node_api_uncaught_exceptions_policy,
kAllowedInEnvironment,
false);
AddOption("--addons",
"disable loading native addons",
&EnvironmentOptions::allow_native_addons,

View File

@ -120,6 +120,7 @@ class EnvironmentOptions : public Options {
bool experimental_repl_await = true;
bool experimental_vm_modules = false;
bool expose_internals = false;
bool force_node_api_uncaught_exceptions_policy = false;
bool frozen_intrinsics = false;
int64_t heap_snapshot_near_heap_limit = 0;
std::string heap_snapshot_signal;

View File

@ -0,0 +1,20 @@
'use strict';
// Flags: --expose-gc --force-node-api-uncaught-exceptions-policy
const common = require('../../common');
const test_reference = require(`./build/${common.buildType}/test_reference`);
const assert = require('assert');
process.on('uncaughtException', common.mustCall((err) => {
assert.throws(() => { throw err; }, /finalizer error/);
}));
(async function() {
{
test_reference.createExternalWithJsFinalize(
common.mustCall(() => {
throw new Error('finalizer error');
}));
}
global.gc();
})().then(common.mustCall());

View File

@ -21,6 +21,20 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
finalize_count++;
}
static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
int *actual_value = data;
NODE_API_ASSERT_RETURN_VOID(env, actual_value == &test_value,
"The correct pointer was passed to the finalizer");
napi_ref finalizer_ref = (napi_ref)hint;
napi_value js_finalizer;
napi_value recv;
NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
}
static napi_value CreateExternal(napi_env env, napi_callback_info info) {
int* data = &test_value;
@ -99,6 +113,31 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
return result;
}
static napi_value
CreateExternalWithJsFinalize(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
napi_value finalizer = args[0];
napi_valuetype finalizer_valuetype;
NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
napi_ref finalizer_ref;
NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
napi_value result;
NODE_API_CALL(env,
napi_create_external(env,
&test_value,
FinalizeExternalCallJs,
finalizer_ref, /* finalize_hint */
&result));
finalize_count = 0;
return result;
}
static napi_value CheckExternal(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value arg;
@ -223,6 +262,8 @@ napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal),
DECLARE_NODE_API_PROPERTY("createExternalWithFinalize",
CreateExternalWithFinalize),
DECLARE_NODE_API_PROPERTY("createExternalWithJsFinalize",
CreateExternalWithJsFinalize),
DECLARE_NODE_API_PROPERTY("checkExternal", CheckExternal),
DECLARE_NODE_API_PROPERTY("createReference", CreateReference),
DECLARE_NODE_API_PROPERTY("createSymbol", CreateSymbol),
@ -234,7 +275,7 @@ napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("decrementRefcount", DecrementRefcount),
DECLARE_NODE_API_GETTER("referenceValue", GetReferenceValue),
DECLARE_NODE_API_PROPERTY("validateDeleteBeforeFinalize",
ValidateDeleteBeforeFinalize),
ValidateDeleteBeforeFinalize),
};
NODE_API_CALL(env, napi_define_properties(

View File

@ -22,6 +22,17 @@ static void noopDeleter(napi_env env, void* data, void* finalize_hint) {
deleterCallCount++;
}
static void malignDeleter(napi_env env, void* data, void* finalize_hint) {
NODE_API_ASSERT_RETURN_VOID(env, data != NULL && strcmp(data, theText) == 0, "invalid data");
napi_ref finalizer_ref = (napi_ref)finalize_hint;
napi_value js_finalizer;
napi_value recv;
NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
}
static napi_value newBuffer(napi_env env, napi_callback_info info) {
napi_value theBuffer;
char* theCopy;
@ -107,6 +118,30 @@ static napi_value staticBuffer(napi_env env, napi_callback_info info) {
return theBuffer;
}
static napi_value malignFinalizerBuffer(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
NODE_API_ASSERT(env, argc == 1, "Wrong number of arguments");
napi_value finalizer = args[0];
napi_valuetype finalizer_valuetype;
NODE_API_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
NODE_API_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
napi_ref finalizer_ref;
NODE_API_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
napi_value theBuffer;
NODE_API_CALL(
env,
napi_create_external_buffer(env,
sizeof(theText),
(void*)theText,
malignDeleter,
finalizer_ref, // finalize_hint
&theBuffer));
return theBuffer;
}
static napi_value Init(napi_env env, napi_value exports) {
napi_value theValue;
@ -123,6 +158,7 @@ static napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("bufferHasInstance", bufferHasInstance),
DECLARE_NODE_API_PROPERTY("bufferInfo", bufferInfo),
DECLARE_NODE_API_PROPERTY("staticBuffer", staticBuffer),
DECLARE_NODE_API_PROPERTY("malignFinalizerBuffer", malignFinalizerBuffer),
};
NODE_API_CALL(env, napi_define_properties(

View File

@ -0,0 +1,21 @@
'use strict';
// Flags: --expose-gc --force-node-api-uncaught-exceptions-policy
const common = require('../../common');
const binding = require(`./build/${common.buildType}/test_buffer`);
const assert = require('assert');
const tick = require('util').promisify(require('../../common/tick'));
process.on('uncaughtException', common.mustCall((err) => {
assert.throws(() => { throw err; }, /finalizer error/);
}));
(async function() {
{
binding.malignFinalizerBuffer(common.mustCall(() => {
throw new Error('finalizer error');
}));
}
global.gc();
await tick(10);
})().then(common.mustCall());

View File

@ -269,6 +269,35 @@ static napi_value StartThreadNoJsFunc(napi_env env, napi_callback_info info) {
/** block_on_full */true, /** alt_ref_js_cb */true);
}
// Testing calling into JavaScript
static void ThreadSafeFunctionFinalize(napi_env env,
void* finalize_data,
void* finalize_hint) {
napi_ref js_func_ref = (napi_ref) finalize_data;
napi_value js_func;
napi_value recv;
NODE_API_CALL_RETURN_VOID(env, napi_get_reference_value(env, js_func_ref, &js_func));
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
NODE_API_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_func, 0, NULL, NULL));
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, js_func_ref));
}
// Testing calling into JavaScript
static napi_value CallIntoModule(napi_env env, napi_callback_info info) {
size_t argc = 4;
napi_value argv[4];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
napi_ref finalize_func;
NODE_API_CALL(env, napi_create_reference(env, argv[3], 1, &finalize_func));
napi_threadsafe_function tsfn;
NODE_API_CALL(env, napi_create_threadsafe_function(env, argv[0], argv[1], argv[2], 0, 1, finalize_func, ThreadSafeFunctionFinalize, NULL, NULL, &tsfn));
NODE_API_CALL(env, napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking));
NODE_API_CALL(env, napi_release_threadsafe_function(tsfn, napi_tsfn_release));
return NULL;
}
// Module init
static napi_value Init(napi_env env, napi_value exports) {
size_t index;
@ -307,6 +336,7 @@ static napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("StopThread", StopThread),
DECLARE_NODE_API_PROPERTY("Unref", Unref),
DECLARE_NODE_API_PROPERTY("Release", Release),
DECLARE_NODE_API_PROPERTY("CallIntoModule", CallIntoModule),
};
NODE_API_CALL(env, napi_define_properties(env, exports,

View File

@ -0,0 +1,22 @@
'use strict';
// Flags: --no-force-node-api-uncaught-exceptions-policy
const common = require('../../common');
const binding = require(`./build/${common.buildType}/binding`);
process.on(
'uncaughtException',
common.mustNotCall('uncaught callback errors should be suppressed ' +
'with the option --no-force-node-api-uncaught-exceptions-policy')
);
binding.CallIntoModule(
common.mustCall(() => {
throw new Error('callback error');
}),
{},
'resource_name',
common.mustCall(function finalizer() {
throw new Error('finalizer error');
})
);

View File

@ -0,0 +1,27 @@
'use strict';
// Flags: --force-node-api-uncaught-exceptions-policy
const common = require('../../common');
const assert = require('assert');
const binding = require(`./build/${common.buildType}/binding`);
const callbackCheck = common.mustCall((err) => {
assert.throws(() => { throw err; }, /callback error/);
process.removeListener('uncaughtException', callbackCheck);
process.on('uncaughtException', finalizerCheck);
});
const finalizerCheck = common.mustCall((err) => {
assert.throws(() => { throw err; }, /finalizer error/);
});
process.on('uncaughtException', callbackCheck);
binding.CallIntoModule(
common.mustCall(() => {
throw new Error('callback error');
}),
{},
'resource_name',
common.mustCall(function finalizer() {
throw new Error('finalizer error');
})
);