node-api: force env shutdown deferring behavior

The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.

Fixes: #37236
Co-authored-by: Chengzhong Wu <[email protected]>
PR-URL: #37303
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

Author: 2 people

src/js_native_api_v8.cc

@@ -270,8 +270,17 @@ class RefBase : protected Finalizer, RefTracker {
 
  protected:
   inline void Finalize(bool is_env_teardown = false) override {
+    // During environment teardown we have to convert a strong reference to
+    // a weak reference to force the deferring behavior if the user's finalizer
+    // happens to delete this reference so that the code in this function that
+    // follows the call to the user's finalizer may safely access variables from
+    // this instance.
+    if (is_env_teardown && RefCount() > 0) _refcount = 0;
+
     if (_finalize_callback != nullptr) {
       _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
+      // This ensures that we never call the finalizer twice.
+      _finalize_callback = nullptr;
     }
 
     // this is safe because if a request to delete the reference

test/js-native-api/test_reference_double_free/binding.gyp

@@ -0,0 +1,11 @@
+{
+  "targets": [
+    {
+      "target_name": "test_reference_double_free",
+      "sources": [
+        "../entry_point.c",
+        "test_reference_double_free.c"
+      ]
+    }
+  ]
+}

test/js-native-api/test_reference_double_free/test.js

@@ -0,0 +1,11 @@
+'use strict';
+
+// This test makes no assertions. It tests a fix without which it will crash
+// with a double free.
+
+const { buildType } = require('../../common');
+
+const addon = require(`./build/${buildType}/test_reference_double_free`);
+
+{ new addon.MyObject(true); }
+{ new addon.MyObject(false); }

test/js-native-api/test_reference_double_free/test_reference_double_free.c

@@ -0,0 +1,55 @@
+#include <stdlib.h>
+#include <js_native_api.h>
+#include "../common.h"
+
+static size_t g_call_count = 0;
+
+static void Destructor(napi_env env, void* data, void* nothing) {
+  napi_ref* ref = data;
+  NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
+  free(ref);
+}
+
+static void NoDeleteDestructor(napi_env env, void* data, void* hint) {
+  napi_ref* ref = data;
+  size_t* call_count = hint;
+
+  // This destructor must be called exactly once.
+  if ((*call_count) > 0) abort();
+  *call_count = ((*call_count) + 1);
+  free(ref);
+}
+
+static napi_value New(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value js_this, js_delete;
+  bool delete;
+  napi_ref* ref = malloc(sizeof(*ref));
+
+  NODE_API_CALL(env,
+      napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL));
+  NODE_API_CALL(env, napi_get_value_bool(env, js_delete, &delete));
+
+  if (delete) {
+    NODE_API_CALL(env,
+        napi_wrap(env, js_this, ref, Destructor, NULL, ref));
+  } else {
+    NODE_API_CALL(env,
+        napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref));
+  }
+  NODE_API_CALL(env, napi_reference_ref(env, *ref, NULL));
+
+  return js_this;
+}
+
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
+  napi_value myobj_ctor;
+  NODE_API_CALL(env,
+      napi_define_class(
+          env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor));
+  NODE_API_CALL(env,
+      napi_set_named_property(env, exports, "MyObject", myobj_ctor));
+  return exports;
+}
+EXTERN_C_END