From 0b4027575216c3ceb5839d4699556b9a27814f88 Mon Sep 17 00:00:00 2001 From: Philipp Renoth Date: Fri, 28 Dec 2018 12:25:15 +0100 Subject: [PATCH] src: fix noexcept control flow issues - adapt `NAPI_THROW`, create `NAPI_THROW_VOID`: either throw or return, never continue - fix `Error::ThrowAsJavaScriptException`: if `napi_throw` fails, either explicitly throw or create fatal error Fixes: https://github.com/nodejs/node-addon-api/issues/419 PR-URL: https://github.com/nodejs/node-addon-api/pull/420 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson --- napi-inl.h | 59 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 261a261ee..aeb14ff3e 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -19,11 +19,15 @@ namespace details { #ifdef NAPI_CPP_EXCEPTIONS -#define NAPI_THROW(e) throw e - // When C++ exceptions are enabled, Errors are thrown directly. There is no need -// to return anything after the throw statement. The variadic parameter is an +// to return anything after the throw statements. The variadic parameter is an // optional return value that is ignored. +// We need _VOID versions of the macros to avoid warnings resulting from +// leaving the NAPI_THROW_* `...` argument empty. + +#define NAPI_THROW(e, ...) throw e +#define NAPI_THROW_VOID(e) throw e + #define NAPI_THROW_IF_FAILED(env, status, ...) \ if ((status) != napi_ok) throw Error::New(env); @@ -32,19 +36,30 @@ namespace details { #else // NAPI_CPP_EXCEPTIONS -#define NAPI_THROW(e) (e).ThrowAsJavaScriptException(); - // When C++ exceptions are disabled, Errors are thrown as JavaScript exceptions, // which are pending until the callback returns to JS. The variadic parameter // is an optional return value; usually it is an empty result. +// We need _VOID versions of the macros to avoid warnings resulting from +// leaving the NAPI_THROW_* `...` argument empty. + +#define NAPI_THROW(e, ...) \ + do { \ + (e).ThrowAsJavaScriptException(); \ + return __VA_ARGS__; \ + } while (0) + +#define NAPI_THROW_VOID(e) \ + do { \ + (e).ThrowAsJavaScriptException(); \ + return; \ + } while (0) + #define NAPI_THROW_IF_FAILED(env, status, ...) \ if ((status) != napi_ok) { \ Error::New(env).ThrowAsJavaScriptException(); \ return __VA_ARGS__; \ } -// We need a _VOID version of this macro to avoid warnings resulting from -// leaving the NAPI_THROW_IF_FAILED `...` argument empty. #define NAPI_THROW_IF_FAILED_VOID(env, status) \ if ((status) != napi_ok) { \ Error::New(env).ThrowAsJavaScriptException(); \ @@ -1312,8 +1327,8 @@ inline DataView DataView::New(napi_env env, size_t byteOffset) { if (byteOffset > arrayBuffer.ByteLength()) { NAPI_THROW(RangeError::New(env, - "Start offset is outside the bounds of the buffer")); - return DataView(); + "Start offset is outside the bounds of the buffer"), + DataView()); } return New(env, arrayBuffer, byteOffset, arrayBuffer.ByteLength() - byteOffset); @@ -1324,8 +1339,8 @@ inline DataView DataView::New(napi_env env, size_t byteOffset, size_t byteLength) { if (byteOffset + byteLength > arrayBuffer.ByteLength()) { - NAPI_THROW(RangeError::New(env, "Invalid DataView length")); - return DataView(); + NAPI_THROW(RangeError::New(env, "Invalid DataView length"), + DataView()); } napi_value value; napi_status status = napi_create_dataview( @@ -1451,8 +1466,7 @@ inline T DataView::ReadData(size_t byteOffset) const { if (byteOffset + sizeof(T) > _length || byteOffset + sizeof(T) < byteOffset) { // overflow NAPI_THROW(RangeError::New(_env, - "Offset is outside the bounds of the DataView")); - return 0; + "Offset is outside the bounds of the DataView"), 0); } return *reinterpret_cast(static_cast(_data) + byteOffset); @@ -1462,9 +1476,8 @@ template inline void DataView::WriteData(size_t byteOffset, T value) const { if (byteOffset + sizeof(T) > _length || byteOffset + sizeof(T) < byteOffset) { // overflow - NAPI_THROW(RangeError::New(_env, + NAPI_THROW_VOID(RangeError::New(_env, "Offset is outside the bounds of the DataView")); - return; } *reinterpret_cast(static_cast(_data) + byteOffset) = value; @@ -1600,7 +1613,7 @@ inline TypedArrayOf::TypedArrayOf(napi_env env, : TypedArray(env, value, type, length), _data(data) { if (!(type == TypedArrayTypeForPrimitiveType() || (type == napi_uint8_clamped_array && std::is_same::value))) { - NAPI_THROW(TypeError::New(env, "Array type must match the template parameter. " + NAPI_THROW_VOID(TypeError::New(env, "Array type must match the template parameter. " "(Uint8 arrays may optionally have the \"clamped\" array type.)")); } } @@ -2034,8 +2047,20 @@ inline const std::string& Error::Message() const NAPI_NOEXCEPT { inline void Error::ThrowAsJavaScriptException() const { HandleScope scope(_env); if (!IsEmpty()) { + + // We intentionally don't use `NAPI_THROW_*` macros here to ensure + // that there is no possible recursion as `ThrowAsJavaScriptException` + // is part of `NAPI_THROW_*` macro definition for noexcept. + napi_status status = napi_throw(_env, Value()); - NAPI_THROW_IF_FAILED_VOID(_env, status); + +#ifdef NAPI_CPP_EXCEPTIONS + if (status != napi_ok) { + throw Error::New(_env); + } +#else // NAPI_CPP_EXCEPTIONS + NAPI_FATAL_IF_FAILED(status, "Error::ThrowAsJavaScriptException", "napi_throw"); +#endif // NAPI_CPP_EXCEPTIONS } }