Skip to content

Commit

Permalink
n-api: free instance data as reference
Browse files Browse the repository at this point in the history 
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
PR-URL: #31638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
  • Loading branch information
@codebytere
Gabriel Schulhof authored and codebytere committed Mar 30, 2020
1 parent 8f75c74 commit 08e09ec
Show file tree
Hide file tree
Showing 8 changed files with 232 additions and 111 deletions.
169 changes: 108 additions & 61 deletions src/js_native_api_v8.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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) ||
Expand All @@ -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(
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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();
Expand Down Expand Up @@ -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<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);
}
Expand All @@ -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<v8impl::RefBase*>(env->instance_data);

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

return napi_clear_last_error(env);
}
Expand Down
54 changes: 44 additions & 10 deletions src/js_native_api_v8.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Context> context)
: isolate(context->GetIsolate()),
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
39 changes: 0 additions & 39 deletions src/js_native_api_v8_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand Down
23 changes: 23 additions & 0 deletions test/node-api/test_instance_data/addon.c
View file
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

0 comments on commit 08e09ec

Please sign in to comment.