From 731a13a1ba4d11e0c7daa7e32335c365f9f190cd Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 29 Apr 2023 13:54:52 -0400 Subject: [PATCH] Fix race in ScriptServer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #76581. TSAN flagged this issue on starting the editor: 1. main calls register_core_types, which calls IP::create(), which calls Thread::start on the resolver thread 2. Thread::callback calls ScriptServer::thread_enter(), as "Scripts may need to attach a stack." 3. ScriptServer::thread_enter() accesses ScriptServer::_languages, which is still being initialized on the main thread by initialize_gdscript_module This fixes the issue by skipping thread enter/exit notifications if languages have not finished initializing yet. I'm assuming that notifying un-initialized languages of thread starts/stops would have been pointless anyways. If we need to somehow notify languages of threads before the languages initialize, we'll need a different solution. ``` Godot Engine v4.0.2.stable.custom_build.7a0977ce2 - https://godotengine.org ================== WARNING: ThreadSanitizer: data race (pid=9426) Write of size 4 at 0x55615b187cd0 by main thread: #0 ScriptServer::register_language(ScriptLanguage*) /home/rcorre/src/godot/godot/core/object/script_language.cpp:177:28 (godot.linuxbsd.editor.x86_64.llvm.san+0x9e52ab9) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #1 initialize_gdscript_module(ModuleInitializationLevel) /home/rcorre/src/godot/godot/modules/gdscript/register_types.cpp:118:3 (godot.linuxbsd.editor.x86_64.llvm.san+0x36f9c6f) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #2 initialize_modules(ModuleInitializationLevel) /home/rcorre/src/godot/godot/modules/register_module_types.gen.cpp:93:2 (godot.linuxbsd.editor.x86_64.llvm.san+0x2f50499) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #3 Main::setup2(unsigned long) /home/rcorre/src/godot/godot/main/main.cpp:1961:2 (godot.linuxbsd.editor.x86_64.llvm.san+0x2f1d40d) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #4 Main::setup(char const*, int, char**, bool) /home/rcorre/src/godot/godot/main/main.cpp:1879:10 (godot.linuxbsd.editor.x86_64.llvm.san+0x2f16370) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #5 main /home/rcorre/src/godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:61:14 (godot.linuxbsd.editor.x86_64.llvm.san+0x2e67e1f) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) Previous read of size 4 at 0x55615b187cd0 by thread T1: #0 ScriptServer::thread_enter() /home/rcorre/src/godot/godot/core/object/script_language.cpp:244:22 (godot.linuxbsd.editor.x86_64.llvm.san+0x9e54aed) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #1 Thread::callback(unsigned long, Thread::Settings const&, void (*)(void*), void*) /home/rcorre/src/godot/godot/core/os/thread.cpp:61:2 (godot.linuxbsd.editor.x86_64.llvm.san+0x9464ab0) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #2 void std::__invoke_impl(std::__invoke_other, void (*&&)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/bits/invoke.h:61:14 (godot.linuxbsd.editor.x86_64.llvm.san+0x9465283) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #3 std::__invoke_result::type std::__invoke(void (*&&)(unsigned long, Thread::Settings const&, void (*)(void*), void*), unsigned long&&, Thread::Settings&&, void (*&&)(void*), void*&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/bits/invoke.h:96:14 (godot.linuxbsd.editor.x86_64.llvm.san+0x9465283) #4 void std::thread::_Invoker>::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul>) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/bits/std_thread.h:258:13 (godot.linuxbsd.editor.x86_64.llvm.san+0x9465283) #5 std::thread::_Invoker>::operator()() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/bits/std_thread.h:265:11 (godot.linuxbsd.editor.x86_64.llvm.san+0x9465283) #6 std::thread::_State_impl>>::_M_run() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.1/../../../../include/c++/12.2.1/bits/std_thread.h:210:13 (godot.linuxbsd.editor.x86_64.llvm.san+0x9465283) #7 execute_native_thread_routine /usr/src/debug/gcc/gcc/libstdc++-v3/src/c++11/thread.cc:82:18 (libstdc++.so.6+0xd72c2) (BuildId: 6fe66a2d539a78c993bd2d377e00fad389220963) Location is global 'ScriptServer::_language_count' of size 4 at 0x55615b187cd0 (godot.linuxbsd.editor.x86_64.llvm.san+0xbf46cd0) Thread T1 (tid=9431, running) created by main thread at: #0 pthread_create (godot.linuxbsd.editor.x86_64.llvm.san+0x2de5776) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #1 __gthread_create /usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:663:35 (libstdc++.so.6+0xd73a9) (BuildId: 6fe66a2d539a78c993bd2d377e00fad389220963) #2 std::thread::_M_start_thread(std::unique_ptr>, void (*)()) /usr/src/debug/gcc/gcc/libstdc++-v3/src/c++11/thread.cc:147:37 (libstdc++.so.6+0xd73a9) #3 IP::IP() /home/rcorre/src/godot/godot/core/io/ip.cpp:347:19 (godot.linuxbsd.editor.x86_64.llvm.san+0x962cbcd) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #4 IPUnix::IPUnix() /home/rcorre/src/godot/godot/drivers/unix/ip_unix.cpp:261:9 (godot.linuxbsd.editor.x86_64.llvm.san+0x4aee599) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #5 IPUnix::_create_unix() /home/rcorre/src/godot/godot/drivers/unix/ip_unix.cpp:258:9 (godot.linuxbsd.editor.x86_64.llvm.san+0x4aee599) #6 IP::create() /home/rcorre/src/godot/godot/core/io/ip.cpp:339:9 (godot.linuxbsd.editor.x86_64.llvm.san+0x962ca5e) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #7 register_core_types() /home/rcorre/src/godot/godot/core/register_core_types.cpp:279:7 (godot.linuxbsd.editor.x86_64.llvm.san+0x93e2333) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #8 Main::setup(char const*, int, char**, bool) /home/rcorre/src/godot/godot/main/main.cpp:690:2 (godot.linuxbsd.editor.x86_64.llvm.san+0x2f08a49) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) #9 main /home/rcorre/src/godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:61:14 (godot.linuxbsd.editor.x86_64.llvm.san+0x2e67e1f) (BuildId: 780a9db7c37d88e78d5ee659c4fa1cd378abd048) SUMMARY: ThreadSanitizer: data race /home/rcorre/src/godot/godot/core/object/script_language.cpp:177:28 in ScriptServer::register_language(ScriptLanguage*) ``` Co-authored-by: Pedro J. Estébanez --- core/object/script_language.cpp | 10 ++++++++-- core/object/script_language.h | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/core/object/script_language.cpp b/core/object/script_language.cpp index 6f047d80aaa..a8b0e426aea 100644 --- a/core/object/script_language.cpp +++ b/core/object/script_language.cpp @@ -42,7 +42,7 @@ int ScriptServer::_language_count = 0; bool ScriptServer::scripting_enabled = true; bool ScriptServer::reload_scripts_on_save = false; -bool ScriptServer::languages_finished = false; +SafeFlag ScriptServer::languages_finished; // Used until GH-76581 is fixed properly. ScriptEditRequestFunction ScriptServer::edit_request_func = nullptr; void Script::_notification(int p_what) { @@ -228,7 +228,7 @@ void ScriptServer::finish_languages() { _languages[i]->finish(); } global_classes_clear(); - languages_finished = true; + languages_finished.set(); } void ScriptServer::set_reload_scripts_on_save(bool p_enable) { @@ -240,12 +240,18 @@ bool ScriptServer::is_reload_scripts_on_save_enabled() { } void ScriptServer::thread_enter() { + if (!languages_finished.is_set()) { + return; + } for (int i = 0; i < _language_count; i++) { _languages[i]->thread_enter(); } } void ScriptServer::thread_exit() { + if (!languages_finished.is_set()) { + return; + } for (int i = 0; i < _language_count; i++) { _languages[i]->thread_exit(); } diff --git a/core/object/script_language.h b/core/object/script_language.h index c22890e30a0..2b685c77a3e 100644 --- a/core/object/script_language.h +++ b/core/object/script_language.h @@ -35,6 +35,7 @@ #include "core/io/resource.h" #include "core/templates/pair.h" #include "core/templates/rb_map.h" +#include "core/templates/safe_refcount.h" #include "core/variant/typed_array.h" class ScriptLanguage; @@ -52,7 +53,7 @@ class ScriptServer { static int _language_count; static bool scripting_enabled; static bool reload_scripts_on_save; - static bool languages_finished; + static SafeFlag languages_finished; // Used until GH-76581 is fixed properly. struct GlobalScriptClass { StringName language; @@ -97,7 +98,7 @@ public: static void init_languages(); static void finish_languages(); - static bool are_languages_finished() { return languages_finished; } + static bool are_languages_finished() { return languages_finished.is_set(); } }; class ScriptInstance;