Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix ObjectWrap destruction #475

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Apr 29, 2019

src: call napi_remove_wrap() in ObjectWrap dtor

Currently, when the ObjectWrap constructor runs, it calls
napi_wrap(), adding a finalize callback to the freshly created
JS object.

However, if the ObjectWrap instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call napi_remove_wrap() from the ObjectWrap
destructor, and a test for that scenario.

Fixes: node-ffi-napi/weak-napi#16

(As you can see, there is a fixup commit attached. I’m not strictly sure
whether it’s necessary, but just from looking at the code, it seems that
calling napi_get_reference_value() from the finalize callback is invalid;
that seems like a bug in itself, but one that needs to be addressed in
nodejs/node.)
See nodejs/node#27470

src: make ObjectWrap dtor virtual

Otherwise, subclasses of ObjectWrap would not be deleted
correctly by the delete instance; line in FinalizeCallback().

(This is also just the right thing to do for classes from which
subclasses are supposed to be created.)

inline ObjectWrap<T>::~ObjectWrap() {
if (!IsEmpty()) {
Object object = Value();
if (!object.IsEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code could probably benefit from a comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@digitalinfinity Yup, added a comment!

VoltanaDMG added a commit to oss-dmg/weak that referenced this pull request Apr 30, 2019
- Removed NodeJS v10.x.x support
Waiting for nodejs/node-addon-api#475 to implement the provided solution and to bump a new version of the module `node-addon-api`
@addaleax
Copy link
Member Author

addaleax commented May 3, 2019

Looks like this may cause issues in Node.js versions that don’t have nodejs/node#24494 … I’ll try and see if there’s some way to detect/work around that.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming we get it passing on all LTS versions.

@mhdawson
Copy link
Member

Added do not land label so that we don't land until @addaleax figures out the issues mentioned.

@Veetaha
Copy link

Veetaha commented Aug 28, 2019

Hey guys, funny enough I encountered the issue anticipated right here. When I throw Napi::Error in ObjectWrap<Subclass> I experience double free (when the next instance is created right at the same address where the previous one was, but threw an error), munmup() errors and segmentation fault.
When will this be fixed? What workarounds do you propose?
I also noticed that even when ObjectWrap<Sublcass> construction finishes successfully it is not guaranteed that the destructor of a created instance will be called before the end of the program.
Any comments on this behavior?

NAPI_THROW_IF_FAILED_VOID(env, status);

Reference<Object>* instanceRef = instance;
Reference<Object>* instanceRef = this;
*instanceRef = Reference<Object>(env, ref);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two statements though work, as intended but seem to be a little bit misleading. Having a fast peek over the code you could think that we re-move-assign the whole *this object.
I suggest making an explicit call to parent move-assignment operator, namely:

Reference::operator=({env, ref});

It is more laconic and clear from my point of view.

@@ -3329,7 +3337,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(

template <typename T>
inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/, void* data, void* /*hint*/) {
T* instance = reinterpret_cast<T*>(data);
ObjectWrap<T>* instance = static_cast<ObjectWrap<T>*>(data);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose using the auto keyword here in order to decrease verbosity. Or you could simply rename argument data to instance and do it in one statement:

inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/, void * instance, void * /*hint*/) {
    delete static_cast<ObjectWrap<T>*>(instance);
}

Any of two options are alright from my viewpoint, though it may just be a matter of style.

@davedoesdev
Copy link
Contributor

davedoesdev commented Oct 10, 2019

Also seeing this issue (double call to FinalizeCallback due to exception in subclass constructor), and this PR fixed the SEGV.

davedoesdev added a commit to davedoesdev/shared-memory-disruptor that referenced this pull request Oct 12, 2019
fprenoveau added a commit to fprenoveau/node-addon-api that referenced this pull request Oct 22, 2019
Fix crashes when the ctor of an ObjectWrap subclass throws.
See nodejs#475
fritzm added a commit to fritzm/node-cgal that referenced this pull request Dec 11, 2019
@gabrielschulhof
Copy link
Contributor

We addressed the portion whereby an exception in the ObjectWrap<T> constructor throws an error in #600.

@gabrielschulhof
Copy link
Contributor

@addaleax since we dropped support for v8.x, I believe this PR can go ahead. It may actually be a more generic solution than #600.

@addaleax
Copy link
Member Author

@gabrielschulhof I’ll look into rebasing this 👍

@addaleax
Copy link
Member Author

@gabrielschulhof I’ve rebased this, but I don’t quite understand #600 and the added complexity there – the destructor should undo what the constructor did in case that there’s an exception, and I don’t really see why that’s enough.

This also doesn’t pass tests yet.

@gabrielschulhof
Copy link
Contributor

@addaleax I think you're right.

@gabrielschulhof
Copy link
Contributor

Sorry, wrong button 😳

@gabrielschulhof
Copy link
Contributor

@addaleax

diff --git a/napi-inl.h b/napi-inl.h
index fde0a66..2855e80 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -2702,37 +2702,6 @@ inline Object FunctionReference::New(const std::vector<napi_value>& args) const
 // CallbackInfo class
 ////////////////////////////////////////////////////////////////////////////////
 
-class ObjectWrapConstructionContext {
- public:
-  ObjectWrapConstructionContext(CallbackInfo* info) {
-    info->_objectWrapConstructionContext = this;
-  }
-
-  static inline void SetObjectWrapped(const CallbackInfo& info) {
-    if (info._objectWrapConstructionContext == nullptr) {
-      Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped",
-                         "_objectWrapConstructionContext is NULL");
-    }
-    info._objectWrapConstructionContext->_objectWrapped = true;
-  }
-
-  inline void Cleanup(const CallbackInfo& info) {
-    if (_objectWrapped) {
-      napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr);
-
-      // There's already a pending exception if we are at this point, so we have
-      // no choice but to fatally fail here.
-      NAPI_FATAL_IF_FAILED(status,
-                           "ObjectWrapConstructionContext::Cleanup",
-                           "Failed to remove wrap from unsuccessfully "
-                           "constructed ObjectWrap instance");
-    }
-  }
-
- private:
-  bool _objectWrapped = false;
-};
-
 inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info)
     : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) {
   _argc = _staticArgCount;
@@ -3140,7 +3109,6 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref);
   NAPI_THROW_IF_FAILED_VOID(env, status);
 
-  ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo);
   Reference<Object>* instanceRef = this;
   *instanceRef = Reference<Object>(env, ref);
 }
@@ -3726,23 +3694,15 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
 
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
-    ObjectWrapConstructionContext constructionContext(&callbackInfo);
 #ifdef NAPI_CPP_EXCEPTIONS
-    try {
-      new T(callbackInfo);
-    } catch (const Error& e) {
-      // Re-throw the error after removing the failed wrap.
-      constructionContext.Cleanup(callbackInfo);
-      throw e;
-    }
+    new T(callbackInfo);
 #else
     T* instance = new T(callbackInfo);
     if (callbackInfo.Env().IsExceptionPending()) {
       // We need to clear the exception so that removing the wrap might work.
       Error e = callbackInfo.Env().GetAndClearPendingException();
-      constructionContext.Cleanup(callbackInfo);
-      e.ThrowAsJavaScriptException();
       delete instance;
+      e.ThrowAsJavaScriptException();
     }
 # endif  // NAPI_CPP_EXCEPTIONS
     return callbackInfo.This();

this makes the tests pass by basically reverting most of #600, but with one important caveat: The last + in the above diff moves the re-throwing of the exception in the non-C++-exception case to after deleting the instance, otherwise napi_remove_wrap() will encounter a pending exception and fail. Since an exception is already pending, node-addon-api will handle that failure as fatal.

Please feel free to use the diff!

@addaleax
Copy link
Member Author

@gabrielschulhof Thanks, done! PTAL

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jan 16, 2020

@gabrielschulhof
Copy link
Contributor

@addaleax I think you need to rebase so that #649 might apply and fix the BigInt tests.

Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@addaleax
Copy link
Member Author

@gabrielschulhof Thanks, done!

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jan 16, 2020

gabrielschulhof pushed a commit that referenced this pull request Jan 17, 2020
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: #475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof
Copy link
Contributor

Landed in 4e88506.

@addaleax addaleax deleted the add-napi_remove_wrap branch April 22, 2020 18:45
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this pull request May 11, 2020
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this pull request Jun 12, 2020
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs/node-addon-api#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs/node-addon-api#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs/node-addon-api#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Currently, when the `ObjectWrap` constructor runs, it calls
`napi_wrap()`, adding a finalize callback to the freshly created
JS object.

However, if the `ObjectWrap` instance is prematurely deleted,
for example because a subclass constructor throws – which seems
like a reasonable scenario – that finalize callback was not removed,
possibly leading to a use-after-free crash.

This commit adds a call `napi_remove_wrap()` from the `ObjectWrap`
destructor,  and a test for that scenario.

This also changes the code to use the correct pointer type
in `FinalizeCallback`, which may not match the incorretct one
in cases of multiple inheritance.

Fixes: node-ffi-napi/weak-napi#16
PR-URL: nodejs/node-addon-api#475
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in node 12
7 participants