n-api: handle reference delete before finalize

Crashes were reported during finalization due to the memory for a reference being deleted and the finalizer running after the deletion. This change ensures the deletion of the memory for the reference only occurs after the finalizer has run. Fixes: nodejs/node-addon-api#393 PR-URL: #24494 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
nodejs · Nov 16, 2018 · 0c51fc51b0074fa96be42dd1133fdb9a4c94147a · 0c51fc5
1 parent 2a67a49
commit 0c51fc51b0074fa96be42dd1133fdb9a4c94147a
Unified Split

Showing with 91 additions and 5 deletions.

+28 −5 src/js_native_api_v8.cc

+1 −0 src/js_native_api_v8.h

+26 −0 test/addons-napi/test_reference/test.js

+36 −0 test/addons-napi/test_reference/test_reference.c
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -227,8 +227,29 @@ class Reference : private Finalizer {
      finalize_hint);
  }
+  
+  // Delete is called in 2 ways. Either from the finalizer or
+  
+  // from one of Unwrap or napi_delete_reference.
+  
+  //
+  
+  // When it is called from Unwrap or napi_delete_reference we only
+  
+  // want to do the delete if the finalizer has already run,
+  
+  // otherwise we may crash when the finalizer does run.
+  
+  // If the finalizer has not already run delay the delete until
+  
+  // the finalizer runs by not doing the delete
+  
+  // and setting _delete_self to true so that the finalizer will
+  
+  // delete it when it runs.
+  
+  //
+  
+  // 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) {
-  
-    delete reference;
+  
+    if ((reference->_delete_self) || (reference->_finalize_ran)) {
+  
+      delete reference;
+  
+    } else {
+  
+      // reduce the reference count to 0 and defer until
+  
+      // finalizer runs
+  
+      reference->_delete_self = true;
+  
+      while (reference->Unref() != 0) {}
+  
+    }
  }
  uint32_t Ref() {
@@ -268,9 +289,6 @@ class Reference : private Finalizer {
    Reference* reference = data.GetParameter();
    reference->_persistent.Reset();
-  
-    // Check before calling the finalize callback, because the callback might
-  
-    // delete it.
-  
-    bool delete_self = reference->_delete_self;
    napi_env env = reference->_env;
    if (reference->_finalize_callback != nullptr) {
@@ -281,8 +299,13 @@ class Reference : private Finalizer {
            reference->_finalize_hint));
    }
-  
-    if (delete_self) {
+  
+    // this is safe because if a request to delete the reference
+  
+    // is made in the finalize_callback it will defer deletion
+  
+    // to this block and set _delete_self to true
+  
+    if (reference->_delete_self) {
      Delete(reference);
+  
+    } else {
+  
+      reference->_finalize_ran = true;
    }
  }

diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
@@ -180,6 +180,7 @@ class Finalizer {
  napi_finalize _finalize_callback;
  void* _finalize_data;
  void* _finalize_hint;
+  
+  bool _finalize_ran = false;
 };
 class TryCatch : public v8::TryCatch {

diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js
@@ -118,3 +118,29 @@ runTests(0, undefined, [
    assert.strictEqual(test_reference.finalizeCount, 1);
  },
 ]);
+  
+
+  
+// This test creates a napi_ref on an object that has
+  
+// been wrapped by napi_wrap and for which the finalizer
+  
+// for the wrap calls napi_delete_ref on that napi_ref.
+  
+//
+  
+// Since both the wrap and the reference use the same
+  
+// object the finalizer for the wrap and reference
+  
+// may run in the same gc and in any order.
+  
+//
+  
+// It does that to validate that napi_delete_ref can be
+  
+// called before the finalizer has been run for the
+  
+// reference (there is a finalizer behind the scenes even
+  
+// though it cannot be passed to napi_create_reference).
+  
+//
+  
+// Since the order is not guarranteed, run the
+  
+// test a number of times maximize the chance that we
+  
+// get a run with the desired order for the test.
+  
+//
+  
+// 1000 reliably recreated the problem without the fix
+  
+// required to ensure delete could be called before
+  
+// the finalizer in manual testing.
+  
+for (let i = 0; i < 1000; i++) {
+  
+  const wrapObject = new Object();
+  
+  test_reference.validateDeleteBeforeFinalize(wrapObject);
+  
+  global.gc();
+  
+}
diff --git a/test/addons-napi/test_reference/test_reference.c b/test/addons-napi/test_reference/test_reference.c
@@ -1,3 +1,4 @@
+  
+#include <stdlib.h>
 #include <node_api.h>
 #include "../common.h"
@@ -131,6 +132,39 @@ static napi_value GetReferenceValue(napi_env env, napi_callback_info info) {
  return result;
 }
+  
+static void DeleteBeforeFinalizeFinalizer(
+  
+    napi_env env, void* finalize_data, void* finalize_hint) {
+  
+  napi_ref* ref = (napi_ref*)finalize_data;
+  
+  napi_delete_reference(env, *ref);
+  
+  free(ref);
+  
+}
+  
+
+  
+static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info info) {
+  
+  napi_value wrapObject;
+  
+  size_t argc = 1;
+  
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapObject, NULL, NULL));
+  
+
+  
+  napi_ref* ref_t = malloc(sizeof(napi_ref));
+  
+  NAPI_CALL(env, napi_wrap(env,
+  
+			   wrapObject,
+  
+			   ref_t,
+  
+			   DeleteBeforeFinalizeFinalizer,
+  
+			   NULL,
+  
+			   NULL));
+  
+
+  
+  // Create a reference that will be eligible for collection at the same
+  
+  // time as the wrapped object by passing in the same wrapObject.
+  
+  // This means that the FinalizeOrderValidation callback may be run
+  
+  // before the finalizer for the newly created reference (there is a finalizer
+  
+  // behind the scenes even though it cannot be passed to napi_create_reference)
+  
+  // The Finalizer for the wrap (which is different than the finalizer
+  
+  // for the reference) calls napi_delete_reference validating that
+  
+  // napi_delete_reference can be called before the finalizer for the
+  
+  // reference runs.
+  
+  NAPI_CALL(env, napi_create_reference(env, wrapObject, 0, ref_t));
+  
+  return wrapObject;
+  
+}
+  
+
 static napi_value Init(napi_env env, napi_value exports) {
  napi_property_descriptor descriptors[] = {
    DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount),
@@ -143,6 +177,8 @@ static napi_value Init(napi_env env, napi_value exports) {
    DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount),
    DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount),
    DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue),
+  
+    DECLARE_NAPI_PROPERTY("validateDeleteBeforeFinalize",
+  
+		          ValidateDeleteBeforeFinalize),
  };
  NAPI_CALL(env, napi_define_properties(