src: call napi_remove_wrap() in ObjectWrap dtor

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: node-ffi-napi/weak-napi#16
PR-URL: #475
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Co-authored-by: Gabriel Schulhof <[email protected]>

Author: addaleax

napi-inl.h

@@ -2702,37 +2702,6 @@ inline Object FunctionReference::New(const std::vector<napi_value>& 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;
@@ -3140,13 +3109,22 @@ inline ObjectWrap<T>::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<Object>* instanceRef = this;
   *instanceRef = Reference<Object>(env, ref);
 }
 
-template<typename T>
-inline ObjectWrap<T>::~ObjectWrap() {}
+template <typename T>
+inline ObjectWrap<T>::~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<typename T>
 inline T* ObjectWrap<T>::Unwrap(Object wrapper) {
@@ -3716,23 +3694,15 @@ inline napi_value ObjectWrap<T>::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();
@@ -3859,7 +3829,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 
 template <typename T>
 inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
-  T* instance = reinterpret_cast<T*>(data);
+  ObjectWrap<T>* instance = static_cast<ObjectWrap<T>*>(data);
   instance->Finalize(Napi::Env(env));
   delete instance;
 }

test/binding.cc

@@ -51,6 +51,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);
@@ -107,6 +108,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));

test/binding.gyp

@@ -42,6 +42,7 @@
         'typedarray.cc',
         'objectwrap.cc',
         'objectwrap_constructor_exception.cc',
+        'objectwrap-removewrap.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

test/index.js

@@ -50,6 +50,7 @@ let testModules = [
   'typedarray-bigint',
   'objectwrap',
   'objectwrap_constructor_exception',
+  'objectwrap-removewrap',
   'objectreference',
   'version_management'
 ];

test/objectwrap-removewrap.cc

@@ -0,0 +1,45 @@
+#include <napi.h>
+#include <assert.h>
+
+#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<Test> {
+public:
+  Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Test>(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;
+}

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`));

test/objectwrap.js

@@ -273,6 +273,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`));