From 683bfcce763dd0bc706c2939f1e0ed9f3f0d48e6 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof 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/ --- napi-inl.h | 51 ++++++++++++++++++++++++ napi.h | 4 ++ 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, 105 insertions(+) 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..1a491ecb8 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2702,6 +2702,38 @@ inline Object FunctionReference::New(const std::vector& args) const // CallbackInfo class //////////////////////////////////////////////////////////////////////////////// +class ObjectWrapCleanup { + public: + ObjectWrapCleanup(CallbackInfo* info): _info(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; + } + + void RemoveWrap() { + if (_wrap_worked) { + napi_value wrapper = _info->This(); + napi_status status = napi_remove_wrap(_info->Env(), + wrapper, + nullptr); + NAPI_FATAL_IF_FAILED(status, + "~ObjectWrapCleanup", + "Failed to remove wrap from unsuccessfully " + "constructed ObjectWrap instance"); + } + } + + private: + bool _wrap_worked = false; + CallbackInfo* _info = nullptr; +}; + inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info) : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) { _argc = _staticArgCount; @@ -3110,6 +3142,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); } @@ -3686,7 +3719,25 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( T* instance; napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); + ObjectWrapCleanup cleanup(&callbackInfo); +#ifdef NAPI_CPP_EXCEPTIONS + try { + instance = new T(callbackInfo); + } catch (const Error& e) { + // re-throw the error after removing the failed wrap. + cleanup.RemoveWrap(); + throw e; + } +#else 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(); + e.ThrowAsJavaScriptException(); + delete instance; + } +# endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); }); diff --git a/napi.h b/napi.h index 57057c53a..5fb133054 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,8 @@ 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`));