From 65fce2be5cb1b766acc97a623760f9a54f3dae76 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 14 Oct 2022 06:58:46 +0000 Subject: [PATCH] fixup! verify dangling napi_ref in napi_wrap --- src/js_native_api_v8.cc | 72 ++++++++++++------- src/js_native_api_v8.h | 22 ++++-- .../6_object_wrap/6_object_wrap.cc | 63 +++++++++++++++- .../6_object_wrap/test-object-wrap-ref.js | 13 ++++ 4 files changed, 141 insertions(+), 29 deletions(-) create mode 100644 test/js-native-api/6_object_wrap/test-object-wrap-ref.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index be7b9083c5ffcf..06473c8ec93cca 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -218,7 +218,12 @@ inline napi_status Unwrap(napi_env env, if (action == RemoveWrap) { CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper)) .FromJust()); - delete reference; + if (reference->ownership() == Ownership::kUserland) { + // When the wrap is been removed, the finalizer should be reset. + reference->ResetFinalizer(); + } else { + delete reference; + } } return GET_RETURN_STATUS(env); @@ -245,7 +250,8 @@ class CallbackBundle { bundle->env = env; v8::Local cbdata = v8::External::New(env->isolate, bundle); - Reference::New(env, cbdata, 0, true, Delete, bundle, nullptr); + Reference::New( + env, cbdata, 0, Ownership::kRuntime, Delete, bundle, nullptr); return cbdata; } napi_env env; // Necessary to invoke C++ NAPI callback @@ -429,8 +435,13 @@ inline napi_status Wrap(napi_env env, // before then, then the finalize callback will never be invoked.) // Therefore a finalize callback is required when returning a reference. CHECK_ARG(env, finalize_cb); - reference = v8impl::Reference::New( - env, obj, 0, false, finalize_cb, native_object, finalize_hint); + reference = v8impl::Reference::New(env, + obj, + 0, + v8impl::Ownership::kUserland, + finalize_cb, + native_object, + finalize_hint); *result = reinterpret_cast(reference); } else { // Create a self-deleting reference. @@ -438,7 +449,7 @@ inline napi_status Wrap(napi_env env, env, obj, 0, - true, + v8impl::Ownership::kRuntime, finalize_cb, native_object, finalize_cb == nullptr ? nullptr : finalize_hint); @@ -456,16 +467,22 @@ inline napi_status Wrap(napi_env env, } // end of anonymous namespace +void Finalizer::ResetFinalizer() { + finalize_callback_ = nullptr; + finalize_data_ = nullptr; + finalize_hint_ = nullptr; +} + // Wrapper around v8impl::Persistent that implements reference counting. RefBase::RefBase(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint), refcount_(initial_refcount), - delete_self_(delete_self) { + ownership_(ownership) { Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); } @@ -480,13 +497,13 @@ RefBase::~RefBase() { RefBase* RefBase::New(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) { return new RefBase(env, initial_refcount, - delete_self, + ownership, finalize_callback, finalize_data, finalize_hint); @@ -512,27 +529,29 @@ uint32_t RefBase::RefCount() { } void RefBase::Finalize() { - bool delete_self = delete_self_; + Ownership ownership = ownership_; // Swap out the field finalize_callback so that it can not be accidentally // called more than once. napi_finalize finalize_callback = finalize_callback_; - finalize_callback_ = nullptr; + void* finalize_data = finalize_data_; + void* finalize_hint = finalize_hint_; + ResetFinalizer(); // Either the RefBase is going to be deleted in the finalize_callback or not, // it should be removed from the tracked list. Unlink(); // 1. If the finalize_callback is present, it should either delete the - // RefBase, or set the flag `delete_self`. + // RefBase, or set ownership with Ownership::kRuntime. // 2. If the finalizer is not present, the RefBase can be deleted after the // call. if (finalize_callback != nullptr) { - env_->CallFinalizer(finalize_callback, finalize_data_, finalize_hint_); + env_->CallFinalizer(finalize_callback, finalize_data, finalize_hint); // No access to `this` after finalize_callback is called. } - // If the RefBase is not self-destructive, userland code should delete it. - // Now delete it if it is self-destructive. - if (delete_self) { + // If the RefBase is not Ownership::kRuntime, userland code should delete it. + // Now delete it if it is Ownership::kRuntime. + if (ownership == Ownership::kRuntime) { delete this; } } @@ -554,14 +573,14 @@ Reference::~Reference() { Reference* Reference::New(napi_env env, v8::Local value, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint) { return new Reference(env, value, initial_refcount, - delete_self, + ownership, finalize_callback, finalize_data, finalize_hint); @@ -2297,8 +2316,13 @@ napi_status NAPI_CDECL napi_create_external(napi_env env, if (finalize_cb) { // The Reference object will delete itself after invoking the finalizer // callback. - v8impl::Reference::New( - env, external_value, 0, true, finalize_cb, data, finalize_hint); + v8impl::Reference::New(env, + external_value, + 0, + v8impl::Ownership::kRuntime, + finalize_cb, + data, + finalize_hint); } *result = v8impl::JsValueFromV8LocalValue(external_value); @@ -2407,8 +2431,8 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, return napi_set_last_error(env, napi_invalid_arg); } - v8impl::Reference* reference = - v8impl::Reference::New(env, v8_value, initial_refcount, false); + v8impl::Reference* reference = v8impl::Reference::New( + env, v8_value, initial_refcount, v8impl::Ownership::kUserland); *result = reinterpret_cast(reference); return napi_clear_last_error(env); @@ -3115,8 +3139,8 @@ napi_status NAPI_CDECL napi_set_instance_data(napi_env env, delete old_data; } - env->instance_data = - v8impl::RefBase::New(env, 0, true, finalize_cb, data, finalize_hint); + env->instance_data = v8impl::RefBase::New( + env, 0, v8impl::Ownership::kRuntime, finalize_cb, data, finalize_hint); return napi_clear_last_error(env); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index f9aa1748bc0cad..b3d0446cb5239f 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -316,6 +316,8 @@ class Finalizer { void* data() { return finalize_data_; } void* hint() { return finalize_hint_; } + void ResetFinalizer(); + protected: napi_env env_; napi_finalize finalize_callback_; @@ -337,12 +339,22 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; +// Ownership of a reference. +enum class Ownership { + // The reference is owned by the runtime. No userland call is needed to + // destruct the reference. + kRuntime, + // The reference is owned by the userland. User code is responsible to delete + // the reference with appropriate node-api calls. + kUserland, +}; + // Wrapper around Finalizer that implements reference counting. class RefBase : public Finalizer, public RefTracker { protected: RefBase(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint); @@ -350,7 +362,7 @@ class RefBase : public Finalizer, public RefTracker { public: static RefBase* New(napi_env env, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback, void* finalize_data, void* finalize_hint); @@ -361,12 +373,14 @@ class RefBase : public Finalizer, public RefTracker { uint32_t Unref(); uint32_t RefCount(); + Ownership ownership() { return ownership_; } + protected: void Finalize() override; private: uint32_t refcount_; - bool delete_self_; + Ownership ownership_; }; // Wrapper around v8impl::Persistent. @@ -379,7 +393,7 @@ class Reference : public RefBase { static Reference* New(napi_env env, v8::Local value, uint32_t initial_refcount, - bool delete_self, + Ownership ownership, napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, void* finalize_hint = nullptr); diff --git a/test/js-native-api/6_object_wrap/6_object_wrap.cc b/test/js-native-api/6_object_wrap/6_object_wrap.cc index 7f8fdd341673e6..7ebe711a6dccf1 100644 --- a/test/js-native-api/6_object_wrap/6_object_wrap.cc +++ b/test/js-native-api/6_object_wrap/6_object_wrap.cc @@ -1,5 +1,6 @@ -#include "myobject.h" #include "../common.h" +#include "assert.h" +#include "myobject.h" napi_ref MyObject::constructor; @@ -151,9 +152,69 @@ napi_value MyObject::Multiply(napi_env env, napi_callback_info info) { return instance; } +// This finalizer should never be invoked. +void ObjectWrapDanglingReferenceFinalizer(napi_env env, + void* finalize_data, + void* finalize_hint) { + assert(0 && "unreachable"); +} + +napi_ref dangling_ref; +napi_value ObjectWrapDanglingReference(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, nullptr, nullptr)); + + // Create a napi_wrap and remove it immediately, whilst leaving the out-param + // ref dangling (not deleted). + NODE_API_CALL(env, + napi_wrap(env, + args[0], + nullptr, + ObjectWrapDanglingReferenceFinalizer, + nullptr, + &dangling_ref)); + NODE_API_CALL(env, napi_remove_wrap(env, args[0], nullptr)); + + return args[0]; +} + +napi_value ObjectWrapDanglingReferenceTest(napi_env env, + napi_callback_info info) { + napi_value out; + napi_value ret; + NODE_API_CALL(env, napi_get_reference_value(env, dangling_ref, &out)); + + if (out == nullptr) { + // If the napi_ref has been invalidated, delete it. + NODE_API_CALL(env, napi_delete_reference(env, dangling_ref)); + NODE_API_CALL(env, napi_get_boolean(env, true, &ret)); + } else { + // The dangling napi_ref is still valid. + NODE_API_CALL(env, napi_get_boolean(env, false, &ret)); + } + return ret; +} + EXTERN_C_START napi_value Init(napi_env env, napi_value exports) { MyObject::Init(env, exports); + + napi_property_descriptor descriptors[] = { + DECLARE_NODE_API_PROPERTY("objectWrapDanglingReference", + ObjectWrapDanglingReference), + DECLARE_NODE_API_PROPERTY("objectWrapDanglingReferenceTest", + ObjectWrapDanglingReferenceTest), + }; + + NODE_API_CALL( + env, + napi_define_properties(env, + exports, + sizeof(descriptors) / sizeof(*descriptors), + descriptors)); + return exports; } EXTERN_C_END diff --git a/test/js-native-api/6_object_wrap/test-object-wrap-ref.js b/test/js-native-api/6_object_wrap/test-object-wrap-ref.js new file mode 100644 index 00000000000000..81832c195c1890 --- /dev/null +++ b/test/js-native-api/6_object_wrap/test-object-wrap-ref.js @@ -0,0 +1,13 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../../common'); +const addon = require(`./build/${common.buildType}/6_object_wrap`); + +(function scope() { + addon.objectWrapDanglingReference({}); +})(); + +common.gcUntil('object-wrap-ref', () => { + return addon.objectWrapDanglingReferenceTest(); +});