From a552a384ddbbf2a6574d478cf3dc20cae87f0459 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 02:07:04 +0200 Subject: [PATCH] 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`));