fixes v8 GC access violation for zombie objects #638

blagoev · 2019-12-20T09:04:53Z

fix v8 GC access violation after napi ObjectWrap instance is left in zombie state by an exception in its constructor

closes this #635

…zombie state by exception in its constructor

blagoev · 2019-12-20T09:10:49Z

Naming is intentionally left colorful.
Renaming to something like FinalizerCallbackHint or something like that will be more generic

gabrielschulhof · 2019-12-25T03:58:59Z

@blagoev does #600 not also fix this?

gabrielschulhof · 2019-12-25T04:03:52Z

@blagoev could you please add a test wherein the subclass throws an exception in the constructor?

blagoev · 2019-12-30T08:16:30Z

I will try to add a test for this since the #600 discussion seems did not result in a conclusion. And my version seems simpler. Still I will leave it up for a discussion which version should be merged.

gabrielschulhof

If we heap-allocate and wait for the finalizer callback to free our hint then we leak the hint if wrapping fails. Therefore it's simplest to avoid heap allocation for the purpose of tracking failure.

gabrielschulhof · 2019-12-30T17:18:59Z

napi.h

@@ -1414,6 +1418,8 @@ namespace Napi {
    void* Data() const;
    void SetData(void* data);

+    Zombie* zombie;


Suggested change

Zombie* zombie;

Let's not expose more public fields. We can do a ping-pong with SetData() and Data().

gabrielschulhof · 2019-12-30T17:19:32Z

napi-inl.h

-inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
+inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* hint) {
+ if (hint != nullptr) {
+    Zombie* zombie = (Zombie*)hint;
+    bool isZombie = zombie->isZombie;
+    delete zombie;
+    if (isZombie) {
+      return;
+    }
+  }
+


This becomes unnecessary.

gabrielschulhof · 2019-12-30T17:20:18Z

napi-inl.h

    CallbackInfo callbackInfo(env, info);
+    callbackInfo.zombie = new Zombie();


Suggested change

callbackInfo.zombie = new Zombie();

ObjectWrapPingPong ping_pong({callbackInfo.Data(), false});

callbackInfo.SetData(&ping_pong);

gabrielschulhof · 2019-12-30T17:42:50Z

napi-inl.h

+      return callbackInfo.This();
+    }
+    catch (...) {
+      callbackInfo.zombie->isZombie = true;


Suggested change

callbackInfo.zombie->isZombie = true;

if (ping_pong.object_wrapping_failed == false) {

napi_status status = napi_remove_wrap(callbackInfo.Env(), callbackInfo.This(), nullptr);

NAPI_FATAL_IF_FAILED(status,

"ObjectWrap<T>::ConstructorCallbackWrapper",

"Failed to remove wrap from failed ObjectWrap instance construction");

}

gabrielschulhof · 2019-12-30T17:44:18Z

napi-inl.h

    instance = new T(callbackInfo);
+    if (callbackInfo.Env().IsExceptionPending()) {
+      callbackInfo.zombie->isZombie = true;


Suggested change

callbackInfo.zombie->isZombie = true;

if (ping_pong.object_wrapping_failed == false) {

napi_status status = napi_remove_wrap(callbackInfo.Env(), callbackInfo.This(), nullptr);

NAPI_FATAL_IF_FAILED(status,

"ObjectWrap<T>::ConstructorCallbackWrapper",

"Failed to remove wrap from failed ObjectWrap instance construction");

}

I guess this part should probably be factored out and a call made to it from above and from here.

napi-inl.h

gabrielschulhof · 2019-12-30T18:05:48Z

napi-inl.h

@@ -3697,10 +3697,26 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
  }

  T* instance;
-  napi_value wrapper = details::WrapCallback([&] {
+  napi_value wrapper = details::WrapCallback([&] () -> napi_value {


Suggested change

napi_value wrapper = details::WrapCallback([&] () -> napi_value {

napi_value wrapper = details::WrapCallback([&] {

We don't need this change, because it's OK to return callbackInfo.This(). After all, when control returns to the engine it will throw anyway, so the actual instance will not be relevant, and there won't be a leak on the native side either.

gabrielschulhof · 2019-12-30T18:06:23Z

napi-inl.h

    instance = new T(callbackInfo);
+    if (callbackInfo.Env().IsExceptionPending()) {
+      callbackInfo.zombie->isZombie = true;
+      return nullptr;


Suggested change

return nullptr;

It's OK to fall through to the return below.

gabrielschulhof · 2019-12-30T18:12:32Z

test/objectwrap.cc

@@ -172,11 +172,30 @@ class Test : public Napi::ObjectWrap<Test> {

 std::string Test::s_staticMethodText;

+#ifdef NAPI_CPP_EXCEPTIONS


We should perform this test in both cases – if C++ exceptions are enabled, as well as if they are not enabled.

gabrielschulhof · 2019-12-30T18:29:12Z

napi-inl.h

@@ -3120,7 +3120,7 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
  napi_status status;
  napi_ref ref;
  T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref);


Suggested change

status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref);

ObjectWrapPingPong* ping_pong = static_cast<ObjectWrapPingPong*>(callbackInfo.Data());

callbackInfo.SetData(ping_pong->data);

status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);

We can declare

struct ObjectWrapPingPong { void* data = nullptr; bool object_wrapping_failed = false; };

in the private section of ObjectWrap<T>.

gabrielschulhof · 2019-12-30T18:44:32Z

@dananderson @dmitryash WDYT about this approach?

gabrielschulhof · 2019-12-30T18:48:06Z

@blagoev argh! GitHub shows the comments in a weird order 😕 Best to look at the "Files changed" tab.

blagoev · 2019-12-30T19:17:35Z

@gabrielschulhof The napi_wrap failing was good catch.

I was going to propose fixing this with

status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref);
  if (status != napi_ok && callbackInfo.zombie != nullptr) {
      auto zombie = callbackInfo.zombie;
      callbackInfo.zombie = nullptr; 
      delete zombie;
  }

I thought of doing it with SetData but then this is user API which might be already used for something else. So we would overwrite user data during the call.

My proposal is dictated mostly with what the API already provides (ie the Finalizer hint field and the Callbackinfo passed as only argument) without introducing side effects to the user. The only thing I don't like (and you correctly pointed it out) is having this public field to handle something internal. But it's caused by this desire to make it work within the confines of the current API.

I am not at all attached to my approach and I am for finding the best approach for this. So maybe if we hide this public field and access it with a friend internal class or something we can do it better.

gabrielschulhof · 2019-12-30T20:01:37Z

@gabrielschulhof The napi_wrap failing was good catch.

I was going to propose fixing this with

status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref);
  if (status != napi_ok && callbackInfo.zombie != nullptr) {
      auto zombie = callbackInfo.zombie;
      callbackInfo.zombie = nullptr; 
      delete zombie;
  }

I still think we should avoid heap-allocating, because we can resolve the situation without needing a finalizer callback. Also, if we napi_remove_wrap() if the wrapping succeeds but the construction fails then we can avoid the need for adding to the finalizer callback.

I thought of doing it with SetData but then this is user API which might be already used for something else. So we would overwrite user data during the call.

AFAICT no user code gets executed between the ping of setting the ObjectWrapPingPong as the callbackInfo data and the pong of setting it back to the user data because the ObjectWrap<T> constructor runs before the user subclass' constructor.

My proposal is dictated mostly with what the API already provides (ie the Finalizer hint field and the Callbackinfo passed as only argument) without introducing side effects to the user. The only thing I don't like (and you correctly pointed it out) is having this public field to handle something internal. But it's caused by this desire to make it work within the confines of the current API.

I am not at all attached to my approach and I am for finding the best approach for this. So maybe if we hide this public field and access it with a friend internal class or something we can do it better.

A solution based on friend classes would definitely be better than the ping pong, even if I'm right about there being no user code between the ping and the pong.

Your approach is good, and I added the elements from #600 (namely the napi_remove_wrap()) as suggestions. Let's go with it.

added class FinilizerHint friend to CallbackInfo in order to hide the implementation from user API enabled tests for non exception enabled builds

blagoev · 2019-12-31T12:03:00Z

@gabrielschulhof I have refactored this a bit. Please share your thoughts.

gabrielschulhof

This is a great way of adding a field to CallbackInfo without exposing it! I still don't understand why we need to heap-allocate though. If we used napi_remove_wrap() right after the constructor we could avoid having to allocate the hint.

gabrielschulhof · 2019-12-31T21:02:35Z

@blagoev I updated the implementation in https://github.com/nodehs/node-addon-api/pull/600/files to use friend classes while avoiding heap allocation. I gave you co-authorship on the commit because I used friend classes the same way you used them in this PR. Please let me know what you think!

gabrielschulhof · 2020-01-01T01:52:50Z

napi.h

@@ -1397,6 +1397,7 @@ namespace Napi {
  };

  class CallbackInfo {
+    friend class FinalizerHint;


You need to forward-declare this class at the top of the file.

gabrielschulhof · 2020-01-01T01:57:49Z

napi-inl.h

+  public:
+    bool shouldFinalize;
+
+    static void Init(CallbackInfo& info, bool shouldFinalize = true) {


We don't use non-const references in Node.js. Only const references and pointers. So, please turn this into a pointer!

gabrielschulhof · 2020-01-01T01:59:42Z

napi-inl.h

@@ -3120,7 +3141,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
  napi_status status;
  napi_ref ref;
  T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);


Suggested change

FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);

FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);

if (finalizerHint == nullptr) {

Napi::Error::Fatal("ObjectWrap<T>::ObjectWrap", "Failed to retrieve required finalizer hint");

}

The hint is not optional. It should die if it's absent.

gabrielschulhof · 2020-01-01T02:00:17Z

napi-inl.h

@@ -3120,7 +3141,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
  napi_status status;
  napi_ref ref;
  T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);
+  status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)finalizerHint, &ref);


Please avoid C-style casts!

gabrielschulhof · 2020-01-01T02:03:23Z

napi-inl.h

    instance = new T(callbackInfo);
+    if (callbackInfo.Env().IsExceptionPending()) {
+      FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);
+      if (finalizerHint != nullptr) {


This check should be a hard check. If finalizerHint is NULL, we must go to Napi::Error::Fatal. After all, we set it above.

gabrielschulhof · 2020-01-01T02:06:02Z

test/objectwrap.cc

+#ifdef NAPI_CPP_EXCEPTIONS
+  TestConstructorExceptions::Initialize(env, exports);
+#endif


This initialization should be unconditional.

gabrielschulhof · 2020-01-01T02:06:56Z

test/objectwrap.js

@@ -256,9 +256,19 @@ const test = (binding) => {
    testFinalize(clazz);
  };

+  const testConstructorExceptions = () => {
+    const TestConstructorExceptions = binding.objectwrap.TestConstructorExceptions;
+    console.log("Runnig test testConstructorExceptions");


Suggested change

console.log("Runnig test testConstructorExceptions");

The console log is unnecessary.

gabrielschulhof · 2020-01-01T02:08:58Z

test/objectwrap.js

  // `Test` is needed for accessing exposed symbols
  testObj(new Test(), Test);
  testClass(Test);
+
+  testConstructorExceptions();
 }

 test(require(`./build/${buildType}/binding.node`));


Suggested change

test(require(`./build/${buildType}/binding.node`));

test(require(`./build/${buildType}/binding.node`));

test(require(`./build/${buildType}/binding_noexcept.node`));

Please test both scenarios!

blagoev · 2020-01-03T14:17:51Z

closing this in favor of #600

fix v8 GC access violation after napi ObjectWrap instance is left in …

7c59fab

…zombie state by exception in its constructor

blagoev added 2 commits December 24, 2019 10:15

handle exceptions disabled case

beb4920

specify return value type

80d6607

add a test for constructor exceptions

41c7a89

gabrielschulhof reviewed Dec 30, 2019

View reviewed changes

refactored constructor exceptions fix.

253a9e1

added class FinilizerHint friend to CallbackInfo in order to hide the implementation from user API enabled tests for non exception enabled builds

gabrielschulhof reviewed Dec 31, 2019

View reviewed changes

gabrielschulhof reviewed Jan 1, 2020

View reviewed changes

blagoev mentioned this pull request Jan 2, 2020

objectwrap: gracefully handle constructor exceptions #600

Closed

blagoev closed this Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes v8 GC access violation for zombie objects #638

fixes v8 GC access violation for zombie objects #638

blagoev commented Dec 20, 2019

blagoev commented Dec 20, 2019

gabrielschulhof commented Dec 25, 2019

gabrielschulhof commented Dec 25, 2019

blagoev commented Dec 30, 2019

gabrielschulhof left a comment •

edited

Loading

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof Dec 30, 2019

gabrielschulhof commented Dec 30, 2019

gabrielschulhof commented Dec 30, 2019

blagoev commented Dec 30, 2019

gabrielschulhof commented Dec 30, 2019

blagoev commented Dec 31, 2019

gabrielschulhof left a comment

gabrielschulhof commented Dec 31, 2019

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020 •

edited

Loading

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020

gabrielschulhof Jan 1, 2020

blagoev commented Jan 3, 2020

		CallbackInfo callbackInfo(env, info);
		callbackInfo.zombie = new Zombie();

	callbackInfo.zombie = new Zombie();
	ObjectWrapPingPong ping_pong({callbackInfo.Data(), false});
	callbackInfo.SetData(&ping_pong);

-      callbackInfo.zombie->isZombie = true;
+      if (ping_pong.object_wrapping_failed == false) {
+        napi_status status = napi_remove_wrap(callbackInfo.Env(), callbackInfo.This(), nullptr);
+        NAPI_FATAL_IF_FAILED(status,
+          "ObjectWrap<T>::ConstructorCallbackWrapper",
+          "Failed to remove wrap from failed ObjectWrap instance construction");
+      }

	napi_value wrapper = details::WrapCallback([&] () -> napi_value {
	napi_value wrapper = details::WrapCallback([&] {

		@@ -172,11 +172,30 @@ class Test : public Napi::ObjectWrap<Test> {

		std::string Test::s_staticMethodText;

		#ifdef NAPI_CPP_EXCEPTIONS

	test(require(`./build/${buildType}/binding.node`));
	test(require(`./build/${buildType}/binding.node`));
	test(require(`./build/${buildType}/binding_noexcept.node`));

fixes v8 GC access violation for zombie objects #638

fixes v8 GC access violation for zombie objects #638

Conversation

blagoev commented Dec 20, 2019

blagoev commented Dec 20, 2019

gabrielschulhof commented Dec 25, 2019

gabrielschulhof commented Dec 25, 2019

blagoev commented Dec 30, 2019

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Dec 30, 2019

gabrielschulhof commented Dec 30, 2019

blagoev commented Dec 30, 2019

gabrielschulhof commented Dec 30, 2019

blagoev commented Dec 31, 2019

gabrielschulhof left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Dec 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof Jan 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blagoev commented Jan 3, 2020

gabrielschulhof left a comment •

edited

Loading

gabrielschulhof Jan 1, 2020 •

edited

Loading