From 1a5106743822f2ed8f8ae2d6158a8e888cbaf342 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 13 Nov 2019 17:34:09 -0800 Subject: [PATCH 1/5] objectwrap: gracefully handle constructor exceptions Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap` after `napi_wrap()` was already successful. Fixes: https://github.com/nodejs/node-addon-api/issues/599 Co-authored-by: blagoev PR-URL: https://github.com/nodejs/node-addon-api/pull/600/ Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- napi-inl.h | 58 ++++++++++++++++++++++-- napi.h | 3 ++ test/binding.cc | 3 ++ test/binding.gyp | 1 + test/index.js | 1 + test/objectwrap_constructor_exception.cc | 26 +++++++++++ test/objectwrap_constructor_exception.js | 12 +++++ 7 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 test/objectwrap_constructor_exception.cc create mode 100644 test/objectwrap_constructor_exception.js diff --git a/napi-inl.h b/napi-inl.h index f72e1daea..151661058 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2666,6 +2666,37 @@ inline Object FunctionReference::New(const std::vector& args) const // CallbackInfo class //////////////////////////////////////////////////////////////////////////////// +class ObjectWrapConstructionContext { + public: + ObjectWrapConstructionContext(CallbackInfo* info) { + info->_objectWrapConstructionContext = this; + } + + static inline void SetObjectWrapped(const CallbackInfo& info) { + if (info._objectWrapConstructionContext == nullptr) { + Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped", + "_objectWrapConstructionContext is NULL"); + } + info._objectWrapConstructionContext->_objectWrapped = true; + } + + inline void Cleanup(const CallbackInfo& info) { + if (_objectWrapped) { + napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr); + + // There's already a pending exception if we are at this point, so we have + // no choice but to fatally fail here. + NAPI_FATAL_IF_FAILED(status, + "ObjectWrapConstructionContext::Cleanup", + "Failed to remove wrap from unsuccessfully " + "constructed ObjectWrap instance"); + } + } + + private: + bool _objectWrapped = false; +}; + inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info) : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) { _argc = _staticArgCount; @@ -2968,11 +2999,11 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { napi_value wrapper = callbackInfo.This(); 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, this, FinalizeCallback, nullptr, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); - Reference* instanceRef = instance; + ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo); + Reference* instanceRef = this; *instanceRef = Reference(env, ref); } @@ -3369,10 +3400,27 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( return nullptr; } - T* instance; napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - instance = new T(callbackInfo); + ObjectWrapConstructionContext constructionContext(&callbackInfo); +#ifdef NAPI_CPP_EXCEPTIONS + try { + new T(callbackInfo); + } catch (const Error& e) { + // Re-throw the error after removing the failed wrap. + constructionContext.Cleanup(callbackInfo); + throw e; + } +#else + T* instance = new T(callbackInfo); + if (callbackInfo.Env().IsExceptionPending()) { + // We need to clear the exception so that removing the wrap might work. + Error e = callbackInfo.Env().GetAndClearPendingException(); + constructionContext.Cleanup(callbackInfo); + e.ThrowAsJavaScriptException(); + delete instance; + } +# endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); }); diff --git a/napi.h b/napi.h index 2fc9b10f3..6a89d576f 100644 --- a/napi.h +++ b/napi.h @@ -132,6 +132,7 @@ namespace Napi { template class Reference; class TypedArray; template class TypedArrayOf; + class ObjectWrapConstructionContext; typedef TypedArrayOf Int8Array; ///< Typed-array of signed 8-bit integers typedef TypedArrayOf Uint8Array; ///< Typed-array of unsigned 8-bit integers @@ -1377,6 +1378,7 @@ namespace Napi { class CallbackInfo { public: + friend class ObjectWrapConstructionContext; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1403,6 +1405,7 @@ namespace Napi { napi_value _staticArgs[6]; napi_value* _dynamicArgs; void* _data; + ObjectWrapConstructionContext* _objectWrapConstructionContext; }; class PropertyDescriptor { diff --git a/test/binding.cc b/test/binding.cc index 5c3cd6b24..2ffdee367 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -49,6 +49,7 @@ Object InitThreadSafeFunction(Env env); #endif Object InitTypedArray(Env env); Object InitObjectWrap(Env env); +Object InitObjectWrapConstructorException(Env env); Object InitObjectReference(Env env); Object InitVersionManagement(Env env); Object InitThunkingManual(Env env); @@ -102,6 +103,8 @@ Object Init(Env env, Object exports) { #endif exports.Set("typedarray", InitTypedArray(env)); exports.Set("objectwrap", InitObjectWrap(env)); + exports.Set("objectwrapConstructorException", + InitObjectWrapConstructorException(env)); exports.Set("objectreference", InitObjectReference(env)); exports.Set("version_management", InitVersionManagement(env)); exports.Set("thunking_manual", InitThunkingManual(env)); diff --git a/test/binding.gyp b/test/binding.gyp index ced1a6802..c54437454 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -44,6 +44,7 @@ 'threadsafe_function/threadsafe_function.cc', 'typedarray.cc', 'objectwrap.cc', + 'objectwrap_constructor_exception.cc', 'objectreference.cc', 'version_management.cc', 'thunking_manual.cc', diff --git a/test/index.js b/test/index.js index e05c25a65..c4f8ef036 100644 --- a/test/index.js +++ b/test/index.js @@ -48,6 +48,7 @@ let testModules = [ 'typedarray', 'typedarray-bigint', 'objectwrap', + 'objectwrap_constructor_exception', 'objectreference', 'version_management' ]; diff --git a/test/objectwrap_constructor_exception.cc b/test/objectwrap_constructor_exception.cc new file mode 100644 index 000000000..d7e1bd517 --- /dev/null +++ b/test/objectwrap_constructor_exception.cc @@ -0,0 +1,26 @@ +#include + +class ConstructorExceptionTest : + public Napi::ObjectWrap { +public: + ConstructorExceptionTest(const Napi::CallbackInfo& info) : + Napi::ObjectWrap(info) { + Napi::Error error = Napi::Error::New(info.Env(), "an exception"); +#ifdef NAPI_DISABLE_CPP_EXCEPTIONS + error.ThrowAsJavaScriptException(); +#else + throw error; +#endif // NAPI_DISABLE_CPP_EXCEPTIONS + } + + static void Initialize(Napi::Env env, Napi::Object exports) { + const char* name = "ConstructorExceptionTest"; + exports.Set(name, DefineClass(env, name, {})); + } +}; + +Napi::Object InitObjectWrapConstructorException(Napi::Env env) { + Napi::Object exports = Napi::Object::New(env); + ConstructorExceptionTest::Initialize(env, exports); + return exports; +} diff --git a/test/objectwrap_constructor_exception.js b/test/objectwrap_constructor_exception.js new file mode 100644 index 000000000..8428c8034 --- /dev/null +++ b/test/objectwrap_constructor_exception.js @@ -0,0 +1,12 @@ +'use strict'; +const buildType = process.config.target_defaults.default_configuration; +const assert = require('assert'); + +const test = (binding) => { + const { ConstructorExceptionTest } = binding.objectwrapConstructorException; + assert.throws(() => (new ConstructorExceptionTest()), /an exception/); + global.gc(); +} + +test(require(`./build/${buildType}/binding.node`)); +test(require(`./build/${buildType}/binding_noexcept.node`)); From a552a384ddbbf2a6574d478cf3dc20cae87f0459 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 02:07:04 +0200 Subject: [PATCH 2/5] src: call `napi_remove_wrap()` in `ObjectWrap` dtor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when the `ObjectWrap` constructor runs, it calls `napi_wrap()`, adding a finalize callback to the freshly created JS object. However, if the `ObjectWrap` instance is prematurely deleted, for example because a subclass constructor throws – which seems like a reasonable scenario – that finalize callback was not removed, possibly leading to a use-after-free crash. This commit adds a call `napi_remove_wrap()` from the `ObjectWrap` destructor, and a test for that scenario. This also changes the code to use the correct pointer type in `FinalizeCallback`, which may not match the incorretct one in cases of multiple inheritance. Fixes: https://github.com/node-ffi-napi/weak-napi/issues/16 PR-URL: https://github.com/nodejs/node-addon-api/pull/475 Reviewed-By: Hitesh Kanwathirtha Reviewed-By: Gabriel Schulhof Reviewed-By: Tobias Nießen Reviewed-By: Michael Dawson Co-authored-by: Gabriel Schulhof --- napi-inl.h | 60 +++++++++-------------------------- test/binding.cc | 2 ++ test/binding.gyp | 1 + test/index.js | 1 + test/objectwrap-removewrap.cc | 45 ++++++++++++++++++++++++++ test/objectwrap-removewrap.js | 17 ++++++++++ test/objectwrap.js | 3 ++ 7 files changed, 84 insertions(+), 45 deletions(-) create mode 100644 test/objectwrap-removewrap.cc create mode 100644 test/objectwrap-removewrap.js diff --git a/napi-inl.h b/napi-inl.h index 151661058..ea3a8542b 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2666,37 +2666,6 @@ inline Object FunctionReference::New(const std::vector& args) const // CallbackInfo class //////////////////////////////////////////////////////////////////////////////// -class ObjectWrapConstructionContext { - public: - ObjectWrapConstructionContext(CallbackInfo* info) { - info->_objectWrapConstructionContext = this; - } - - static inline void SetObjectWrapped(const CallbackInfo& info) { - if (info._objectWrapConstructionContext == nullptr) { - Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped", - "_objectWrapConstructionContext is NULL"); - } - info._objectWrapConstructionContext->_objectWrapped = true; - } - - inline void Cleanup(const CallbackInfo& info) { - if (_objectWrapped) { - napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr); - - // There's already a pending exception if we are at this point, so we have - // no choice but to fatally fail here. - NAPI_FATAL_IF_FAILED(status, - "ObjectWrapConstructionContext::Cleanup", - "Failed to remove wrap from unsuccessfully " - "constructed ObjectWrap instance"); - } - } - - private: - bool _objectWrapped = false; -}; - inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info) : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) { _argc = _staticArgCount; @@ -3002,13 +2971,22 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); - ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo); Reference* instanceRef = this; *instanceRef = Reference(env, ref); } -template -inline ObjectWrap::~ObjectWrap() {} +template +inline ObjectWrap::~ObjectWrap() { + // If the JS object still exists at this point, remove the finalizer added + // through `napi_wrap()`. + if (!IsEmpty()) { + Object object = Value(); + // It is not valid to call `napi_remove_wrap()` with an empty `object`. + // This happens e.g. during garbage collection. + if (!object.IsEmpty()) + napi_remove_wrap(Env(), object, nullptr); + } +} template inline T* ObjectWrap::Unwrap(Object wrapper) { @@ -3402,23 +3380,15 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - ObjectWrapConstructionContext constructionContext(&callbackInfo); #ifdef NAPI_CPP_EXCEPTIONS - try { - new T(callbackInfo); - } catch (const Error& e) { - // Re-throw the error after removing the failed wrap. - constructionContext.Cleanup(callbackInfo); - throw e; - } + new T(callbackInfo); #else T* instance = new T(callbackInfo); if (callbackInfo.Env().IsExceptionPending()) { // We need to clear the exception so that removing the wrap might work. Error e = callbackInfo.Env().GetAndClearPendingException(); - constructionContext.Cleanup(callbackInfo); - e.ThrowAsJavaScriptException(); delete instance; + e.ThrowAsJavaScriptException(); } # endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); @@ -3545,7 +3515,7 @@ inline napi_value ObjectWrap::InstanceSetterCallbackWrapper( template inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* /*hint*/) { - T* instance = reinterpret_cast(data); + ObjectWrap* instance = static_cast*>(data); instance->Finalize(Napi::Env(env)); delete instance; } diff --git a/test/binding.cc b/test/binding.cc index 2ffdee367..ad27484fe 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -50,6 +50,7 @@ Object InitThreadSafeFunction(Env env); Object InitTypedArray(Env env); Object InitObjectWrap(Env env); Object InitObjectWrapConstructorException(Env env); +Object InitObjectWrapRemoveWrap(Env env); Object InitObjectReference(Env env); Object InitVersionManagement(Env env); Object InitThunkingManual(Env env); @@ -105,6 +106,7 @@ Object Init(Env env, Object exports) { exports.Set("objectwrap", InitObjectWrap(env)); exports.Set("objectwrapConstructorException", InitObjectWrapConstructorException(env)); + exports.Set("objectwrap_removewrap", InitObjectWrapRemoveWrap(env)); exports.Set("objectreference", InitObjectReference(env)); exports.Set("version_management", InitVersionManagement(env)); exports.Set("thunking_manual", InitThunkingManual(env)); diff --git a/test/binding.gyp b/test/binding.gyp index c54437454..d0be28ada 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -45,6 +45,7 @@ 'typedarray.cc', 'objectwrap.cc', 'objectwrap_constructor_exception.cc', + 'objectwrap-removewrap.cc', 'objectreference.cc', 'version_management.cc', 'thunking_manual.cc', diff --git a/test/index.js b/test/index.js index c4f8ef036..bff1708ca 100644 --- a/test/index.js +++ b/test/index.js @@ -49,6 +49,7 @@ let testModules = [ 'typedarray-bigint', 'objectwrap', 'objectwrap_constructor_exception', + 'objectwrap-removewrap', 'objectreference', 'version_management' ]; diff --git a/test/objectwrap-removewrap.cc b/test/objectwrap-removewrap.cc new file mode 100644 index 000000000..31a8c6870 --- /dev/null +++ b/test/objectwrap-removewrap.cc @@ -0,0 +1,45 @@ +#include +#include + +#ifdef NAPI_CPP_EXCEPTIONS +namespace { + +static int dtor_called = 0; + +class DtorCounter { + public: + ~DtorCounter() { + assert(dtor_called == 0); + dtor_called++; + } +}; + +Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) { + return Napi::Number::New(info.Env(), dtor_called); +} + +class Test : public Napi::ObjectWrap { +public: + Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) { + throw Napi::Error::New(Env(), "Some error"); + } + + static void Initialize(Napi::Env env, Napi::Object exports) { + exports.Set("Test", DefineClass(env, "Test", {})); + exports.Set("getDtorCalled", Napi::Function::New(env, GetDtorCalled)); + } + +private: + DtorCounter dtor_ounter_; +}; + +} // anonymous namespace +#endif // NAPI_CPP_EXCEPTIONS + +Napi::Object InitObjectWrapRemoveWrap(Napi::Env env) { + Napi::Object exports = Napi::Object::New(env); +#ifdef NAPI_CPP_EXCEPTIONS + Test::Initialize(env, exports); +#endif + return exports; +} diff --git a/test/objectwrap-removewrap.js b/test/objectwrap-removewrap.js new file mode 100644 index 000000000..fe7a8274a --- /dev/null +++ b/test/objectwrap-removewrap.js @@ -0,0 +1,17 @@ +'use strict'; +const buildType = process.config.target_defaults.default_configuration; +const assert = require('assert'); + +const test = (binding) => { + const Test = binding.objectwrap_removewrap.Test; + const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled; + + assert.strictEqual(getDtorCalled(), 0); + assert.throws(() => { + new Test(); + }); + assert.strictEqual(getDtorCalled(), 1); + global.gc(); // Does not crash. +} + +test(require(`./build/${buildType}/binding.node`)); diff --git a/test/objectwrap.js b/test/objectwrap.js index 533c05f75..56d106c06 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -222,6 +222,9 @@ const test = (binding) => { // `Test` is needed for accessing exposed symbols testObj(new Test(), Test); testClass(Test); + + // Make sure the C++ object can be garbage collected without issues. + setImmediate(global.gc); } test(require(`./build/${buildType}/binding.node`)); From 204f07252c695dc7f272caa548c7860ca7c1f5cb Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 29 Jan 2020 19:20:18 -0800 Subject: [PATCH 3/5] objectwrap: remove wrap only on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `napi_remove_wrap()` was intended for objects that are alive for which the native addon wishes to withdraw its native pointer, and perhaps replace it with another. Therefore we need not `napi_remove_wrap()` during gc/env-cleanup. It is sufficient to `napi_delete_reference()`, as `Reference` already does. We need only `napi_remove_wrap()` if the construction failed and therefore no gc callback will ever happen. This change also removes references to `ObjectWrapConstructionContext` from the header because the class is not used anymore. Fixes: https://github.com/nodejs/node-addon-api/issues/660 Reviewed-By: Tobias Nießen Reviewed-By: Chengzhong Wu Reviewed-By: Kevin Eady Reviewed-By: Michael Dawson --- napi-inl.h | 9 ++++++--- napi.h | 5 ++--- test/objectwrap-removewrap.cc | 10 +++++----- test/objectwrap-removewrap.js | 22 ++++++++++++++++++++-- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index ea3a8542b..df512e562 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2983,8 +2983,9 @@ inline ObjectWrap::~ObjectWrap() { Object object = Value(); // It is not valid to call `napi_remove_wrap()` with an empty `object`. // This happens e.g. during garbage collection. - if (!object.IsEmpty()) + if (!object.IsEmpty() && _construction_failed) { napi_remove_wrap(Env(), object, nullptr); + } } } @@ -3380,15 +3381,17 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); + T* instance = new T(callbackInfo); #ifdef NAPI_CPP_EXCEPTIONS - new T(callbackInfo); + instance->_construction_failed = false; #else - T* instance = new T(callbackInfo); if (callbackInfo.Env().IsExceptionPending()) { // We need to clear the exception so that removing the wrap might work. Error e = callbackInfo.Env().GetAndClearPendingException(); delete instance; e.ThrowAsJavaScriptException(); + } else { + instance->_construction_failed = false; } # endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); diff --git a/napi.h b/napi.h index 6a89d576f..83834b4a1 100644 --- a/napi.h +++ b/napi.h @@ -132,7 +132,6 @@ namespace Napi { template class Reference; class TypedArray; template class TypedArrayOf; - class ObjectWrapConstructionContext; typedef TypedArrayOf Int8Array; ///< Typed-array of signed 8-bit integers typedef TypedArrayOf Uint8Array; ///< Typed-array of unsigned 8-bit integers @@ -1378,7 +1377,6 @@ namespace Napi { class CallbackInfo { public: - friend class ObjectWrapConstructionContext; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1405,7 +1403,6 @@ namespace Napi { napi_value _staticArgs[6]; napi_value* _dynamicArgs; void* _data; - ObjectWrapConstructionContext* _objectWrapConstructionContext; }; class PropertyDescriptor { @@ -1738,6 +1735,8 @@ namespace Napi { StaticAccessorCallbackData; typedef AccessorCallbackData InstanceAccessorCallbackData; + + bool _construction_failed = true; }; class HandleScope { diff --git a/test/objectwrap-removewrap.cc b/test/objectwrap-removewrap.cc index 31a8c6870..fdcec07c7 100644 --- a/test/objectwrap-removewrap.cc +++ b/test/objectwrap-removewrap.cc @@ -1,7 +1,6 @@ #include #include -#ifdef NAPI_CPP_EXCEPTIONS namespace { static int dtor_called = 0; @@ -21,7 +20,11 @@ Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) { class Test : public Napi::ObjectWrap { public: Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) { +#ifdef NAPI_CPP_EXCEPTIONS throw Napi::Error::New(Env(), "Some error"); +#else + Napi::Error::New(Env(), "Some error").ThrowAsJavaScriptException(); +#endif } static void Initialize(Napi::Env env, Napi::Object exports) { @@ -30,16 +33,13 @@ class Test : public Napi::ObjectWrap { } private: - DtorCounter dtor_ounter_; + DtorCounter dtor_counter_; }; } // anonymous namespace -#endif // NAPI_CPP_EXCEPTIONS Napi::Object InitObjectWrapRemoveWrap(Napi::Env env) { Napi::Object exports = Napi::Object::New(env); -#ifdef NAPI_CPP_EXCEPTIONS Test::Initialize(env, exports); -#endif return exports; } diff --git a/test/objectwrap-removewrap.js b/test/objectwrap-removewrap.js index fe7a8274a..01c8b9169 100644 --- a/test/objectwrap-removewrap.js +++ b/test/objectwrap-removewrap.js @@ -1,8 +1,16 @@ 'use strict'; + +if (process.argv[2] === 'child') { + // Create a single wrapped instance then exit. + return new (require(process.argv[3]).objectwrap.Test)(); +} + const buildType = process.config.target_defaults.default_configuration; const assert = require('assert'); +const { spawnSync } = require('child_process'); -const test = (binding) => { +const test = (bindingName) => { + const binding = require(bindingName); const Test = binding.objectwrap_removewrap.Test; const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled; @@ -12,6 +20,16 @@ const test = (binding) => { }); assert.strictEqual(getDtorCalled(), 1); global.gc(); // Does not crash. + + // Start a child process that creates a single wrapped instance to ensure that + // it is properly freed at its exit. It must not segfault. + // Re: https://github.com/nodejs/node-addon-api/issues/660 + const child = spawnSync(process.execPath, [ + __filename, 'child', bindingName + ]); + assert.strictEqual(child.signal, null); + assert.strictEqual(child.status, 0); } -test(require(`./build/${buildType}/binding.node`)); +test(`./build/${buildType}/binding.node`); +test(`./build/${buildType}/binding_noexcept.node`); From 81e2eac7ba73d63cc160e303d5d1060e607f8399 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 15 Jan 2020 15:16:46 -0500 Subject: [PATCH 4/5] test: update BigInt test for recent change in core https://github.com/nodejs/node/commit/689ab46c646bc8add5560a606ba73d3760f8f924 changed the expected error for one of the BigInt test cases. This is ok because BigInt is still in experimental. However, the result was a failed/hanging test. See https://github.com/nodejs/build/issues/2131 This changes the test to accept either the new or old behaviour. We need that so that older versions of Node.js will also pass the test until the the node core commit above is backported. PR-URL: https://github.com/nodejs/node-addon-api/pull/649 Reviewed-By: Gabriel Schulhof Reviewed-By: Nicola Del Gobbo --- test/bigint.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/bigint.js b/test/bigint.js index e4255172c..0af867b43 100644 --- a/test/bigint.js +++ b/test/bigint.js @@ -46,7 +46,7 @@ function test(binding) { }); assert.throws(TestTooBigBigInt, { - name: 'RangeError', - message: 'Maximum BigInt size exceeded', + name: /^(RangeError|Error)$/, + message: /^(Maximum BigInt size exceeded|Invalid argument)$/, }); } From 470f1306663215d726333a653bd4d179205db11a Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 12 May 2020 13:50:02 -0700 Subject: [PATCH 5/5] objectwrap: avoid double-free on old Node.js On Node.js versions earlier than 10.15.3 references were being cleaned up differently. This has resulted in a segfault because of a double free when the `ObjectWrap` subclass constructor throws. We need a fix that only runs on Node.js < 10.15.3. We use `Napi::VersionManagement::GetNodeVersion()` at addon load time to determine whether we need the workaround. We store the result in an atomic boolean and consult its value whenever we destroy an `ObjectWrap` instance. --- napi-inl.h | 34 ++++++++++++++++++++---- test/objectwrap_constructor_exception.cc | 1 + 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index df512e562..db3e53885 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -10,6 +10,7 @@ // Note: Do not include this file directly! Include "napi.h" instead. #include +#include #include #include #include @@ -19,6 +20,8 @@ namespace Napi { // Helpers to handle functions exposed from C++. namespace details { +extern std::atomic_bool needs_objectwrap_destructor_fix; + // Attach a data item to an object and delete it when the object gets // garbage-collected. // TODO: Replace this code with `napi_add_finalizer()` whenever it becomes @@ -251,11 +254,16 @@ struct AccessorCallbackData { // Module registration //////////////////////////////////////////////////////////////////////////////// -#define NODE_API_MODULE(modname, regfunc) \ - napi_value __napi_ ## regfunc(napi_env env, \ - napi_value exports) { \ - return Napi::RegisterModule(env, exports, regfunc); \ - } \ +#define NODE_API_MODULE(modname, regfunc) \ + namespace Napi { \ + namespace details { \ + std::atomic_bool needs_objectwrap_destructor_fix(false); \ + } \ + } \ + napi_value __napi_ ## regfunc(napi_env env, \ + napi_value exports) { \ + return Napi::RegisterModule(env, exports, regfunc); \ + } \ NAPI_MODULE(modname, __napi_ ## regfunc) // Adapt the NAPI_MODULE registration function: @@ -264,6 +272,12 @@ struct AccessorCallbackData { inline napi_value RegisterModule(napi_env env, napi_value exports, ModuleRegisterCallback registerCallback) { + const napi_node_version* nver = Napi::VersionManagement::GetNodeVersion(env); + Napi::details::needs_objectwrap_destructor_fix = + (nver->major < 10 || + (nver->major == 10 && nver->minor < 15) || + (nver->major == 10 && nver->minor == 15 && nver->patch < 3)); + return details::WrapCallback([&] { return napi_value(registerCallback(Napi::Env(env), Napi::Object(env, exports))); @@ -2985,6 +2999,16 @@ inline ObjectWrap::~ObjectWrap() { // This happens e.g. during garbage collection. if (!object.IsEmpty() && _construction_failed) { napi_remove_wrap(Env(), object, nullptr); + + if (Napi::details::needs_objectwrap_destructor_fix) { + // If construction failed we delete the reference via + // `napi_remove_wrap()`, not via `napi_delete_reference()` in the + // `Reference` destructor. This will prevent the + // `Reference` destructor from doing a double delete of this + // reference. + _ref = nullptr; + _env = nullptr; + } } } } diff --git a/test/objectwrap_constructor_exception.cc b/test/objectwrap_constructor_exception.cc index d7e1bd517..015baa6a8 100644 --- a/test/objectwrap_constructor_exception.cc +++ b/test/objectwrap_constructor_exception.cc @@ -5,6 +5,7 @@ class ConstructorExceptionTest : public: ConstructorExceptionTest(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info) { + fprintf(stderr, "That was the interesting reference\n"); Napi::Error error = Napi::Error::New(info.Env(), "an exception"); #ifdef NAPI_DISABLE_CPP_EXCEPTIONS error.ThrowAsJavaScriptException();