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.

Fixes: node-ffi-napi/weak-napi#16

napi-inl.h

@@ -2814,6 +2814,12 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   *instanceRef = Reference<Object>(env, ref);
 }
 
+template <typename T>
+inline ObjectWrap<T>::~ObjectWrap() {
+  if (!IsEmpty())
+    napi_remove_wrap(Env(), Value(), nullptr);
+}
+
 template<typename T>
 inline T* ObjectWrap<T>::Unwrap(Object wrapper) {
   T* unwrapped;

napi.h

@@ -1569,6 +1569,7 @@ namespace Napi {
   class ObjectWrap : public Reference<Object> {
   public:
     ObjectWrap(const CallbackInfo& callbackInfo);
+    ~ObjectWrap();
 
     static T* Unwrap(Object wrapper);
 

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

test/binding.gyp

@@ -34,6 +34,7 @@
         'promise.cc',
         'typedarray.cc',
         'objectwrap.cc',
+        'objectwrap-removewrap.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

test/index.js

@@ -38,6 +38,7 @@ let testModules = [
   'typedarray',
   'typedarray-bigint',
   'objectwrap',
+  '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

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