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 <legendecas@gmail.com>
PR-URL: #37303
Backport-PR-URL: #42512
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>

Author: 2 people

src/js_native_api_v8.cc

@@ -269,8 +269,17 @@ class RefBase : protected Finalizer, RefTracker {
 
 protected:
 inlinevoidFinalize(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');
+
+constaddon=require(`./build/${buildType}/test_reference_double_free`);
+
+{newaddon.MyObject(true);}
+{newaddon.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"
+
+staticsize_tg_call_count=0;
+
+staticvoidDestructor(napi_envenv, void*data, void*nothing) {
+napi_ref*ref=data;
+NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
+free(ref);
+}
+
+staticvoidNoDeleteDestructor(napi_envenv, 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);
+}
+
+staticnapi_valueNew(napi_envenv, napi_callback_infoinfo) {
+size_targc=1;
+napi_valuejs_this, js_delete;
+booldelete;
+napi_ref*ref=malloc(sizeof(*ref));
+
+NAPI_CALL(env,
+napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL));
+NAPI_CALL(env, napi_get_value_bool(env, js_delete, &delete));
+
+if (delete) {
+NAPI_CALL(env,
+napi_wrap(env, js_this, ref, Destructor, NULL, ref));
+ } else {
+NAPI_CALL(env,
+napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref));
+ }
+NAPI_CALL(env, napi_reference_ref(env, *ref, NULL));
+
+returnjs_this;
+}
+
+EXTERN_C_START
+napi_valueInit(napi_envenv, napi_valueexports) {
+napi_valuemyobj_ctor;
+NAPI_CALL(env,
+napi_define_class(
+env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor));
+NAPI_CALL(env,
+napi_set_named_property(env, exports, "MyObject", myobj_ctor));
+returnexports;
+}
+EXTERN_C_END