Skip to content

Commit

Permalink
node-api: emit uncaught-exception on unhandled tsfn callbacks
Browse files Browse the repository at this point in the history
PR-URL: #36510
Fixes: #36402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
legendecas authored and targos committed Jul 12, 2022
1 parent 417d019 commit 4a0cc80
Show file tree
Hide file tree
Showing 13 changed files with 304 additions and 46 deletions.
13 changes: 13 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,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
Expand Down Expand Up @@ -1576,6 +1588,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`
Expand Down
22 changes: 13 additions & 9 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down
101 changes: 65 additions & 36 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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();
}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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); });
}

Expand All @@ -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();
Expand Down Expand Up @@ -721,7 +757,7 @@ napi_status 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);
}
Expand Down Expand Up @@ -1061,16 +1097,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.
Expand Down
8 changes: 8 additions & 0 deletions src/node_api_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,26 @@
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());
}
inline const char* GetFilename() const { return filename.c_str(); }

std::string filename;
bool destructing = false;
};

using node_napi_env = node_napi_env__*;
Expand Down
6 changes: 6 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,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,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,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;
Expand Down
20 changes: 20 additions & 0 deletions test/js-native-api/test_reference/test_finalizer.js
Original file line number Diff line number Diff line change
@@ -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());
43 changes: 42 additions & 1 deletion test/js-native-api/test_reference/test_reference.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand All @@ -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(
Expand Down
Loading

0 comments on commit 4a0cc80

Please sign in to comment.