From f6ff11c9f99c75231cdeb168e7ac28e4cf544b64 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 4 Dec 2023 00:42:57 +0800 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/50849 Refs: https://github.com/nodejs/node/issues/50761 Reviewed-By: Joyee Cheung --- src/api/environment.cc | 2 +- src/inspector_agent.cc | 6 +----- src/node_errors.cc | 20 ++++++------------- src/node_errors.h | 9 +++++++-- src/node_process_methods.cc | 2 +- src/node_watchdog.cc | 2 +- src/util.h | 39 ++++++++++++++++++++++++++----------- 7 files changed, 45 insertions(+), 35 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.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 e804c250ab169a..21e51f658db4c6 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -19,9 +19,14 @@ 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. +// Prefer UNREACHABLE() internally instead, it doesn't need manually set +// location. [[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]]. 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 // 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/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 28873dbe4024df..08b9151f1cfabf 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,32 @@ void DumpJavaScriptBacktrace(FILE* fp); #define ABORT_NO_BACKTRACE() abort() #endif -#define ABORT() node::Abort() +// 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() \ + 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); \ + /* `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) #ifdef __GNUC__