src: fix usage of napi_extended_error_info in Error::New()

All fields of the `napi_extended_error_info` structure gets reset in subsequent Node-API function calls on the same `env`. This includes a call to `napi_is_exception_pending()`. So here it is necessary to make a copy of the information as the `error_code` field is used later on. Signed-off-by: Darshan Sen <[email protected]> PR-URL: #1092 Fixes: #1089 Reviewed-By: Michael Dawson <[email protected]>
nodejs · Oct 21, 2021 · 4663453 · 4663453
1 parent cb22841
commit 4663453
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 34 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -2520,12 +2520,23 @@ inline Error Error::New(napi_env env) {
   napi_status status;
   napi_value error = nullptr;
   bool is_exception_pending;
-  const napi_extended_error_info* info;
+  napi_extended_error_info last_error_info_copy;
 
-  // We must retrieve the last error info before doing anything else, because
-  // doing anything else will replace the last error info.
-  status = napi_get_last_error_info(env, &info);
-  NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
+  {
+    // We must retrieve the last error info before doing anything else because
+    // doing anything else will replace the last error info.
+    const napi_extended_error_info* last_error_info;
+    status = napi_get_last_error_info(env, &last_error_info);
+    NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");
+
+    // All fields of the `napi_extended_error_info` structure gets reset in
+    // subsequent Node-API function calls on the same `env`. This includes a
+    // call to `napi_is_exception_pending()`. So here it is necessary to make a
+    // copy of the information as the `error_code` field is used later on.
+    memcpy(&last_error_info_copy,
+           last_error_info,
+           sizeof(napi_extended_error_info));
+  }
 
   status = napi_is_exception_pending(env, &is_exception_pending);
   NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");
@@ -2536,8 +2547,9 @@ inline Error Error::New(napi_env env) {
     NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
   }
   else {
-    const char* error_message = info->error_message != nullptr ?
-      info->error_message : "Error in native callback";
+    const char* error_message = last_error_info_copy.error_message != nullptr
+                                    ? last_error_info_copy.error_message
+                                    : "Error in native callback";
 
     napi_value message;
     status = napi_create_string_utf8(
@@ -2547,16 +2559,16 @@ inline Error Error::New(napi_env env) {
       &message);
     NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_string_utf8");
 
-    switch (info->error_code) {
-    case napi_object_expected:
-    case napi_string_expected:
-    case napi_boolean_expected:
-    case napi_number_expected:
-      status = napi_create_type_error(env, nullptr, message, &error);
-      break;
-    default:
-      status = napi_create_error(env, nullptr,  message, &error);
-      break;
+    switch (last_error_info_copy.error_code) {
+      case napi_object_expected:
+      case napi_string_expected:
+      case napi_boolean_expected:
+      case napi_number_expected:
+        status = napi_create_type_error(env, nullptr, message, &error);
+        break;
+      default:
+        status = napi_create_error(env, nullptr, message, &error);
+        break;
     }
     NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_error");
   }

diff --git a/test/error.cc b/test/error.cc
@@ -59,6 +59,16 @@ void ThrowApiError(const CallbackInfo& info) {
   Function(info.Env(), nullptr).Call(std::initializer_list<napi_value>{});
 }
 
+void LastExceptionErrorCode(const CallbackInfo& info) {
+  // Previously, `napi_extended_error_info.error_code` got reset to `napi_ok` in
+  // subsequent Node-API function calls, so this would have previously thrown an
+  // `Error` object instead of a `TypeError` object.
+  Env env = info.Env();
+  bool res;
+  napi_get_value_bool(env, Value::From(env, "asd"), &res);
+  NAPI_THROW_VOID(Error::New(env));
+}
+
 #ifdef NAPI_CPP_EXCEPTIONS
 
 void ThrowJSError(const CallbackInfo& info) {
@@ -256,6 +266,8 @@ void ThrowDefaultError(const CallbackInfo& info) {
 Object InitError(Env env) {
   Object exports = Object::New(env);
   exports["throwApiError"] = Function::New(env, ThrowApiError);
+  exports["lastExceptionErrorCode"] =
+      Function::New(env, LastExceptionErrorCode);
   exports["throwJSError"] = Function::New(env, ThrowJSError);
   exports["throwTypeError"] = Function::New(env, ThrowTypeError);
   exports["throwRangeError"] = Function::New(env, ThrowRangeError);

diff --git a/test/error.js b/test/error.js
@@ -5,27 +5,30 @@ const assert = require('assert');
 if (process.argv[2] === 'fatal') {
   const binding = require(process.argv[3]);
   binding.error.throwFatalError();
-  return;
 }
 
 module.exports = require('./common').runTestWithBindingPath(test);
 
-function test(bindingPath) {
+function test (bindingPath) {
   const binding = require(bindingPath);
 
-  assert.throws(() => binding.error.throwApiError('test'), function(err) {
+  assert.throws(() => binding.error.throwApiError('test'), function (err) {
     return err instanceof Error && err.message.includes('Invalid');
   });
 
-  assert.throws(() => binding.error.throwJSError('test'), function(err) {
+  assert.throws(() => binding.error.lastExceptionErrorCode(), function (err) {
+    return err instanceof TypeError && err.message === 'A boolean was expected';
+  });
+
+  assert.throws(() => binding.error.throwJSError('test'), function (err) {
     return err instanceof Error && err.message === 'test';
   });
 
-  assert.throws(() => binding.error.throwTypeError('test'), function(err) {
+  assert.throws(() => binding.error.throwTypeError('test'), function (err) {
     return err instanceof TypeError && err.message === 'test';
   });
 
-  assert.throws(() => binding.error.throwRangeError('test'), function(err) {
+  assert.throws(() => binding.error.throwRangeError('test'), function (err) {
     return err instanceof RangeError && err.message === 'test';
   });
 
@@ -34,7 +37,7 @@ function test(bindingPath) {
       () => {
         throw new TypeError('test');
       }),
-    function(err) {
+    function (err) {
       return err instanceof TypeError && err.message === 'test' && !err.caught;
     });
 
@@ -43,7 +46,7 @@ function test(bindingPath) {
       () => {
         throw new TypeError('test');
       }),
-    function(err) {
+    function (err) {
       return err instanceof TypeError && err.message === 'test' && err.caught;
     });
 
@@ -56,19 +59,19 @@ function test(bindingPath) {
     () => { throw new TypeError('test'); });
   assert.strictEqual(msg, 'test');
 
-  assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function(err) {
+  assert.throws(() => binding.error.throwErrorThatEscapesScope('test'), function (err) {
     return err instanceof Error && err.message === 'test';
   });
 
-  assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function(err) {
+  assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), function (err) {
     return err instanceof Error && err.message === 'test' && err.caught;
   });
 
   const p = require('./napi_child').spawnSync(
-      process.execPath, [ __filename, 'fatal', bindingPath ]);
+    process.execPath, [__filename, 'fatal', bindingPath]);
   assert.ifError(p.error);
   assert.ok(p.stderr.toString().includes(
-      'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
+    'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
 
   assert.throws(() => binding.error.throwDefaultError(false),
     /Cannot convert undefined or null to object/);

diff --git a/test/run_script.js b/test/run_script.js
@@ -5,7 +5,7 @@ const testUtil = require('./testUtil');
 
 module.exports = require('./common').runTest(test);
 
-function test(binding) {
+function test (binding) {
   return testUtil.runGCTests([
     'Plain C string',
     () => {
@@ -21,7 +21,7 @@ function test(binding) {
 
     'JavaScript string',
     () => {
-      const sum = binding.run_script.jsString("1 + 2 + 3");
+      const sum = binding.run_script.jsString('1 + 2 + 3');
       assert.strictEqual(sum, 1 + 2 + 3);
     },
 
@@ -30,15 +30,15 @@ function test(binding) {
       assert.throws(() => {
         binding.run_script.jsString(true);
       }, {
-        name: 'Error',
+        name: 'TypeError',
         message: 'A string was expected'
       });
     },
 
     'With context',
     () => {
-      const a = 1, b = 2, c = 3;
-      const sum = binding.run_script.withContext("a + b + c", { a, b, c });
+      const a = 1; const b = 2; const c = 3;
+      const sum = binding.run_script.withContext('a + b + c', { a, b, c });
       assert.strictEqual(sum, a + b + c);
     }
   ]);