Crash on node api add-on finalization

Version: v10.23.2, v12.20.1, v14.15.4, v15.8.0 (all latest lts and current version)
Platform: all
Subsystem: node-api

What steps will reproduce the bug?

Repo to re-produce: https://github.com/legendecas/repro-napi-v8impl-refbase-double-free

$ make
v14.15.4
force gc
fish: 'node --expose_gc index.js' terminated by signal SIGSEGV (Address boundary error)

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

No segment faults.

What do you see instead?

Segment faults on double free of v8impl::<anonymous>::RefBase. The RefBases were deleted once one module's napi_env was going to destroy, and the installed weak v8impl::Persistents of v8impl::<anonymous>Reference was not destroyed and these RefBase will be deleted again on finalization callbacks.

@legendecas

@legendecas the assumption is that once the environment begins the process of being torn down, no more gc runs will happen. This test AFAICT forces gc runs after napi_env teardown, so it is entirely possible that finalizers will be called after napi_env's FinalizeAll. TBH the scenario created in the test seems unlikely to happen under normal circumstances. OTOH, it would of course be ideal to handle even unusual circumstances. Nevertheless, please be careful to not re-introduce the problem fixed in c822ba7!

Is this related: #36868?

@gabrielschulhof

@gabrielschulhof we just identified the problem in random CI failures on our system. The force GC in the repro is to ensure the reproduce is reliable to show how the problem happens in the case. The crashes do happens in a low rate, but I have to say it's not unlikely to happen: in our internal tests the ratio is roughly 1/6 to crash on exit.

Is this related: #36868?

Yes, the crash call stacks seem very similar to the case. I strongly believe it's the same problem.

@legendecas

@legendecas I ran the repro and it dies reliably even without --expose-gc.

@legendecas

@legendecas

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index e037c4297d..22932dc6b9 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -199,7 +199,8 @@ class RefBase : protected Finalizer, RefTracker {
           void* finalize_hint)
        : Finalizer(env, finalize_callback, finalize_data, finalize_hint),
         _refcount(initial_refcount),
-        _delete_self(delete_self) {
+        _delete_self(delete_self),
+        _is_gone(nullptr) {
     Link(finalize_callback == nullptr
         ? &env->reflist
         : &env->finalizing_reflist);
@@ -220,7 +221,10 @@ class RefBase : protected Finalizer, RefTracker {
                        finalize_hint);
   }
 
-  virtual ~RefBase() { Unlink(); }
+  virtual ~RefBase() {
+    if (_is_gone != nullptr) *_is_gone = true;
+    Unlink();
+  }
 
   inline void* Data() {
     return _finalize_data;
@@ -270,10 +274,14 @@ class RefBase : protected Finalizer, RefTracker {
 
  protected:
   inline void Finalize(bool is_env_teardown = false) override {
+    bool reference_is_gone = false;
+    _is_gone = &reference_is_gone;
     if (_finalize_callback != nullptr) {
       _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
     }
 
+    if (reference_is_gone) return;
+
     // 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
@@ -287,6 +295,7 @@ class RefBase : protected Finalizer, RefTracker {
  private:
   uint32_t _refcount;
   bool _delete_self;
+  bool* _is_gone;
 };
 
 class Reference : public RefBase {

seems to avert the problem without breaking the tests.

@legendecas

@legendecas in your repro you delete wrapper_ in two places:

In the instance data finalizer:
https://github.com/legendecas/repro-napi-v8impl-refbase-double-free/blob/5b0ae1874b9b26fb0fe453565484a004801c70e4/test.cc#L56
In the napi_wrap finalizer (which calls ~MyObject)
https://github.com/legendecas/repro-napi-v8impl-refbase-double-free/blob/5b0ae1874b9b26fb0fe453565484a004801c70e4/test.cc#L27

So, it looks like the double free is not really caused by anything Node-API does wrong.

NM. Looks like it crashes even if I remove the napi_delete_reference from the instance finalizer.

@legendecas

@legendecas I have been able to reduce the test case to legendecas/repro-napi-v8impl-refbase-double-free#1 while still retaining the crash.

I think the key is the napi_reference_ref it performs on the reference returned from napi_wrap.

@gabrielschulhof

@gabrielschulhof Yeah, you're right. I found I've been mistaken on the repro. In the reproduction, there are strong references to the object, this is not the case I was intended to show in the first place. After reading #37303 and tried the PR on my internal case and it does crash regardlessly, I discovered that my original crash on double free of v8impl::reference is not caused by strong references but weak references. These references were set weak before the process going to exit - there is a uv ref on the corresponding handle so only after the set weak the process is going to exit. I'll double-check the re-production to make a reliable reproduction on weak references.