[v14.x] Backport environment teardown Node-API reference double free fixes

src/js_native_api_v8.cc

@@ -270,8 +270,32 @@ class RefBase : protected Finalizer, RefTracker {
 
  protected:
   inline void Finalize(bool is_env_teardown = false) override {
+    // In addition to being called during environment teardown, this method is
+    // also the entry point for the garbage collector. During environment
+    // teardown we have to remove the garbage collector's reference to this
+    // method so that, if, as part of the user's callback, JS gets executed,
+    // resulting in a garbage collection pass, this method is not re-entered as
+    // part of that pass, because that'll cause a double free (as seen in
+    // https://github.com/nodejs/node/issues/37236).
+    //
+    // Since this class does not have access to the V8 persistent reference,
+    // this method is overridden in the `Reference` class below. Therein the
+    // weak callback is removed, ensuring that the garbage collector does not
+    // re-enter this method, and the method chains up to continue the process of
+    // environment-teardown-induced finalization.
+
+    // 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.
+      napi_finalize fini = _finalize_callback;
+      _finalize_callback = nullptr;
+      _env->CallFinalizer(fini, _finalize_data, _finalize_hint);
     }
 
     // this is safe because if a request to delete the reference
@@ -346,6 +370,20 @@ class Reference : public RefBase {
     }
   }
 
+ protected:
+  inline void Finalize(bool is_env_teardown = false) override {
+    // During env teardown, `~napi_env()` alone is responsible for finalizing.
+    // Thus, we don't want any stray gc passes to trigger a second call to
+    // `Finalize()`, so let's reset the persistent here if nothing is
+    // keeping it alive.
+    if (is_env_teardown && _persistent.IsWeak()) {
+      _persistent.ClearWeak();
+    }
+
+    // Chain up to perform the rest of the finalization.
+    RefBase::Finalize(is_env_teardown);
+  }
+
  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

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;
+  NAPI_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));
+
+  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));
+
+  return js_this;
+}
+
+EXTERN_C_START
+napi_value Init(napi_env env, napi_value exports) {
+  napi_value myobj_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));
+  return exports;
+}
+EXTERN_C_END

test/node-api/test_env_teardown_gc/binding.c

@@ -0,0 +1,37 @@
+#include <stdlib.h>
+#include <node_api.h>
+#include "../../js-native-api/common.h"
+
+static void MyObject_fini(napi_env env, void* data, void* hint) {
+  napi_ref* ref = data;
+  napi_value global;
+  napi_value cleanup;
+  NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global));
+  NAPI_CALL_RETURN_VOID(
+      env, napi_get_named_property(env, global, "cleanup", &cleanup));
+  napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL);
+  // We may not be allowed to call into JS, in which case a pending exception
+  // will be returned.
+  NAPI_ASSERT_RETURN_VOID(env,
+      status == napi_ok || status == napi_pending_exception,
+      "Unexpected status for napi_call_function");
+  NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
+  free(ref);
+}
+
+static napi_value MyObject(napi_env env, napi_callback_info info) {
+  napi_value js_this;
+  napi_ref* ref = malloc(sizeof(*ref));
+  NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL));
+  NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref));
+  return NULL;
+}
+
+NAPI_MODULE_INIT() {
+  napi_value ctor;
+  NAPI_CALL(
+      env, napi_define_class(
+          env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor));
+  NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject",  ctor));
+  return exports;
+}

test/node-api/test_env_teardown_gc/binding.gyp

@@ -0,0 +1,8 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.c' ]
+    }
+  ]
+}

test/node-api/test_env_teardown_gc/test.js

@@ -0,0 +1,14 @@
+'use strict';
+// Flags: --expose-gc
+
+process.env.NODE_TEST_KNOWN_GLOBALS = 0;
+
+const common = require('../../common');
+const binding = require(`./build/${common.buildType}/binding`);
+
+global.it = new binding.MyObject();
+
+global.cleanup = () => {
+  delete global.it;
+  global.gc();
+};