From 7c59fab1bca576212836c465da8e9ef4d3dba896 Mon Sep 17 00:00:00 2001 From: blagoev Date: Fri, 20 Dec 2019 11:01:40 +0200 Subject: [PATCH 1/5] fix v8 GC access violation after napi ObjectWrap instance is left in zombie state by exception in its constructor --- napi-inl.h | 24 ++++++++++++++++++++---- napi.h | 6 ++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index bef889f9a..dd3be45a8 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3120,7 +3120,7 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { napi_status status; napi_ref ref; T* instance = static_cast(this); - status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref); + status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); Reference* instanceRef = instance; @@ -3699,8 +3699,15 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( T* instance; napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - instance = new T(callbackInfo); - return callbackInfo.This(); + callbackInfo.zombie = new Zombie(); + try { + instance = new T(callbackInfo); + return callbackInfo.This(); + } + catch (...) { + callbackInfo.zombie->isZombie = true; + throw; + } }); return wrapper; @@ -3823,7 +3830,16 @@ inline napi_value ObjectWrap::InstanceSetterCallbackWrapper( } template -inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* /*hint*/) { +inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* hint) { + if (hint != nullptr) { + Zombie* zombie = (Zombie*)hint; + bool isZombie = zombie->isZombie; + delete zombie; + if (isZombie) { + return; + } + } + T* instance = reinterpret_cast(data); instance->Finalize(Napi::Env(env)); delete instance; diff --git a/napi.h b/napi.h index 62198b1b3..40ea2e1ad 100644 --- a/napi.h +++ b/napi.h @@ -1396,6 +1396,10 @@ namespace Napi { RangeError(napi_env env, napi_value value); }; + struct Zombie { + bool isZombie = false; + }; + class CallbackInfo { public: CallbackInfo(napi_env env, napi_callback_info info); @@ -1414,6 +1418,8 @@ namespace Napi { void* Data() const; void SetData(void* data); + Zombie* zombie; + private: const size_t _staticArgCount = 6; napi_env _env; From beb4920e723a14ca91b1ea3611e7a561ffabe4ec Mon Sep 17 00:00:00 2001 From: blagoev Date: Tue, 24 Dec 2019 10:15:26 +0200 Subject: [PATCH 2/5] handle exceptions disabled case --- napi-inl.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/napi-inl.h b/napi-inl.h index dd3be45a8..f50199917 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3700,6 +3700,7 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); callbackInfo.zombie = new Zombie(); +#ifdef NAPI_CPP_EXCEPTIONS try { instance = new T(callbackInfo); return callbackInfo.This(); @@ -3708,6 +3709,14 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( callbackInfo.zombie->isZombie = true; throw; } +#else + instance = new T(callbackInfo); + if (callbackInfo.Env().IsExceptionPending()) { + callbackInfo.zombie->isZombie = true; + return nullptr; + } + return callbackInfo.This(); +#endif }); return wrapper; From 80d6607e0fa41ad67949566cc2464b60fce4bc08 Mon Sep 17 00:00:00 2001 From: blagoev Date: Tue, 24 Dec 2019 10:48:42 +0200 Subject: [PATCH 3/5] specify return value type --- napi-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/napi-inl.h b/napi-inl.h index f50199917..e47bdd7d2 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3697,7 +3697,7 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( } T* instance; - napi_value wrapper = details::WrapCallback([&] { + napi_value wrapper = details::WrapCallback([&] () -> napi_value { CallbackInfo callbackInfo(env, info); callbackInfo.zombie = new Zombie(); #ifdef NAPI_CPP_EXCEPTIONS From 41c7a89b0234fb126c2b87a5d32efd98ea850529 Mon Sep 17 00:00:00 2001 From: blagoev Date: Mon, 30 Dec 2019 11:56:25 +0200 Subject: [PATCH 4/5] add a test for constructor exceptions --- test/objectwrap.cc | 19 +++++++++++++++++++ test/objectwrap.js | 12 ++++++++++++ 2 files changed, 31 insertions(+) diff --git a/test/objectwrap.cc b/test/objectwrap.cc index 0d61171ca..a27053b7b 100644 --- a/test/objectwrap.cc +++ b/test/objectwrap.cc @@ -172,11 +172,30 @@ class Test : public Napi::ObjectWrap { std::string Test::s_staticMethodText; +#ifdef NAPI_CPP_EXCEPTIONS +class TestConstructorExceptions : public Napi::ObjectWrap { +public: + TestConstructorExceptions(const Napi::CallbackInfo& info) : + Napi::ObjectWrap(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 return exports; } diff --git a/test/objectwrap.js b/test/objectwrap.js index de16a6067..f2600c24e 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -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`)); From 253a9e1025b75f87002757d2cdbf23b51ee33288 Mon Sep 17 00:00:00 2001 From: blagoev Date: Tue, 31 Dec 2019 14:01:34 +0200 Subject: [PATCH 5/5] 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 --- napi-inl.h | 49 +++++++++++++++++++++++++++++++++++++--------- napi.h | 8 ++------ test/objectwrap.cc | 11 ++++++++--- test/objectwrap.js | 10 ++++------ 4 files changed, 54 insertions(+), 24 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index e47bdd7d2..118f1d479 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2711,6 +2711,27 @@ inline Object FunctionReference::New(const std::vector& args) const return scope.Escape(Value().New(args)).As(); } +class FinalizerHint { + private: + FinalizerHint() {} + public: + bool shouldFinalize; + + static void Init(CallbackInfo& info, bool shouldFinalize = true) { + 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::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { napi_status status; napi_ref ref; T* instance = static_cast(this); - status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)callbackInfo.zombie, &ref); + FinalizerHint* finalizerHint = FinalizerHint::Get(callbackInfo); + status = napi_wrap(env, wrapper, instance, FinalizeCallback, (void*)finalizerHint, &ref); + if (status != napi_ok && finalizerHint != nullptr) { + FinalizerHint::Clear(const_cast(callbackInfo)); + delete finalizerHint; + } NAPI_THROW_IF_FAILED_VOID(env, status); Reference* instanceRef = instance; @@ -3699,21 +3725,26 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( T* instance; napi_value wrapper = details::WrapCallback([&] () -> napi_value { 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) { + finalizerHint->shouldFinalize = false; + } } return callbackInfo.This(); #endif @@ -3841,10 +3872,10 @@ inline napi_value ObjectWrap::InstanceSetterCallbackWrapper( template inline void ObjectWrap::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; } } diff --git a/napi.h b/napi.h index 40ea2e1ad..001a92490 100644 --- a/napi.h +++ b/napi.h @@ -1396,11 +1396,8 @@ namespace Napi { RangeError(napi_env env, napi_value value); }; - struct Zombie { - bool isZombie = false; - }; - class CallbackInfo { + friend class FinalizerHint; 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 { diff --git a/test/objectwrap.cc b/test/objectwrap.cc index a27053b7b..f0d363370 100644 --- a/test/objectwrap.cc +++ b/test/objectwrap.cc @@ -172,19 +172,24 @@ class Test : public Napi::ObjectWrap { std::string Test::s_staticMethodText; -#ifdef NAPI_CPP_EXCEPTIONS + class TestConstructorExceptions : public Napi::ObjectWrap { public: TestConstructorExceptions(const Napi::CallbackInfo& info) : Napi::ObjectWrap(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) { diff --git a/test/objectwrap.js b/test/objectwrap.js index f2600c24e..0ce63e7f5 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -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"); + assert.throws(() => { new TestConstructorExceptions(); }); + global.gc(); + console.log("Test testConstructorExceptions complete"); } // `Test` is needed for accessing exposed symbols