ObjectWrap destructor crashes node due to double napi delete calls

[melchor629]

Hi, using latest version from git, I'm getting crashes when node is closing about invalid access to memory or even double-free of the same pointer. I compiled node 13.7.0 with debug and asan to get a full report of the crash and that's the output: see gist.

For what I found running the debugger:

• When an object is freed in this scenario, the ObjectWrap destructor runs the napi_remove_wrap function. This function calls another inside the NAPI which does not remove the reference yet, but sets a variable to true (see v8impl::Reference::Delete).
• Then, as ObjectWrap inherits from Reference, the Reference destructor calls the function napi_delete_reference, in which the NAPI internally runs again the same Delete function from before but this time deleting the object (see code).
• All of above is running because node is running the finalizer for the wrap (I guess) here.
• After running the finalizer, then it tries to continue doing its job to see if v8impl::Reference::Delete should be called or not, but as the reference (this) itself has been deleted already, asan complains and shows the above report (see code).

I replicated the crash under 12.14.1 and 13.7.0 but not on 10.18.1 using the latest master code of this package (commit 4648420) all on macOS. The crash also replicates for any of the objects I create from JS which is a ObjectWrap.

While doing some changes to try to avoid the crash, I come up with adding this->SuppressDestruct(); when the napi_remove_wrap is called in the ObjectWrap destructor:

template <typename T>
inline ObjectWrap<T>::~ObjectWrap() {
  // If the JS object still exists at this point, remove the finalizer added
  // through `napi_wrap()`.
  if (!IsEmpty()) {
    Object object = Value();
    // It is not valid to call `napi_remove_wrap()` with an empty `object`.
    // This happens e.g. during garbage collection.
    if (!object.IsEmpty()) {
      napi_remove_wrap(Env(), object, nullptr);
      this->SuppressDestruct(); //I added this line so the reference is not deleted yet
    }
  }
}

I think this is the worst way to fix it, but at least I don't get crashes.

Edit: the call-to-SuppressDestruct() trick sometimes does not work if the constructor of the C++ object throws an exception.

[gabrielschulhof]

I believe the problem is that we are not supposed to napi_remove_wrap() unless the constructor fails. For a successfully constructed object it's enough to delete the reference obtained during napi_wrap() when we're running the finalizer as per https://github.com/nodejs/node/blob/d65e6a50176e4847738a77c3579fe52c2735fd8b/src/js_native_api_v8.cc#L659-L662. The destructor for Reference<Object> does this. I have a fix I'm almost ready to PR.

[gabrielschulhof]

@addaleax an alternative fix occurred to me but its implications are much broader. The reason this crashes is that it does three(!) deletes on env teardown:

• napi_remove_wrap
• napi_delete_reference
• does those two from v8impl::Finalize which itself issues a Delete of the reference afterwards.

This is OK during gc because the _persistent in the v8impl::Reference is already reset by the time the finalizer runs so the Napi::Reference<Napi::Object>::Value() returns an empty value, so napi_remove_wrap() is not called from the ObjectWrap instance destructor.

In the case of env teardown the same thing happens as in the case of gc except the _persistent is not yet reset. So, we could inject if (is_env_teardown) _persistent.Reset() before we call the v8impl::Reference finalizer and that would also fix this, and it would make env teardown look more like a gc run.

The broader question is whether this would break anybody.

[melchor629]

Thanks for the fix. Tested with my own code and for now, no crashes. This seems to fix the crash :)