From 10d80a3f96d4fb64c2e052082ee112b84c3f2fb9 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Wed, 22 Nov 2023 14:30:27 +0800 Subject: [PATCH 1/4] src: fix backtrace with tail [[noreturn]] abort A function tail calls [[noreturn]] node::Abort will print an incorrect call stack because the frame pc was advanced when calling node::Abort to an invalid op, which may vary on different platforms. Dumps the backtrace in the ABORT macro instead to avoid calling backtrace in a tail [[noreturn]] call. Removes the [[noreturn]] attribute if a function calls backtrace and may be called as a tail statement. [[noreturn]] attribute of public functions like `napi_fatal_error` and `node::OnFatalError` can not be removed as compilers may complain about no return values after the removal. --- src/node_errors.cc | 20 ++++++-------------- src/node_errors.h | 6 ++++-- src/node_process_methods.cc | 2 +- src/util.h | 30 +++++++++++++++++++----------- 4 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index 9c4ebbe4502698..4dfecbaa5a94f7 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -376,14 +376,7 @@ void AppendExceptionLine(Environment* env, .FromMaybe(false)); } -[[noreturn]] void Abort() { - DumpNativeBacktrace(stderr); - DumpJavaScriptBacktrace(stderr); - fflush(stderr); - ABORT_NO_BACKTRACE(); -} - -[[noreturn]] void Assert(const AssertionInfo& info) { +void Assert(const AssertionInfo& info) { std::string name = GetHumanReadableProcessName(); fprintf(stderr, @@ -396,7 +389,7 @@ void AppendExceptionLine(Environment* env, info.message); fflush(stderr); - Abort(); + ABORT(); } enum class EnhanceFatalException { kEnhance, kDontEnhance }; @@ -404,7 +397,7 @@ enum class EnhanceFatalException { kEnhance, kDontEnhance }; /** * Report the exception to the inspector, then print it to stderr. * This should only be used when the Node.js instance is about to exit - * (i.e. this should be followed by a env->Exit() or an Abort()). + * (i.e. this should be followed by a env->Exit() or an ABORT()). * * Use enhance_stack = EnhanceFatalException::kDontEnhance * when it's unsafe to call into JavaScript. @@ -576,8 +569,7 @@ static void ReportFatalException(Environment* env, ABORT(); } -[[noreturn]] void OOMErrorHandler(const char* location, - const v8::OOMDetails& details) { +void OOMErrorHandler(const char* location, const v8::OOMDetails& details) { // We should never recover from this handler so once it's true it's always // true. is_in_oom.store(true); @@ -1063,7 +1055,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo& args) { if (env != nullptr && env->abort_on_uncaught_exception()) { ReportFatalException( env, exception, message, EnhanceFatalException::kEnhance); - Abort(); + ABORT(); } bool from_promise = args[1]->IsTrue(); errors::TriggerUncaughtException(isolate, exception, message, from_promise); @@ -1174,7 +1166,7 @@ void TriggerUncaughtException(Isolate* isolate, // much we can do, so we just print whatever is useful and crash. PrintToStderrAndFlush( FormatCaughtException(isolate, context, error, message)); - Abort(); + ABORT(); } // Invoke process._fatalException() to give user a chance to handle it. diff --git a/src/node_errors.h b/src/node_errors.h index 927d92972b1dd8..ff4993224eb500 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -19,9 +19,11 @@ void AppendExceptionLine(Environment* env, v8::Local message, enum ErrorHandlingMode mode); +// This function calls backtrace, it should have not be marked as [[noreturn]]. +// But it is a public API, removing the attribute can break. [[noreturn]] void OnFatalError(const char* location, const char* message); -[[noreturn]] void OOMErrorHandler(const char* location, - const v8::OOMDetails& details); +// This function calls backtrace, do not mark as [[noreturn]]. +void OOMErrorHandler(const char* location, const v8::OOMDetails& details); // Helpers to construct errors similar to the ones provided by // lib/internal/errors.js. diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 0342658c35ebdb..cdbe07947b01d1 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -64,7 +64,7 @@ Mutex umask_mutex; #define NANOS_PER_SEC 1000000000 static void Abort(const FunctionCallbackInfo& args) { - Abort(); + ABORT(); } // For internal testing only, not exposed to userland. diff --git a/src/util.h b/src/util.h index 28873dbe4024df..7588bc6f604df6 100644 --- a/src/util.h +++ b/src/util.h @@ -113,8 +113,9 @@ struct AssertionInfo { const char* message; const char* function; }; -[[noreturn]] void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info); -[[noreturn]] void NODE_EXTERN_PRIVATE Abort(); + +// This indirectly calls backtrace so it can not be marked as [[noreturn]]. +void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info); void DumpNativeBacktrace(FILE* fp); void DumpJavaScriptBacktrace(FILE* fp); @@ -125,16 +126,23 @@ void DumpJavaScriptBacktrace(FILE* fp); #define ABORT_NO_BACKTRACE() abort() #endif -#define ABORT() node::Abort() +#define ABORT() \ + do { \ + node::DumpNativeBacktrace(stderr); \ + node::DumpJavaScriptBacktrace(stderr); \ + fflush(stderr); \ + ABORT_NO_BACKTRACE(); \ + } while (0) -#define ERROR_AND_ABORT(expr) \ - do { \ - /* Make sure that this struct does not end up in inline code, but */ \ - /* rather in a read-only data section when modifying this code. */ \ - static const node::AssertionInfo args = { \ - __FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME \ - }; \ - node::Assert(args); \ +#define ERROR_AND_ABORT(expr) \ + do { \ + /* Make sure that this struct does not end up in inline code, but */ \ + /* rather in a read-only data section when modifying this code. */ \ + static const node::AssertionInfo args = { \ + __FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME}; \ + node::Assert(args); \ + /* Mark the macro as [[noreturn]]. */ \ + ABORT_NO_BACKTRACE(); \ } while (0) #ifdef __GNUC__ From 090a9a4b34e925a5748c692d7c6e970ffbb6076c Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 28 Nov 2023 22:16:45 +0800 Subject: [PATCH 2/4] fixup! src: fix backtrace with [[noreturn]] abort --- src/api/environment.cc | 2 +- src/inspector_agent.cc | 6 +----- src/node_errors.h | 5 ++++- src/node_watchdog.cc | 2 +- src/util.h | 9 ++++++++- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index a37a1b01fffb2a..7e30a35ad7e611 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -734,7 +734,7 @@ Maybe InitializeContextRuntime(Local context) { } } else if (per_process::cli_options->disable_proto != "") { // Validated in ProcessGlobalArgs - OnFatalError("InitializeContextRuntime()", "invalid --disable-proto mode"); + UNREACHABLE("invalid --disable-proto mode"); } return Just(true); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index de372400fd9ced..31597de1e14083 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -36,9 +36,6 @@ namespace node { namespace inspector { namespace { - -using node::OnFatalError; - using v8::Context; using v8::Function; using v8::HandleScope; @@ -917,8 +914,7 @@ void Agent::ToggleAsyncHook(Isolate* isolate, Local fn) { USE(fn->Call(context, Undefined(isolate), 0, nullptr)); if (try_catch.HasCaught() && !try_catch.HasTerminated()) { PrintCaughtException(isolate, context, try_catch); - OnFatalError("\nnode::inspector::Agent::ToggleAsyncHook", - "Cannot toggle Inspector's AsyncHook, please report this."); + UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this."); } } diff --git a/src/node_errors.h b/src/node_errors.h index ff4993224eb500..9e7aaf2c36dd0e 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -21,8 +21,11 @@ void AppendExceptionLine(Environment* env, // This function calls backtrace, it should have not be marked as [[noreturn]]. // But it is a public API, removing the attribute can break. +// Prefer UNREACHABLE() internally instead, it doesn't need manually set +// location. [[noreturn]] void OnFatalError(const char* location, const char* message); -// This function calls backtrace, do not mark as [[noreturn]]. +// This function calls backtrace, do not mark as [[noreturn]]. Read more in the +// ABORT macro. void OOMErrorHandler(const char* location, const v8::OOMDetails& details); // Helpers to construct errors similar to the ones provided by diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc index 8f0a3844121e23..d4eb79ebc32154 100644 --- a/src/node_watchdog.cc +++ b/src/node_watchdog.cc @@ -45,7 +45,7 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out) int rc; rc = uv_loop_init(&loop_); if (rc != 0) { - OnFatalError("node::Watchdog::Watchdog()", "Failed to initialize uv loop."); + UNREACHABLE("Failed to initialize uv loop."); } rc = uv_async_init(&loop_, &async_, [](uv_async_t* signal) { diff --git a/src/util.h b/src/util.h index 7588bc6f604df6..339e79d91e7d34 100644 --- a/src/util.h +++ b/src/util.h @@ -126,6 +126,11 @@ void DumpJavaScriptBacktrace(FILE* fp); #define ABORT_NO_BACKTRACE() abort() #endif +// A function calls a [[noreturn]] function will print an incorrect +// call stack because the frame pc was advanced to an invalid op at the call +// site, which may vary on different platforms. +// `ABORT` must be a macro and not a [[noreturn]] function to make sure the +// backtrace is correct. #define ABORT() \ do { \ node::DumpNativeBacktrace(stderr); \ @@ -141,7 +146,9 @@ void DumpJavaScriptBacktrace(FILE* fp); static const node::AssertionInfo args = { \ __FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME}; \ node::Assert(args); \ - /* Mark the macro as [[noreturn]]. */ \ + /* `node::Assert` doesn't return. Add an [[noreturn]] abort() here to */ \ + /* make the compiler happy about no return value in the caller */ \ + /* function when calling ERROR_AND_ABORT. */ \ ABORT_NO_BACKTRACE(); \ } while (0) From 536823ae6bcd5bfab7f054afb0f0332737d35d8e Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 28 Nov 2023 23:23:13 +0800 Subject: [PATCH 3/4] fixup! src: fix backtrace with [[noreturn]] abort Co-authored-by: Joyee Cheung --- src/util.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util.h b/src/util.h index 339e79d91e7d34..cf73081f7e0fb6 100644 --- a/src/util.h +++ b/src/util.h @@ -126,9 +126,11 @@ void DumpJavaScriptBacktrace(FILE* fp); #define ABORT_NO_BACKTRACE() abort() #endif -// A function calls a [[noreturn]] function will print an incorrect -// call stack because the frame pc was advanced to an invalid op at the call -// site, which may vary on different platforms. +// Caller of this macro must not be marked as [[noreturn]]. Printing of +// backtraces may not work correctly in [[noreturn]] functions because +// when generating code for them the compiler can choose not to +// maintain the frame pointers or link registers that are necessary for +// correct backtracing. // `ABORT` must be a macro and not a [[noreturn]] function to make sure the // backtrace is correct. #define ABORT() \ From 3b89fba65143cf12e2fc3bf78602e17b2a71802f Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 28 Nov 2023 23:46:05 +0800 Subject: [PATCH 4/4] fixup! src: fix backtrace with [[noreturn]] abort --- src/util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util.h b/src/util.h index cf73081f7e0fb6..08b9151f1cfabf 100644 --- a/src/util.h +++ b/src/util.h @@ -130,7 +130,7 @@ void DumpJavaScriptBacktrace(FILE* fp); // backtraces may not work correctly in [[noreturn]] functions because // when generating code for them the compiler can choose not to // maintain the frame pointers or link registers that are necessary for -// correct backtracing. +// correct backtracing. // `ABORT` must be a macro and not a [[noreturn]] function to make sure the // backtrace is correct. #define ABORT() \