From 6929823838ee3e36380d9eb8e8d2b294f8b417c9 Mon Sep 17 00:00:00 2001 From: Mikael Hermansson Date: Mon, 2 Jun 2025 18:15:10 +0200 Subject: [PATCH] Fix various race conditions with capturing of script backtraces --- core/error/error_macros.cpp | 10 +------ core/object/script_language.cpp | 29 +++++++++++++++---- platform/linuxbsd/crash_handler_linuxbsd.cpp | 13 ++++----- platform/macos/crash_handler_macos.mm | 13 ++++----- .../windows/crash_handler_windows_seh.cpp | 13 ++++----- .../windows/crash_handler_windows_signal.cpp | 13 ++++----- 6 files changed, 44 insertions(+), 47 deletions(-) diff --git a/core/error/error_macros.cpp b/core/error/error_macros.cpp index 224bf216f7..220a5741a6 100644 --- a/core/error/error_macros.cpp +++ b/core/error/error_macros.cpp @@ -92,15 +92,7 @@ void _err_print_error(const char *p_function, const char *p_file, int p_line, co // Main error printing function. void _err_print_error(const char *p_function, const char *p_file, int p_line, const char *p_error, const char *p_message, bool p_editor_notify, ErrorHandlerType p_type) { if (OS::get_singleton()) { - Vector> script_backtraces; - - // If script languages aren't initialized, we could be in the process of shutting down, in which case we don't want to allocate any objects, as we could be - // logging ObjectDB leaks, where ObjectDB would be locked, thus causing a deadlock. - if (ScriptServer::are_languages_initialized()) { - script_backtraces = ScriptServer::capture_script_backtraces(false); - } - - OS::get_singleton()->print_error(p_function, p_file, p_line, p_error, p_message, p_editor_notify, (Logger::ErrorType)p_type, script_backtraces); + OS::get_singleton()->print_error(p_function, p_file, p_line, p_error, p_message, p_editor_notify, (Logger::ErrorType)p_type, ScriptServer::capture_script_backtraces(false)); } else { // Fallback if errors happen before OS init or after it's destroyed. const char *err_details = (p_message && *p_message) ? p_message : p_error; diff --git a/core/object/script_language.cpp b/core/object/script_language.cpp index 3935f7137c..215ca918ea 100644 --- a/core/object/script_language.cpp +++ b/core/object/script_language.cpp @@ -45,6 +45,15 @@ bool ScriptServer::scripting_enabled = true; bool ScriptServer::reload_scripts_on_save = false; ScriptEditRequestFunction ScriptServer::edit_request_func = nullptr; +// These need to be the last static variables in this file, since we're exploiting the reverse-order destruction of static variables. +static bool is_program_exiting = false; +struct ProgramExitGuard { + ~ProgramExitGuard() { + is_program_exiting = true; + } +}; +static ProgramExitGuard program_exit_guard; + void Script::_notification(int p_what) { switch (p_what) { case NOTIFICATION_POSTINITIALIZE: { @@ -536,13 +545,21 @@ void ScriptServer::save_global_classes() { } Vector> ScriptServer::capture_script_backtraces(bool p_include_variables) { - int language_count = ScriptServer::get_language_count(); - Vector> result; - result.resize(language_count); - for (int i = 0; i < language_count; i++) { - ScriptLanguage *language = ScriptServer::get_language(i); - result.write[i].instantiate(language, p_include_variables); + if (is_program_exiting) { + return Vector>(); } + + MutexLock lock(languages_mutex); + if (!languages_ready) { + return Vector>(); + } + + Vector> result; + result.resize(_language_count); + for (int i = 0; i < _language_count; i++) { + result.write[i].instantiate(_languages[i], p_include_variables); + } + return result; } diff --git a/platform/linuxbsd/crash_handler_linuxbsd.cpp b/platform/linuxbsd/crash_handler_linuxbsd.cpp index 770bc835fc..bbf9ef8889 100644 --- a/platform/linuxbsd/crash_handler_linuxbsd.cpp +++ b/platform/linuxbsd/crash_handler_linuxbsd.cpp @@ -146,14 +146,11 @@ static void handle_crash(int sig) { print_error("-- END OF C++ BACKTRACE --"); print_error("================================================================"); - if (ScriptServer::are_languages_initialized()) { - Vector> script_backtraces = ScriptServer::capture_script_backtraces(false); - for (const Ref &backtrace : script_backtraces) { - if (!backtrace->is_empty()) { - print_error(backtrace->format()); - print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); - print_error("================================================================"); - } + for (const Ref &backtrace : ScriptServer::capture_script_backtraces(false)) { + if (!backtrace->is_empty()) { + print_error(backtrace->format()); + print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); + print_error("================================================================"); } } diff --git a/platform/macos/crash_handler_macos.mm b/platform/macos/crash_handler_macos.mm index 6dc389d037..38c1625dac 100644 --- a/platform/macos/crash_handler_macos.mm +++ b/platform/macos/crash_handler_macos.mm @@ -176,14 +176,11 @@ static void handle_crash(int sig) { print_error("-- END OF C++ BACKTRACE --"); print_error("================================================================"); - if (ScriptServer::are_languages_initialized()) { - Vector> script_backtraces = ScriptServer::capture_script_backtraces(false); - for (const Ref &backtrace : script_backtraces) { - if (!backtrace->is_empty()) { - print_error(backtrace->format()); - print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); - print_error("================================================================"); - } + for (const Ref &backtrace : ScriptServer::capture_script_backtraces(false)) { + if (!backtrace->is_empty()) { + print_error(backtrace->format()); + print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); + print_error("================================================================"); } } diff --git a/platform/windows/crash_handler_windows_seh.cpp b/platform/windows/crash_handler_windows_seh.cpp index 02c95120b6..e3efab37b8 100644 --- a/platform/windows/crash_handler_windows_seh.cpp +++ b/platform/windows/crash_handler_windows_seh.cpp @@ -230,14 +230,11 @@ DWORD CrashHandlerException(EXCEPTION_POINTERS *ep) { SymCleanup(process); - if (ScriptServer::are_languages_initialized()) { - Vector> script_backtraces = ScriptServer::capture_script_backtraces(false); - for (const Ref &backtrace : script_backtraces) { - if (!backtrace->is_empty()) { - print_error(backtrace->format()); - print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); - print_error("================================================================"); - } + for (const Ref &backtrace : ScriptServer::capture_script_backtraces(false)) { + if (!backtrace->is_empty()) { + print_error(backtrace->format()); + print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); + print_error("================================================================"); } } diff --git a/platform/windows/crash_handler_windows_signal.cpp b/platform/windows/crash_handler_windows_signal.cpp index cd1f39d3a4..0d1a0734b3 100644 --- a/platform/windows/crash_handler_windows_signal.cpp +++ b/platform/windows/crash_handler_windows_signal.cpp @@ -184,14 +184,11 @@ extern void CrashHandlerException(int signal) { print_error("-- END OF C++ BACKTRACE --"); print_error("================================================================"); - if (ScriptServer::are_languages_initialized()) { - Vector> script_backtraces = ScriptServer::capture_script_backtraces(false); - for (const Ref &backtrace : script_backtraces) { - if (!backtrace->is_empty()) { - print_error(backtrace->format()); - print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); - print_error("================================================================"); - } + for (const Ref &backtrace : ScriptServer::capture_script_backtraces(false)) { + if (!backtrace->is_empty()) { + print_error(backtrace->format()); + print_error(vformat("-- END OF %s BACKTRACE --", backtrace->get_language_name().to_upper())); + print_error("================================================================"); } } }