From 5accca119c296f20516905cbf8b96b7f24db3bac Mon Sep 17 00:00:00 2001 From: Nicholas McDaniel Date: Mon, 13 Jan 2025 10:02:25 -0500 Subject: [PATCH 1/2] fix: Prevent deadlocks when attempting unlock of an already unlocked CoreSuspender. `dec_tool_count` was being called on already unlocked CoreSuspenders, causing the tool count to go negative. As the core only resumes when the tool count is 0, a deadlock occured. By returning early if the underlying lock isn't held, this issue is avoided. --- library/Core.cpp | 16 ---------------- library/include/Core.h | 2 ++ 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 8538dd65e9..cddc76dbd5 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -419,9 +419,6 @@ bool is_builtin(color_ostream &con, const std::string &command) { } void get_commands(color_ostream &con, std::vector &commands) { -#ifdef LINUX_BUILD - CoreSuspender suspend; -#else ConditionalCoreSuspender suspend{}; if (!suspend) { @@ -429,7 +426,6 @@ void get_commands(color_ostream &con, std::vector &commands) { commands.clear(); return; } -#endif auto L = DFHack::Core::getInstance().getLuaState(); Lua::StackUnwinder top(L); @@ -630,16 +626,12 @@ static std::string sc_event_name (state_change_event id) { } void help_helper(color_ostream &con, const std::string &entry_name) { -#ifdef LINUX_BUILD - CoreSuspender suspend; -#else ConditionalCoreSuspender suspend{}; if (!suspend) { con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); return; } -#endif auto L = DFHack::Core::getInstance().getLuaState(); Lua::StackUnwinder top(L); @@ -658,16 +650,12 @@ void help_helper(color_ostream &con, const std::string &entry_name) { } void tags_helper(color_ostream &con, const std::string &tag) { -#ifdef LINUX_BUILD - CoreSuspender suspend; -#else ConditionalCoreSuspender suspend{}; if (!suspend) { con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); return; } -#endif auto L = DFHack::Core::getInstance().getLuaState(); Lua::StackUnwinder top(L); @@ -705,16 +693,12 @@ void ls_helper(color_ostream &con, const std::vector ¶ms) { filter.push_back(str); } -#ifdef LINUX_BUILD - CoreSuspender suspend; -#else ConditionalCoreSuspender suspend{}; if (!suspend) { con.printerr("Failed Lua call to helpdb.help (could not acquire core lock).\n"); return; } -#endif auto L = DFHack::Core::getInstance().getLuaState(); Lua::StackUnwinder top(L); diff --git a/library/include/Core.h b/library/include/Core.h index 7e3c825bce..3e388ccfae 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -465,6 +465,8 @@ namespace DFHack void unlock() { + if (!owns_lock()) + return; parent_t::unlock(); dec_tool_count(); } From bb0a2110dc6e7fbc75d5195034c2de5f89143ef5 Mon Sep 17 00:00:00 2001 From: Nicholas McDaniel Date: Mon, 13 Jan 2025 10:27:31 -0500 Subject: [PATCH 2/2] Move has_lock checks to the destructors --- library/include/Core.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/library/include/Core.h b/library/include/Core.h index 3e388ccfae..8725c1faeb 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -376,8 +376,6 @@ namespace DFHack void unlock() { - if (!owns_lock()) - return; /* Restore core owner to previous value */ if (tid == std::thread::id{}) Lua::Core::Reset(core.getConsole(), "suspend"); @@ -386,7 +384,8 @@ namespace DFHack } ~CoreSuspenderBase() { - unlock(); + if (owns_lock()) + unlock(); } protected: @@ -438,7 +437,8 @@ namespace DFHack // note that this is needed so the destructor will call CoreSuspender::unlock instead of CoreSuspenderBase::unlock ~CoreSuspender() { - unlock(); + if (owns_lock()) + unlock(); } protected: @@ -465,8 +465,6 @@ namespace DFHack void unlock() { - if (!owns_lock()) - return; parent_t::unlock(); dec_tool_count(); }