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
PR-URL: nodejs#31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>

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

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<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) {

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 <typename T>
 using Persistent = v8::Global<T>;
 

test/node-api/test_instance_data/addon.c

@@ -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;
+}