From 08e09eca3407747b64716f4152d5f4943c75157c Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 3 Feb 2020 20:46:37 -0800 Subject: [PATCH] n-api: free instance data as reference Instance data associated with a `napi_env` is no longer stored on the env itself but is instead rendered as a reference. Since `v8impl::Reference` is tied to a JS object, this modification factors out the `v8impl::Reference` refcounting and the deletion process into a base class for `v8impl::Reference`, called `v8impl::RefBase`. The instance data is then stored as a `v8impl::RefBase`, along with other references, preventing a segfault that arises from the fact that, up until now, upon `napi_env` destruction, the instance data was freed after all references had already been forcefully freed. If the addon freed a reference during the `napi_set_instance_data` finalizer callback, such a reference had already been freed during environment teardown, causing a double free. Re: https://github.com/nodejs/node-addon-api/pull/663 PR-URL: https://github.com/nodejs/node/pull/31638 Reviewed-By: Anna Henningsen Reviewed-By: David Carlier --- src/js_native_api_v8.cc | 169 +++++++++++------- src/js_native_api_v8.h | 54 ++++-- src/js_native_api_v8_internals.h | 39 ---- test/node-api/test_instance_data/addon.c | 23 +++ test/node-api/test_instance_data/binding.gyp | 16 +- test/node-api/test_instance_data/test.js | 20 +++ .../test_instance_data/test_ref_then_set.c | 11 ++ .../test_instance_data/test_set_then_ref.c | 11 ++ 8 files changed, 232 insertions(+), 111 deletions(-) create mode 100644 test/node-api/test_instance_data/addon.c create mode 100644 test/node-api/test_instance_data/test_ref_then_set.c create mode 100644 test/node-api/test_instance_data/test_set_then_ref.c diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 1889a22d28d8a8..d26b28dd0df4e8 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -186,49 +186,41 @@ inline static napi_status ConcludeDeferred(napi_env env, } // Wrapper around v8impl::Persistent that implements reference counting. -class Reference : protected Finalizer, RefTracker { +class RefBase : protected Finalizer, RefTracker { protected: - Reference(napi_env env, - v8::Local value, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) + RefBase(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - _persistent(env->isolate, value), _refcount(initial_refcount), _delete_self(delete_self) { - if (initial_refcount == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); - } Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); } public: - void* Data() { + static RefBase* New(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new RefBase(env, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); + } + + inline void* Data() { return _finalize_data; } - static Reference* New(napi_env env, - v8::Local value, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr) { - return new Reference(env, - value, - initial_refcount, - delete_self, - finalize_callback, - finalize_data, - finalize_hint); - } - // Delete is called in 2 ways. Either from the finalizer or // from one of Unwrap or napi_delete_reference. // @@ -244,7 +236,7 @@ class Reference : protected Finalizer, RefTracker { // The second way this is called is from // the finalizer and _delete_self is set. In this case we // know we need to do the deletion so just do it. - static void Delete(Reference* reference) { + static inline void Delete(RefBase* reference) { reference->Unlink(); if ((reference->RefCount() != 0) || (reference->_delete_self) || @@ -257,40 +249,23 @@ class Reference : protected Finalizer, RefTracker { } } - uint32_t Ref() { - if (++_refcount == 1) { - _persistent.ClearWeak(); - } - - return _refcount; + inline uint32_t Ref() { + return ++_refcount; } - uint32_t Unref() { + inline uint32_t Unref() { if (_refcount == 0) { return 0; } - if (--_refcount == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); - } - - return _refcount; + return --_refcount; } - uint32_t RefCount() { + inline uint32_t RefCount() { return _refcount; } - v8::Local Get() { - if (_persistent.IsEmpty()) { - return v8::Local(); - } else { - return v8::Local::New(_env->isolate, _persistent); - } - } - protected: - void Finalize(bool is_env_teardown = false) override { + inline void Finalize(bool is_env_teardown = false) override { if (_finalize_callback != nullptr) { _env->CallIntoModuleThrow([&](napi_env env) { _finalize_callback( @@ -310,6 +285,68 @@ class Reference : protected Finalizer, RefTracker { } } + private: + uint32_t _refcount; + bool _delete_self; +}; + +class Reference : public RefBase { + protected: + template + Reference(napi_env env, + v8::Local value, + Args&&... args) + : RefBase(env, std::forward(args)...), + _persistent(env->isolate, value) { + if (RefCount() == 0) { + _persistent.SetWeak( + this, FinalizeCallback, v8::WeakCallbackType::kParameter); + } + } + + public: + static inline Reference* New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback = nullptr, + void* finalize_data = nullptr, + void* finalize_hint = nullptr) { + return new Reference(env, + value, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); + } + + inline uint32_t Ref() { + uint32_t refcount = RefBase::Ref(); + if (refcount == 1) { + _persistent.ClearWeak(); + } + return refcount; + } + + inline uint32_t Unref() { + uint32_t old_refcount = RefCount(); + uint32_t refcount = RefBase::Unref(); + if (old_refcount == 1 && refcount == 0) { + _persistent.SetWeak( + this, FinalizeCallback, v8::WeakCallbackType::kParameter); + } + return refcount; + } + + inline v8::Local Get() { + if (_persistent.IsEmpty()) { + return v8::Local(); + } else { + return v8::Local::New(_env->isolate, _persistent); + } + } + private: // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does @@ -332,8 +369,6 @@ class Reference : protected Finalizer, RefTracker { } v8impl::Persistent _persistent; - uint32_t _refcount; - bool _delete_self; }; class ArrayBufferReference final : public Reference { @@ -354,7 +389,7 @@ class ArrayBufferReference final : public Reference { } private: - void Finalize(bool is_env_teardown) override { + inline void Finalize(bool is_env_teardown) override { if (is_env_teardown) { v8::HandleScope handle_scope(_env->isolate); v8::Local ab = Get(); @@ -3023,9 +3058,19 @@ napi_status napi_set_instance_data(napi_env env, void* finalize_hint) { CHECK_ENV(env); - env->instance_data.data = data; - env->instance_data.finalize_cb = finalize_cb; - env->instance_data.hint = finalize_hint; + v8impl::RefBase* old_data = static_cast(env->instance_data); + if (old_data != nullptr) { + // Our contract so far has been to not finalize any old data there may be. + // So we simply delete it. + v8impl::RefBase::Delete(old_data); + } + + env->instance_data = v8impl::RefBase::New(env, + 0, + true, + finalize_cb, + data, + finalize_hint); return napi_clear_last_error(env); } @@ -3035,7 +3080,9 @@ napi_status napi_get_instance_data(napi_env env, CHECK_ENV(env); CHECK_ARG(env, data); - *data = env->instance_data.data; + v8impl::RefBase* idata = static_cast(env->instance_data); + + *data = (idata == nullptr ? nullptr : idata->Data()); return napi_clear_last_error(env); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 0d5424cccaddf0..ec397275164c8d 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -8,6 +8,49 @@ static napi_status napi_clear_last_error(napi_env env); +namespace v8impl { + +class RefTracker { + public: + RefTracker() {} + virtual ~RefTracker() {} + virtual void Finalize(bool isEnvTeardown) {} + + typedef RefTracker RefList; + + inline void Link(RefList* list) { + prev_ = list; + next_ = list->next_; + if (next_ != nullptr) { + next_->prev_ = this; + } + list->next_ = this; + } + + inline void Unlink() { + if (prev_ != nullptr) { + prev_->next_ = next_; + } + if (next_ != nullptr) { + next_->prev_ = prev_; + } + prev_ = nullptr; + next_ = nullptr; + } + + static void FinalizeAll(RefList* list) { + while (list->next_ != nullptr) { + list->next_->Finalize(true); + } + } + + private: + RefList* next_ = nullptr; + RefList* prev_ = nullptr; +}; + +} // end of namespace v8impl + struct napi_env__ { explicit napi_env__(v8::Local context) : isolate(context->GetIsolate()), @@ -22,11 +65,6 @@ struct napi_env__ { // `napi_finalizer` deleted them subsequently. v8impl::RefTracker::FinalizeAll(&finalizing_reflist); v8impl::RefTracker::FinalizeAll(&reflist); - if (instance_data.finalize_cb != nullptr) { - CallIntoModuleThrow([&](napi_env env) { - instance_data.finalize_cb(env, instance_data.data, instance_data.hint); - }); - } } v8::Isolate* const isolate; // Shortcut for context()->GetIsolate() v8impl::Persistent context_persistent; @@ -76,11 +114,7 @@ struct napi_env__ { int open_handle_scopes = 0; int open_callback_scopes = 0; int refs = 1; - struct { - void* data = nullptr; - void* hint = nullptr; - napi_finalize finalize_cb = nullptr; - } instance_data; + void* instance_data = nullptr; }; static inline napi_status napi_clear_last_error(napi_env env) { diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index 74afd1172e527e..ddd219818cdfa9 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -28,45 +28,6 @@ namespace v8impl { -class RefTracker { - public: - RefTracker() {} - virtual ~RefTracker() {} - virtual void Finalize(bool isEnvTeardown) {} - - typedef RefTracker RefList; - - inline void Link(RefList* list) { - prev_ = list; - next_ = list->next_; - if (next_ != nullptr) { - next_->prev_ = this; - } - list->next_ = this; - } - - inline void Unlink() { - if (prev_ != nullptr) { - prev_->next_ = next_; - } - if (next_ != nullptr) { - next_->prev_ = prev_; - } - prev_ = nullptr; - next_ = nullptr; - } - - static void FinalizeAll(RefList* list) { - while (list->next_ != nullptr) { - list->next_->Finalize(true); - } - } - - private: - RefList* next_ = nullptr; - RefList* prev_ = nullptr; -}; - template using Persistent = v8::Global; diff --git a/test/node-api/test_instance_data/addon.c b/test/node-api/test_instance_data/addon.c new file mode 100644 index 00000000000000..928b4dfaf8e11e --- /dev/null +++ b/test/node-api/test_instance_data/addon.c @@ -0,0 +1,23 @@ +#include +#include +#define NAPI_EXPERIMENTAL +#include + +static void addon_free(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + napi_delete_reference(env, *ref); + free(ref); + fprintf(stderr, "addon_free"); +} + +napi_value addon_new(napi_env env, napi_value exports, bool ref_first) { + napi_ref* ref = malloc(sizeof(*ref)); + if (ref_first) { + napi_create_reference(env, exports, 1, ref); + napi_set_instance_data(env, ref, addon_free, NULL); + } else { + napi_set_instance_data(env, ref, addon_free, NULL); + napi_create_reference(env, exports, 1, ref); + } + return exports; +} diff --git a/test/node-api/test_instance_data/binding.gyp b/test/node-api/test_instance_data/binding.gyp index 0d55905e9e7236..eb20531269e0f8 100644 --- a/test/node-api/test_instance_data/binding.gyp +++ b/test/node-api/test_instance_data/binding.gyp @@ -5,6 +5,20 @@ "sources": [ "test_instance_data.c" ] - } + }, + { + "target_name": "test_set_then_ref", + "sources": [ + "addon.c", + "test_set_then_ref.c", + ] + }, + { + "target_name": "test_ref_then_set", + "sources": [ + "addon.c", + "test_ref_then_set.c", + ] + }, ] } diff --git a/test/node-api/test_instance_data/test.js b/test/node-api/test_instance_data/test.js index 969c164afdad58..7dc3f4b3176877 100644 --- a/test/node-api/test_instance_data/test.js +++ b/test/node-api/test_instance_data/test.js @@ -24,12 +24,32 @@ if (module.parent) { } else { // When launched as a script, run tests in either a child process or in a // worker thread. + const assert = require('assert'); const requireAs = require('../../common/require-as'); const runOptions = { stdio: ['inherit', 'pipe', 'inherit'] }; + const { spawnSync } = require('child_process'); // Run tests in a child process. requireAs(__filename, ['--expose-gc'], runOptions, 'child'); // Run tests in a worker thread in a child process. requireAs(__filename, ['--expose-gc'], runOptions, 'worker'); + + function testProcessExit(addonName) { + // Make sure that process exit is clean when the instance data has + // references to JS objects. + const path = require + .resolve(`./build/${common.buildType}/${addonName}`) + // Replace any backslashes with double backslashes because they'll be re- + // interpreted back to single backslashes in the command line argument + // to the child process. Windows needs this. + .replace(/\\/g, '\\\\'); + const child = spawnSync(process.execPath, ['-e', `require('${path}');`]); + assert.strictEqual(child.signal, null); + assert.strictEqual(child.status, 0); + assert.strictEqual(child.stderr.toString(), 'addon_free'); + } + + testProcessExit('test_ref_then_set'); + testProcessExit('test_set_then_ref'); } diff --git a/test/node-api/test_instance_data/test_ref_then_set.c b/test/node-api/test_instance_data/test_ref_then_set.c new file mode 100644 index 00000000000000..a0df1e5b9f8a64 --- /dev/null +++ b/test/node-api/test_instance_data/test_ref_then_set.c @@ -0,0 +1,11 @@ +#include +#include +#define NAPI_EXPERIMENTAL +#include + +napi_value addon_new(napi_env env, napi_value exports, bool ref_first); + +// static napi_value +NAPI_MODULE_INIT(/*napi_env env, napi_value exports */) { + return addon_new(env, exports, true); +} diff --git a/test/node-api/test_instance_data/test_set_then_ref.c b/test/node-api/test_instance_data/test_set_then_ref.c new file mode 100644 index 00000000000000..6ebed2d1e86871 --- /dev/null +++ b/test/node-api/test_instance_data/test_set_then_ref.c @@ -0,0 +1,11 @@ +#include +#include +#define NAPI_EXPERIMENTAL +#include + +napi_value addon_new(napi_env env, napi_value exports, bool ref_first); + +// static napi_value +NAPI_MODULE_INIT(/*napi_env env, napi_value exports */) { + return addon_new(env, exports, false); +}