From 7c1713f428a666986e813db7146855ffabf574c9 Mon Sep 17 00:00:00 2001 From: lethosor Date: Sat, 30 Nov 2024 01:30:50 -0500 Subject: [PATCH 1/3] Lua: switch to recursive mutex in lua_lock() Previously, calling a Lua C API function that throws an error (e.g. with any of the `lua_error()` family of functions, which use `luaD_throw()` internally) in a function that also uses our own `StackUnwinder` would deadlock, because: - `luaD_throw()` calls `lua_lock()` but apparently expects something else to call `lua_unlock()` - `~StackUnwinder()` calls `lua_settop()` to rewind the stack, which internally also calls `lua_lock()` and `lua_unlock()`. This is called before the mutex can be unlocked. This was the root cause of the issue behind #5040, #5055, #5056 - `GetVector()` was called with an invalid index (only when invoked from `gui/launcher`, because this changed the data on the Lua stack) and threw an exception, which caused the `~StackUnwinder()` destructor to run while the Lua state was still locked. Other things to note: - We control the locking/unlocking implementation - the default, defined in `llimits.h`, is a _no-op_. - @ab9rf points out that the `CRITICAL_SECTION` API we're using on Windows already appears to be equivalent to a recursive mutex, so this issue likely would not have occurred there. The issue can be reproduced relatively easily with a simple test command, e.g.: ```diff @@ -716,6 +723,15 @@ void ls_helper(color_ostream &con, const std::vector ¶ms) { command_result Core::runCommand(color_ostream &con, const std::string &first_, std::vector &parts) { std::string first = first_; + if (first == "luaerror") { + auto L = Lua::Core::State; + Lua::StackUnwinder top(L); + + luaL_error(L, "test error"); + + return CR_OK; + } + CommandDepthCounter counter; if (!counter.ok()) { ``` In `gui/launcher`: - before this change, invoking `luaerror` would hang DF. - after this change, `luaerror` prints `nil` in red to the native console and does nothing. This comes from the last-resort error handler in `LuaTools.cpp:report_error()`, and is equivalent to how other unexpected errors are handled in code invoked from Lua. Not ideal, but better than a crash. From the native console, invoking `luaerror` crashes DF in both cases (SIGABRT), and logs `PANIC: unprotected error in call to Lua API (test error)` to `stderr.log`. The difference in behavior is because `report_error()` above is part of the error handling system present when code is called _from Lua_, through variants of `SafeCall`. There is no Lua layer involved in the native console. (Note: the same crash would have been observed in the original issue in #5056 et. al. if the error had occurred when invoked through the console.) --- depends/lua/include/dfhack_llimits.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/depends/lua/include/dfhack_llimits.h b/depends/lua/include/dfhack_llimits.h index f8ca518190..603475d12e 100644 --- a/depends/lua/include/dfhack_llimits.h +++ b/depends/lua/include/dfhack_llimits.h @@ -54,8 +54,17 @@ struct lua_extra_state { #define lua_lock(L) EnterCriticalSection(luai_mutex(L)) #define lua_unlock(L) LeaveCriticalSection(luai_mutex(L)) #else -#define luai_userstateopen(L) luai_mutex(L) = (mutex_t*)malloc(sizeof(mutex_t)); *luai_mutex(L) = PTHREAD_MUTEX_INITIALIZER -#define luai_userstateclose(L) lua_unlock(L); pthread_mutex_destroy(luai_mutex(L)); free(luai_mutex(L)) +#define luai_userstateopen(L) do { \ + luai_mutex(L) = (mutex_t*)malloc(sizeof(mutex_t)); \ + pthread_mutexattr_t attr; \ + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); \ + pthread_mutex_init(luai_mutex(L), &attr); \ + } while (0) +#define luai_userstateclose(L) do { \ + lua_unlock(L); \ + pthread_mutex_destroy(luai_mutex(L)); \ + free(luai_mutex(L)); \ + } while (0) #define lua_lock(L) pthread_mutex_lock(luai_mutex(L)) #define lua_unlock(L) pthread_mutex_unlock(luai_mutex(L)) #endif From bd80a64563303c1315c1871f13d3e9a1bbf56da7 Mon Sep 17 00:00:00 2001 From: Myk Taylor Date: Sat, 30 Nov 2024 16:51:40 -0800 Subject: [PATCH 2/3] more informative commit descriptions --- .github/workflows/generate-symbols.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/generate-symbols.yml b/.github/workflows/generate-symbols.yml index 850ce83951..f863e281b2 100644 --- a/.github/workflows/generate-symbols.yml +++ b/.github/workflows/generate-symbols.yml @@ -194,7 +194,7 @@ jobs: - name: Commit symbol updates uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update symbols + commit_message: Auto-update symbols for Linux DF version ${{ inputs.version }} repository: xml commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com @@ -297,7 +297,7 @@ jobs: - name: Commit symbol updates uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update symbols + commit_message: Auto-update symbols for Windows DF version ${{ inputs.version }} repository: xml commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com @@ -361,6 +361,7 @@ jobs: done df_ver=`./dfhack-run.exe lua 'print(dfhack.gui.getDFViewscreen(true).str_version)'` echo "Found version string: '$df_ver'" + echo "DETECTED_DF_VER=$df_ver" >>$GITHUB_ENV sed -i "s/v0.50.0 win64 STEAM/v0.$df_ver win64 STEAM/" ../xml/symbols.xml ./dfhack-run.exe die || true - name: Merge updates @@ -375,7 +376,7 @@ jobs: - name: Commit symbol updates uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update Windows DF version in symbols + commit_message: Auto-update Windows DF version ${{ env.DETECTED_DF_VER }} in symbols repository: xml commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com @@ -405,7 +406,7 @@ jobs: - name: Commit ref update uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update structures ref + commit_message: Auto-update structures ref for ${{ env.DETECTED_DF_VER }} commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com - name: Launch steam-deploy From 39af25c392221fb508b5a5a88cee08df41e2e681 Mon Sep 17 00:00:00 2001 From: Myk Taylor Date: Sat, 30 Nov 2024 17:22:53 -0800 Subject: [PATCH 3/3] even better commit messages --- .github/workflows/generate-symbols.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/generate-symbols.yml b/.github/workflows/generate-symbols.yml index f863e281b2..8f13adbb4f 100644 --- a/.github/workflows/generate-symbols.yml +++ b/.github/workflows/generate-symbols.yml @@ -147,6 +147,7 @@ jobs: done df_ver=`./dfhack-run lua 'print(dfhack.gui.getDFViewscreen(true).str_version)' | ansifilter` echo "Found version string: '$df_ver'" + echo "DETECTED_DF_VER=$df_ver" >>$GITHUB_ENV sed -i "s/v0.50.0 linux64 STEAM/v0.$df_ver linux64 STEAM/" ../xml/symbols.xml ./dfhack-run die || true fi @@ -194,7 +195,7 @@ jobs: - name: Commit symbol updates uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update symbols for Linux DF version ${{ inputs.version }} + commit_message: Auto-update symbols for Linux DF version ${{ env.DETECTED_DF_VER || inputs.version }} repository: xml commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com @@ -376,7 +377,7 @@ jobs: - name: Commit symbol updates uses: stefanzweifel/git-auto-commit-action@v5 with: - commit_message: Auto-update Windows DF version ${{ env.DETECTED_DF_VER }} in symbols + commit_message: Auto-update Windows DF version to ${{ env.DETECTED_DF_VER }} in symbols repository: xml commit_user_name: DFHack-Urist via GitHub Actions commit_user_email: 63161697+DFHack-Urist@users.noreply.github.com