From 048a37857e97acd0a7acd55ab49cc0d996dcb5b3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 02:07:04 +0200 Subject: [PATCH 1/4] 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. Fixes: https://github.com/node-ffi-napi/weak-napi/issues/16 --- napi-inl.h | 6 +++++ napi.h | 1 + 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 +++ 8 files changed, 76 insertions(+) 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 c15ed97a1..35071590a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2814,6 +2814,12 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { *instanceRef = Reference(env, ref); } +template +inline ObjectWrap::~ObjectWrap() { + if (!IsEmpty()) + napi_remove_wrap(Env(), Value(), nullptr); +} + template inline T* ObjectWrap::Unwrap(Object wrapper) { T* unwrapped; diff --git a/napi.h b/napi.h index 7a83d792e..a03958652 100644 --- a/napi.h +++ b/napi.h @@ -1569,6 +1569,7 @@ namespace Napi { class ObjectWrap : public Reference { public: ObjectWrap(const CallbackInfo& callbackInfo); + ~ObjectWrap(); static T* Unwrap(Object wrapper); diff --git a/test/binding.cc b/test/binding.cc index 4a5fec15c..292a16d43 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -35,6 +35,7 @@ Object InitObjectDeprecated(Env env); Object InitPromise(Env env); Object InitTypedArray(Env env); Object InitObjectWrap(Env env); +Object InitObjectWrapRemoveWrap(Env env); Object InitObjectReference(Env env); Object InitVersionManagement(Env env); Object InitThunkingManual(Env env); @@ -73,6 +74,7 @@ Object Init(Env env, Object exports) { exports.Set("promise", InitPromise(env)); exports.Set("typedarray", InitTypedArray(env)); exports.Set("objectwrap", InitObjectWrap(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 0e2b7d05c..f7fde9a5c 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -34,6 +34,7 @@ 'promise.cc', 'typedarray.cc', 'objectwrap.cc', + 'objectwrap-removewrap.cc', 'objectreference.cc', 'version_management.cc', 'thunking_manual.cc', diff --git a/test/index.js b/test/index.js index 67e630d7f..48778d13d 100644 --- a/test/index.js +++ b/test/index.js @@ -38,6 +38,7 @@ let testModules = [ 'typedarray', 'typedarray-bigint', 'objectwrap', + '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 1f888234d..0b67265f1 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -204,6 +204,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 277bf2bc137c250977c7c9c5f87f9ddc59ec9e43 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 02:36:38 +0200 Subject: [PATCH 2/4] fixup! src: call `napi_remove_wrap()` in `ObjectWrap` dtor --- napi-inl.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 35071590a..76844aa10 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2816,8 +2816,11 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { template inline ObjectWrap::~ObjectWrap() { - if (!IsEmpty()) - napi_remove_wrap(Env(), Value(), nullptr); + if (!IsEmpty()) { + Object object = Value(); + if (!object.IsEmpty()) + napi_remove_wrap(Env(), object, nullptr); + } } template From f4bfde2a3bd9dafdd5b3542de07f6b040ae10c1a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 02:34:05 +0200 Subject: [PATCH 3/4] src: make ObjectWrap dtor virtual Otherwise, subclasses of `ObjectWrap` would not be deleted correctly by the `delete instance;` line in `FinalizeCallback()`. (This is also just the right thing to do for classes from which subclasses are supposed to be created.) --- napi-inl.h | 7 +++---- napi.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 76844aa10..ab920a4cf 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2806,11 +2806,10 @@ 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; + Reference* instanceRef = this; *instanceRef = Reference(env, ref); } @@ -3338,7 +3337,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); delete instance; } diff --git a/napi.h b/napi.h index a03958652..f604867e4 100644 --- a/napi.h +++ b/napi.h @@ -1569,7 +1569,7 @@ namespace Napi { class ObjectWrap : public Reference { public: ObjectWrap(const CallbackInfo& callbackInfo); - ~ObjectWrap(); + virtual ~ObjectWrap(); static T* Unwrap(Object wrapper); From 0fd142bf1738c03b9f4b935dbc17f900a2e77d63 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 29 Apr 2019 14:59:37 +0200 Subject: [PATCH 4/4] fixup! fixup! src: call `napi_remove_wrap()` in `ObjectWrap` dtor --- napi-inl.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/napi-inl.h b/napi-inl.h index ab920a4cf..65142cffa 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2815,8 +2815,12 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { 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); }