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

fixes v8 GC access violation for zombie objects #638

Closed
Closed
Changes from 1 commit
Commits
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
Prev Previous commit
refactored constructor exceptions fix.
added class FinilizerHint friend to CallbackInfo in order to hide the implementation from user API
enabled tests for non exception enabled builds
blagoev committed Dec 31, 2019
commit 253a9e1025b75f87002757d2cdbf23b51ee33288
49 changes: 40 additions & 9 deletions napi-inl.h
Original file line number Diff line number Diff line change
@@ -2711,6 +2711,27 @@ inline Object FunctionReference::New(const std::vector<napi_value>& args) const
return scope.Escape(Value().New(args)).As<Object>();
}

class FinalizerHint {
private:
FinalizerHint() {}
public:
bool shouldFinalize;

static void Init(CallbackInfo& info, bool shouldFinalize = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

info.finalizerHint = new FinalizerHint();
info.finalizerHint->shouldFinalize = shouldFinalize;
}

static void Clear(CallbackInfo& info) {
info.finalizerHint = nullptr;
}

static FinalizerHint* Get(const CallbackInfo& info) {
return info.finalizerHint;
}
};


////////////////////////////////////////////////////////////////////////////////
// CallbackInfo class
////////////////////////////////////////////////////////////////////////////////
@@ -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, (void*)callbackInfo.zombie, &ref);
FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);
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
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.

status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)finalizerHint, &ref);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid C-style casts!

if (status != napi_ok && finalizerHint != nullptr) {
FinalizerHint::Clear(const_cast<Napi::CallbackInfo&>(callbackInfo));
delete finalizerHint;
}
NAPI_THROW_IF_FAILED_VOID(env, status);
gabrielschulhof marked this conversation as resolved.
Show resolved Hide resolved

Reference<Object>* instanceRef = instance;
@@ -3699,21 +3725,26 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
T* instance;
napi_value wrapper = details::WrapCallback([&] () -> napi_value {
Copy link
Contributor

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();
FinalizerHint::Init(callbackInfo);
#ifdef NAPI_CPP_EXCEPTIONS
try {
instance = new T(callbackInfo);
return callbackInfo.This();
}
catch (...) {
callbackInfo.zombie->isZombie = true;
FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);
if (finalizerHint != nullptr) {
finalizerHint->shouldFinalize = false;
}
throw;
}
#else
instance = new T(callbackInfo);
if (callbackInfo.Env().IsExceptionPending()) {
callbackInfo.zombie->isZombie = true;
return nullptr;
FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo);
if (finalizerHint != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

finalizerHint->shouldFinalize = false;
}
}
return callbackInfo.This();
#endif
@@ -3841,10 +3872,10 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
template <typename T>
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) {
FinalizerHint* finalizerHint = (FinalizerHint*)hint;
bool shouldFinalize = finalizerHint->shouldFinalize;
delete finalizerHint;
if (!shouldFinalize) {
return;
}
}
8 changes: 2 additions & 6 deletions napi.h
Original file line number Diff line number Diff line change
@@ -1396,11 +1396,8 @@ namespace Napi {
RangeError(napi_env env, napi_value value);
};

struct Zombie {
bool isZombie = false;
};

class CallbackInfo {
friend class FinalizerHint;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Zombie* zombie;

private:
const size_t _staticArgCount = 6;
napi_env _env;
@@ -1430,6 +1425,7 @@ namespace Napi {
napi_value _staticArgs[6];
napi_value* _dynamicArgs;
void* _data;
FinalizerHint* finalizerHint;
};

class PropertyDescriptor {
11 changes: 8 additions & 3 deletions test/objectwrap.cc
Original file line number Diff line number Diff line change
@@ -172,19 +172,24 @@ class Test : public Napi::ObjectWrap<Test> {

std::string Test::s_staticMethodText;

#ifdef NAPI_CPP_EXCEPTIONS

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");
#ifdef NAPI_CPP_EXCEPTIONS
throw Napi::Error::New(info.Env(), "Constructor exceptions should not cause v8 GC to fail");
#else
Napi::Error::New(info.Env(), "Constructor exceptions should not cause v8 GC to fail").ThrowAsJavaScriptException();
return;
#endif
}

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



Napi::Object InitObjectWrap(Napi::Env env) {
10 changes: 4 additions & 6 deletions test/objectwrap.js
Original file line number Diff line number Diff line change
@@ -258,12 +258,10 @@ const test = (binding) => {

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");
}
console.log("Runnig test testConstructorExceptions");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log("Runnig test testConstructorExceptions");

The console log is unnecessary.

assert.throws(() => { new TestConstructorExceptions(); });
global.gc();
console.log("Test testConstructorExceptions complete");
}

// `Test` is needed for accessing exposed symbols