From 6f2bf7b59f2523eef4044f3e40aed808400503f3 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 17 Oct 2021 13:55:01 +0530 Subject: [PATCH] src: fix usage of `napi_extended_error_info` in `Error::New()` All fields of the `napi_extended_error_info` structure, except `error_message`, 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. Fixes: https://github.com/nodejs/node-addon-api/issues/1089 Signed-off-by: Darshan Sen --- napi-inl.h | 47 +++++++++++++++++++++++++++++----------------- test/error.cc | 12 ++++++++++++ test/error.js | 27 ++++++++++++++------------ test/run_script.js | 10 +++++----- 4 files changed, 62 insertions(+), 34 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 19aaab447..e20a0d419 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2520,12 +2520,24 @@ 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, except + // `error_message`, 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 +2548,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 +2560,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 index 15858182e..5b470a957 100644 --- 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{}); } +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 index 763115c64..d1519ec8e 100644 --- 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 index 711ab1035..81b5879bd 100644 --- 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); } ]);