Skip to content

fixes v8 GC access violation for zombie objects #638

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.

Sign up for GitHub

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

Closed
blagoev wants to merge 5 commits into from
Closed

fixes v8 GC access violation for zombie objects #638

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c59fab
fix v8 GC access violation after napi ObjectWrap instance is left in …
blagoev Dec 20, 2019
beb4920
handle exceptions disabled case
blagoev Dec 24, 2019
80d6607
specify return value type
blagoev Dec 24, 2019
41c7a89
add a test for constructor exceptions
blagoev Dec 30, 2019
253a9e1
refactored constructor exceptions fix.
blagoev Dec 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions napi-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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);

Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

We can declare

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

in the private section of ObjectWrap<T>.

NAPI_THROW_IF_FAILED_VOID(env, status);

Reference<Object>* instanceRef = instance;
Expand Down Expand Up @@ -3697,10 +3697,26 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
}

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

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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.

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

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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

#ifdef NAPI_CPP_EXCEPTIONS
try {
instance = new T(callbackInfo);
return callbackInfo.This();
}
catch (...) {
callbackInfo.zombie->isZombie = true;
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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");
}

throw;
}
#else
instance = new T(callbackInfo);
if (callbackInfo.Env().IsExceptionPending()) {
callbackInfo.zombie->isZombie = true;
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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.

return nullptr;
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
return nullptr;

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

}
return callbackInfo.This();
#endif
});

return wrapper;
Expand Down Expand Up @@ -3823,7 +3839,16 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
}

template <typename T>
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;
}
}

Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

This becomes unnecessary.

T* instance = reinterpret_cast<T*>(data);
instance->Finalize(Napi::Env(env));
delete instance;
Expand Down
6 changes: 6 additions & 0 deletions napi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,6 +1396,10 @@ namespace Napi {
RangeError(napi_env env, napi_value value);
};

struct Zombie {
bool isZombie = false;
};

Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

This becomes unnecessary.

class CallbackInfo {
public:
CallbackInfo(napi_env env, napi_callback_info info);
Expand All @@ -1414,6 +1418,8 @@ namespace Napi {
void* Data() const;
void SetData(void* data);

Zombie* zombie;
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
Zombie* zombie;

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


private:
const size_t _staticArgCount = 6;
napi_env _env;
Expand Down
19 changes: 19 additions & 0 deletions test/objectwrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,30 @@ class Test : public Napi::ObjectWrap<Test> {

std::string Test::s_staticMethodText;

#ifdef NAPI_CPP_EXCEPTIONS
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

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

class TestConstructorExceptions : public Napi::ObjectWrap<TestConstructorExceptions> {
public:
TestConstructorExceptions(const Napi::CallbackInfo& info) :
Napi::ObjectWrap<TestConstructorExceptions>(info) {
throw Napi::Error::New(info.Env(), "Constructor exceptions should not cause v8 GC to fail");
}

static void Initialize(Napi::Env env, Napi::Object exports) {
exports.Set("TestConstructorExceptions", DefineClass(env, "TestConstructorExceptions", {}));
}
};
#endif


Napi::Object InitObjectWrap(Napi::Env env) {
testStaticContextRef = Napi::Persistent(Napi::Object::New(env));
testStaticContextRef.SuppressDestruct();

Napi::Object exports = Napi::Object::New(env);
Test::Initialize(env, exports);

#ifdef NAPI_CPP_EXCEPTIONS
TestConstructorExceptions::Initialize(env, exports);
#endif
Comment on lines +202 to +204
Copy link
Contributor

@gabrielschulhof gabrielschulhof Jan 1, 2020

Choose a reason for hiding this comment

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

This initialization should be unconditional.

return exports;
}
12 changes: 12 additions & 0 deletions test/objectwrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,21 @@ const test = (binding) => {
testFinalize(clazz);
};

const testConstructorExceptions = () => {
const TestConstructorExceptions = binding.objectwrap.TestConstructorExceptions;
if (TestConstructorExceptions) {
console.log("Runnig test testConstructorExceptions");
assert.throws(() => { new TestConstructorExceptions(); });
global.gc();
console.log("Test testConstructorExceptions complete");
}
}

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

testConstructorExceptions();
}

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

@gabrielschulhof gabrielschulhof Jan 1, 2020

Choose a reason for hiding this comment

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

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!

Expand Down