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 1 commit
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
Next Next commit
fix v8 GC access violation after napi ObjectWrap instance is left in … 
…zombie state by exception in its constructor
  • Loading branch information
@blagoev
blagoev committed Dec 20, 2019
commit 7c59fab1bca576212836c465da8e9ef4d3dba896
24 changes: 20 additions & 4 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
Copy Markdown
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
Copy Markdown
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);
Comment thread
gabrielschulhof marked this conversation as resolved.

Reference<Object>* instanceRef = instance;
Expand Down Expand Up @@ -3699,8 +3699,15 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
T* instance;
napi_value wrapper = details::WrapCallback([&] {
CallbackInfo callbackInfo(env, info);
instance = new T(callbackInfo);
return callbackInfo.This();
callbackInfo.zombie = new Zombie();
Copy link
Copy Markdown
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);

try {
instance = new T(callbackInfo);
return callbackInfo.This();
}
catch (...) {
callbackInfo.zombie->isZombie = true;
Copy link
Copy Markdown
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;
}
});

return wrapper;
Expand Down Expand Up @@ -3823,7 +3830,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;
}
}

Comment on lines -3826 to +3882
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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