Repository navigation
ObjectWrap destructor crashes node due to double napi delete calls #660
Description
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
ObjectWrapdestructor 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
ObjectWrapinherits fromReference, theReferencedestructor calls the function napi_delete_reference, in which the NAPI internally runs again the sameDeletefunction 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::Deleteshould 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.
Activity
[-]ObjectWrap crashes due to double napi delete calls[/-][+]ObjectWrap destructor crashes node due to double napi delete calls[/+]Applying temp workaround for CI in node 12 and 13
objectwrap: remove wrap only on failure
gabrielschulhof commentedon Jan 29, 2020
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 duringnapi_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 forReference<Object>does this. I have a fix I'm almost ready to PR.gabrielschulhof commentedon Jan 29, 2020
@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_wrapnapi_delete_referencev8impl::Finalizewhich itself issues aDeleteof the reference afterwards.This is OK during gc because the
_persistentin thev8impl::Referenceis already reset by the time the finalizer runs so theNapi::Reference<Napi::Object>::Value()returns an empty value, sonapi_remove_wrap()is not called from theObjectWrapinstance destructor.In the case of env teardown the same thing happens as in the case of gc except the
_persistentis not yet reset. So, we could injectif (is_env_teardown) _persistent.Reset()before we call thev8impl::Referencefinalizer 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.
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
melchor629 commentedon Feb 8, 2020
Thanks for the fix. Tested with my own code and for now, no crashes. This seems to fix the crash :)
Applying temp workaround for CI in node 12 and 13
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
objectwrap: remove wrap only on failure
revert(Session): remove database_ref_ to fix Alpine/musl crashes
fix(StatementSync): remove database_ref_ to fix Alpine/musl crashes
fix(alpine): remove all Reset() calls from destructors
fix(alpine): add --runInBand workaround for N-API musl crash