From 8cf3755bff2f567ebd44e03c83232f6d94c3acd7 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 13 Nov 2019 17:34:09 -0800 Subject: [PATCH 1/4] 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/ --- napi-inl.h | 53 +++++++++++++++++++++++- 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 | 19 +++++++++ 7 files changed, 104 insertions(+), 2 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 2250b6b1c..c75ce6c03 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2702,6 +2702,37 @@ inline Object FunctionReference::New(const std::vector& args) const // CallbackInfo class //////////////////////////////////////////////////////////////////////////////// +class ObjectWrapCleanup { + public: + ObjectWrapCleanup(CallbackInfo* info) { + info->_object_wrap_cleanup = this; + } + + static inline void MarkWrapOK(const CallbackInfo& info) { + if (info._object_wrap_cleanup == nullptr) { + Napi::Error::Fatal("ObjectWrapCleanup::MarkWrapOK", + "_object_wrap_cleanup is NULL"); + } + info._object_wrap_cleanup->_wrap_worked = true; + } + + inline void RemoveWrap(const CallbackInfo& info) { + if (_wrap_worked) { + 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, + "ObjectWrapCleanup::RemoveWrap", + "Failed to remove wrap from unsuccessfully " + "constructed ObjectWrap instance"); + } + } + + private: + bool _wrap_worked = false; +}; + inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info) : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) { _argc = _staticArgCount; @@ -3110,6 +3141,7 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); + ObjectWrapCleanup::MarkWrapOK(callbackInfo); Reference* instanceRef = instance; *instanceRef = Reference(env, ref); } @@ -3683,10 +3715,27 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( return nullptr; } - T* instance; napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - instance = new T(callbackInfo); + ObjectWrapCleanup cleanup(&callbackInfo); +#ifdef NAPI_CPP_EXCEPTIONS + try { + new T(callbackInfo); + } catch (const Error& e) { + // re-throw the error after removing the failed wrap. + cleanup.RemoveWrap(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(); + cleanup.RemoveWrap(callbackInfo); + e.ThrowAsJavaScriptException(); + delete instance; + } +# endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); }); diff --git a/napi.h b/napi.h index 57057c53a..876b14b64 100644 --- a/napi.h +++ b/napi.h @@ -138,6 +138,7 @@ namespace Napi { class CallbackInfo; class TypedArray; template class TypedArrayOf; + class ObjectWrapCleanup; typedef TypedArrayOf Int8Array; ///< Typed-array of signed 8-bit integers typedef TypedArrayOf Uint8Array; ///< Typed-array of unsigned 8-bit integers @@ -1402,6 +1403,7 @@ namespace Napi { class CallbackInfo { public: + friend class ObjectWrapCleanup; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1427,6 +1429,7 @@ namespace Napi { napi_value _staticArgs[6]; napi_value* _dynamicArgs; void* _data; + ObjectWrapCleanup* _object_wrap_cleanup; }; class PropertyDescriptor { diff --git a/test/binding.cc b/test/binding.cc index ad4650819..bfed9ae5f 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -50,6 +50,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); @@ -104,6 +105,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 97c47899e..a84ab073d 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -41,6 +41,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 4fffd1d84..5f8bdea90 100644 --- a/test/index.js +++ b/test/index.js @@ -49,6 +49,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..db64e2e05 --- /dev/null +++ b/test/objectwrap_constructor_exception.js @@ -0,0 +1,19 @@ +'use strict'; +const buildType = process.config.target_defaults.default_configuration; +const assert = require('assert'); + +const test = (binding) => { + const { ConstructorExceptionTest } = binding.objectwrapConstructorException; + let gotException = false; + try { + new ConstructorExceptionTest(); + } catch (anException) { + gotException = true; + } + global.gc(); + + assert.strictEqual(gotException, true); +} + +test(require(`./build/${buildType}/binding.node`)); +test(require(`./build/${buildType}/binding_noexcept.node`)); From b00638cf428cd6dd0bea7a4076cb98d3c25e3452 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 31 Dec 2019 16:57:18 -0800 Subject: [PATCH 2/4] remove static cast in constructor --- napi-inl.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index c75ce6c03..f51c4a790 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3137,12 +3137,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); ObjectWrapCleanup::MarkWrapOK(callbackInfo); - Reference* instanceRef = instance; + Reference* instanceRef = this; *instanceRef = Reference(env, ref); } @@ -3722,7 +3721,7 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( try { new T(callbackInfo); } catch (const Error& e) { - // re-throw the error after removing the failed wrap. + // Re-throw the error after removing the failed wrap. cleanup.RemoveWrap(callbackInfo); throw e; } From 868b4bd1f003897e9f086a9adb75182041619964 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 2 Jan 2020 12:40:59 -0800 Subject: [PATCH 3/4] perform some renames to better reflect semantics --- napi-inl.h | 32 ++++++++++++------------ napi.h | 6 ++--- test/objectwrap_constructor_exception.js | 9 +------ 3 files changed, 20 insertions(+), 27 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index f51c4a790..5367a0bec 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2702,35 +2702,35 @@ inline Object FunctionReference::New(const std::vector& args) const // CallbackInfo class //////////////////////////////////////////////////////////////////////////////// -class ObjectWrapCleanup { +class ObjectWrapConstructionContext { public: - ObjectWrapCleanup(CallbackInfo* info) { - info->_object_wrap_cleanup = this; + ObjectWrapConstructionContext(CallbackInfo* info) { + info->_objectWrapConstructionContext = this; } - static inline void MarkWrapOK(const CallbackInfo& info) { - if (info._object_wrap_cleanup == nullptr) { - Napi::Error::Fatal("ObjectWrapCleanup::MarkWrapOK", - "_object_wrap_cleanup is NULL"); + static inline void SetObjectWrapped(const CallbackInfo& info) { + if (info._objectWrapConstructionContext == nullptr) { + Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped", + "_objectWrapConstructionContext is NULL"); } - info._object_wrap_cleanup->_wrap_worked = true; + info._objectWrapConstructionContext->_objectWrapped = true; } - inline void RemoveWrap(const CallbackInfo& info) { - if (_wrap_worked) { + 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, - "ObjectWrapCleanup::RemoveWrap", + "ObjectWrapConstructionContext::Cleanup", "Failed to remove wrap from unsuccessfully " "constructed ObjectWrap instance"); } } private: - bool _wrap_worked = false; + bool _objectWrapped = false; }; inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info) @@ -3140,7 +3140,7 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); - ObjectWrapCleanup::MarkWrapOK(callbackInfo); + ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo); Reference* instanceRef = this; *instanceRef = Reference(env, ref); } @@ -3716,13 +3716,13 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - ObjectWrapCleanup cleanup(&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. - cleanup.RemoveWrap(callbackInfo); + constructionContext.Cleanup(callbackInfo); throw e; } #else @@ -3730,7 +3730,7 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( if (callbackInfo.Env().IsExceptionPending()) { // We need to clear the exception so that removing the wrap might work. Error e = callbackInfo.Env().GetAndClearPendingException(); - cleanup.RemoveWrap(callbackInfo); + constructionContext.Cleanup(callbackInfo); e.ThrowAsJavaScriptException(); delete instance; } diff --git a/napi.h b/napi.h index 876b14b64..61375b7c6 100644 --- a/napi.h +++ b/napi.h @@ -138,7 +138,7 @@ namespace Napi { class CallbackInfo; class TypedArray; template class TypedArrayOf; - class ObjectWrapCleanup; + class ObjectWrapConstructionContext; typedef TypedArrayOf Int8Array; ///< Typed-array of signed 8-bit integers typedef TypedArrayOf Uint8Array; ///< Typed-array of unsigned 8-bit integers @@ -1403,7 +1403,7 @@ namespace Napi { class CallbackInfo { public: - friend class ObjectWrapCleanup; + friend class ObjectWrapConstructionContext; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1429,7 +1429,7 @@ namespace Napi { napi_value _staticArgs[6]; napi_value* _dynamicArgs; void* _data; - ObjectWrapCleanup* _object_wrap_cleanup; + ObjectWrapConstructionContext* _objectWrapConstructionContext; }; class PropertyDescriptor { diff --git a/test/objectwrap_constructor_exception.js b/test/objectwrap_constructor_exception.js index db64e2e05..1615ed363 100644 --- a/test/objectwrap_constructor_exception.js +++ b/test/objectwrap_constructor_exception.js @@ -4,15 +4,8 @@ const assert = require('assert'); const test = (binding) => { const { ConstructorExceptionTest } = binding.objectwrapConstructorException; - let gotException = false; - try { - new ConstructorExceptionTest(); - } catch (anException) { - gotException = true; - } + assert.throws(() => (new ConstructorExceptionTest())); global.gc(); - - assert.strictEqual(gotException, true); } test(require(`./build/${buildType}/binding.node`)); From bd465dea41f2b3c13e832441dfd757f0fc193241 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 9 Jan 2020 15:15:40 -0800 Subject: [PATCH 4/4] test for a specific exception --- test/objectwrap_constructor_exception.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/objectwrap_constructor_exception.js b/test/objectwrap_constructor_exception.js index 1615ed363..8428c8034 100644 --- a/test/objectwrap_constructor_exception.js +++ b/test/objectwrap_constructor_exception.js @@ -4,7 +4,7 @@ const assert = require('assert'); const test = (binding) => { const { ConstructorExceptionTest } = binding.objectwrapConstructorException; - assert.throws(() => (new ConstructorExceptionTest())); + assert.throws(() => (new ConstructorExceptionTest()), /an exception/); global.gc(); }