From 85435af17189515e9fa2dd0ecd7aad82a5428f03 Mon Sep 17 00:00:00 2001 From: Kenvin Davies Date: Wed, 13 Nov 2019 17:34:09 -0800 Subject: [PATCH] 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 2250b6b..5367a0b 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 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; @@ -3106,11 +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); - Reference* instanceRef = instance; + ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo); + Reference* instanceRef = this; *instanceRef = Reference(env, ref); } @@ -3683,10 +3714,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 57057c5..61375b7 100644 --- a/napi.h +++ b/napi.h @@ -138,6 +138,7 @@ namespace Napi { class CallbackInfo; 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 @@ -1402,6 +1403,7 @@ namespace Napi { class CallbackInfo { public: + friend class ObjectWrapConstructionContext; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1427,6 +1429,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 ad46508..bfed9ae 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 97c4789..a84ab07 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 4fffd1d..5f8bdea 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 0000000..d7e1bd5 --- /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 0000000..8428c80 --- /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`));