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<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: #599 Co-authored-by: blagoev <[email protected]> PR-URL: #600
nodejs · Dec 31, 2019 · 683bfcc · 683bfcc
1 parent 5eeabb0
commit 683bfcc
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 0 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -2702,6 +2702,38 @@ inline Object FunctionReference::New(const std::vector<napi_value>& 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<T>::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<Object>* instanceRef = instance;
   *instanceRef = Reference<Object>(env, ref);
 }
@@ -3686,7 +3719,25 @@ inline napi_value ObjectWrap<T>::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
@@ -138,6 +138,7 @@ namespace Napi {
   class CallbackInfo;
   class TypedArray;
   template <typename T> class TypedArrayOf;
+  class ObjectWrapCleanup;
 
   typedef TypedArrayOf<int8_t> Int8Array;     ///< Typed-array of signed 8-bit integers
   typedef TypedArrayOf<uint8_t> 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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,26 @@
+#include <napi.h>
+
+class ConstructorExceptionTest :
+  public Napi::ObjectWrap<ConstructorExceptionTest> {
+public:
+  ConstructorExceptionTest(const Napi::CallbackInfo& info) :
+    Napi::ObjectWrap<ConstructorExceptionTest>(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
@@ -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`));