Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: free instance data as reference #31638

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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: nodejs/node-addon-api#663
  • Loading branch information
Gabriel Schulhof committed Feb 6, 2020
commit 38b4b6f9c8268870d50903e283cc61cadfb19640
169 changes: 108 additions & 61 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
@@ -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<v8::Value> 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<v8::Value> 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<v8::Value> Get() {
if (_persistent.IsEmpty()) {
return v8::Local<v8::Value>();
} else {
return v8::Local<v8::Value>::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 <typename... Args>
Reference(napi_env env,
v8::Local<v8::Value> value,
Args&&... args)
: RefBase(env, std::forward<Args>(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<v8::Value> 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<v8::Value> Get() {
if (_persistent.IsEmpty()) {
return v8::Local<v8::Value>();
} else {
return v8::Local<v8::Value>::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<v8::Value> _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<v8::Value> ab = Get();
@@ -3111,9 +3146,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<v8impl::RefBase*>(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);
}
@@ -3123,7 +3168,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<v8impl::RefBase*>(env->instance_data);

*data = (idata == nullptr ? nullptr : idata->Data());

return napi_clear_last_error(env);
}
54 changes: 44 additions & 10 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can replace this entire custom linked list implementation by ListHead/ListNode from util.h? That works pretty well for other parts of the codebase :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #31639 for this.


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<v8::Context> 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<v8::Context> 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) {
39 changes: 0 additions & 39 deletions src/js_native_api_v8_internals.h
Original file line number Diff line number Diff line change
@@ -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 <typename T>
using Persistent = v8::Global<T>;

23 changes: 23 additions & 0 deletions test/node-api/test_instance_data/addon.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include <stdio.h>
#include <stdlib.h>
#define NAPI_EXPERIMENTAL
#include <node_api.h>

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;
}
Loading