From 9347e10d6ec28ae60aab2ac10148fdf6562245f4 Mon Sep 17 00:00:00 2001
From: Gabriel Schulhof <gabriel.schulhof@intel.com>
Date: Wed, 13 Nov 2019 17:34:09 -0800
Subject: [PATCH] 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.

Fixes: https://github.com/nodejs/node-addon-api/issues/599
---
 napi-inl.h                               | 21 +++++++++++++++++++
 test/binding.cc                          |  3 +++
 test/binding.gyp                         |  1 +
 test/index.js                            |  1 +
 test/objectwrap_constructor_exception.cc | 26 ++++++++++++++++++++++++
 test/objectwrap_constructor_exception.js | 19 +++++++++++++++++
 6 files changed, 71 insertions(+)
 create mode 100644 test/objectwrap_constructor_exception.cc
 create mode 100644 test/objectwrap_constructor_exception.js

diff --git a/napi-inl.h b/napi-inl.h
index 98c578ed1..4232287f9 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -3370,9 +3370,30 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
   }
 
   T* instance;
+  auto handleConstructorException = [](const CallbackInfo& info) {
+    napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr);
+    NAPI_FATAL_IF_FAILED(status,
+        "ObjectWrap<T>::ConstructorCallbackWrapper",
+        "Failed to remove wrap from failed ObjectWrap instance construction");
+  };
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
+#ifdef NAPI_CPP_EXCEPTIONS
+    try {
+      instance = new T(callbackInfo);
+    } catch (const Error& e) {
+      handleConstructorException(callbackInfo);
+      e.ThrowAsJavaScriptException();
+    }
+#else
     instance = new T(callbackInfo);
+    if (callbackInfo.Env().IsExceptionPending()) {
+      Error e = callbackInfo.Env().GetAndClearPendingException();
+      handleConstructorException(callbackInfo);
+      e.ThrowAsJavaScriptException();
+      delete instance;
+    }
+# endif  // NAPI_CPP_EXCEPTIONS
     return callbackInfo.This();
   });
 
diff --git a/test/binding.cc b/test/binding.cc
index 0999ec7c4..ec4a3645a 100644
--- a/test/binding.cc
+++ b/test/binding.cc
@@ -48,6 +48,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);
@@ -100,6 +101,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
index a21ef8e95..69f1256e5 100644
--- a/test/binding.gyp
+++ b/test/binding.gyp
@@ -43,6 +43,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
index 2269aed8a..ee1095942 100644
--- a/test/index.js
+++ b/test/index.js
@@ -47,6 +47,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
new file mode 100644
index 000000000..d7e1bd517
--- /dev/null
+++ 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
new file mode 100644
index 000000000..db64e2e05
--- /dev/null
+++ 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`));