From 5cdf46026529788c6946fc87024e065a9dc114ba Mon Sep 17 00:00:00 2001 From: sharkautarch <128002472+sharkautarch@users.noreply.github.com> Date: Thu, 24 Oct 2024 12:17:18 -0400 Subject: [PATCH 1/5] sdlwindow.cpp: close m_thread w/o std::terminate being called --- src/Backends/SDLBackend.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Backends/SDLBackend.cpp b/src/Backends/SDLBackend.cpp index 6d50f8d34e..a5b0d963dc 100644 --- a/src/Backends/SDLBackend.cpp +++ b/src/Backends/SDLBackend.cpp @@ -55,6 +55,7 @@ namespace gamescope GAMESCOPE_SDL_EVENT_CURSOR, GAMESCOPE_SDL_EVENT_COUNT, + GAMESCOPE_SDL_EVENT_REQ_EXIT, }; class CSDLConnector final : public IBackendConnector @@ -113,6 +114,7 @@ namespace gamescope { public: CSDLBackend(); + ~CSDLBackend(); ///////////// // IBackend @@ -515,6 +517,11 @@ namespace gamescope // Do nothing. } + CSDLBackend::~CSDLBackend() { + PushUserEvent(GAMESCOPE_SDL_EVENT_REQ_EXIT); + m_SDLThread.join(); + } + void CSDLBackend::SDLThreadFunc() { pthread_setname_np( pthread_self(), "gamescope-sdl" ); @@ -912,6 +919,9 @@ namespace gamescope SDL_SetCursor( m_pCursor ); } + else if ( event.type == GetUserEventIndex( GAMESCOPE_SDL_EVENT_REQ_EXIT ) ) { + return; + } } break; } From 3ca06f6f992bc3cb95df9a6ecbbd9beebcda55c0 Mon Sep 17 00:00:00 2001 From: sharkautarch <128002472+sharkautarch@users.noreply.github.com> Date: Fri, 6 Dec 2024 13:00:54 -0500 Subject: [PATCH 2/5] backend, steamcompmgr: fix use-after-free issue at exit all: also release the xwayland_server_guard lock before calling pthread_exit() to prevent gamescope from hanging at exit --- src/backend.cpp | 21 ++++++++++----------- src/steamcompmgr.cpp | 8 +++++--- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/backend.cpp b/src/backend.cpp index 8a6bbe8ed9..d07f847ba6 100644 --- a/src/backend.cpp +++ b/src/backend.cpp @@ -16,34 +16,33 @@ namespace gamescope // IBackend ///////////// - static IBackend *s_pBackend = nullptr; + static std::atomic s_pBackend = nullptr; IBackend *IBackend::Get() { return s_pBackend; } - + + bool IBackend::Set( IBackend *pBackend ) { - if ( s_pBackend ) - { - delete s_pBackend; - s_pBackend = nullptr; - } + if ( s_pBackend ) { + GetBackend()->IBackend::~IBackend(); + } if ( pBackend ) { s_pBackend = pBackend; - if ( !s_pBackend->Init() ) + if ( !GetBackend()->Init() ) { - delete s_pBackend; - s_pBackend = nullptr; + s_pBackend = nullptr; return false; } } return true; } + ///////////////// // CBaseBackendFb @@ -184,4 +183,4 @@ namespace gamescope GetBackend()->DumpDebugInfo(); }); -} +} \ No newline at end of file diff --git a/src/steamcompmgr.cpp b/src/steamcompmgr.cpp index 11a7cade6c..8d15bda9d2 100644 --- a/src/steamcompmgr.cpp +++ b/src/steamcompmgr.cpp @@ -5896,7 +5896,7 @@ error(Display *dpy, XErrorEvent *ev) } [[noreturn]] static void -steamcompmgr_exit(void) +steamcompmgr_exit(std::optional> lock = std::nullopt) { g_ImageWaiter.Shutdown(); @@ -5936,7 +5936,9 @@ steamcompmgr_exit(void) wlserver_lock(); wlserver_shutdown(); wlserver_unlock(false); - + + if (lock) + lock->unlock(); pthread_exit(NULL); } @@ -8042,7 +8044,7 @@ steamcompmgr_main(int argc, char **argv) vblank = false; } - steamcompmgr_exit(); + steamcompmgr_exit(std::optional> {std::in_place_t{}, std::move(xwayland_server_guard)} ); } void steamcompmgr_send_frame_done_to_focus_window() From fff553f09e8984e60588fd08533e182641358e2d Mon Sep 17 00:00:00 2001 From: sharkautarch <128002472+sharkautarch@users.noreply.github.com> Date: Sun, 8 Dec 2024 11:00:20 -0500 Subject: [PATCH 3/5] steamcompmgr: fix another coredump-at-exit --- src/steamcompmgr.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/steamcompmgr.cpp b/src/steamcompmgr.cpp index 8d15bda9d2..996458d945 100644 --- a/src/steamcompmgr.cpp +++ b/src/steamcompmgr.cpp @@ -5898,6 +5898,23 @@ error(Display *dpy, XErrorEvent *ev) [[noreturn]] static void steamcompmgr_exit(std::optional> lock = std::nullopt) { + + // Need to clear all the vk_lutxd references (which can be tied to backend-allocated memory) + // for the colormgmt globals/statics, to avoid coredump at exit from within the colormgmt exit-time destructors: + for (auto& colorMgmtArr : + { + std::ref(g_ColorMgmtLuts), + std::ref(g_ColorMgmtLutsOverride), + std::ref(g_ScreenshotColorMgmtLuts), + std::ref(g_ScreenshotColorMgmtLutsHDR) + }) + { + for (auto& colorMgmt : colorMgmtArr.get()) + { + colorMgmt.gamescope_color_mgmt_luts::~gamescope_color_mgmt_luts(); //dtor call also calls all the subobjects' dtors + } + } + g_ImageWaiter.Shutdown(); // Clean up any commits. @@ -5922,7 +5939,6 @@ steamcompmgr_exit(std::optional> lock = std::nullop { g_ColorMgmt.pending.appHDRMetadata = nullptr; g_ColorMgmt.current.appHDRMetadata = nullptr; - s_scRGB709To2020Matrix = nullptr; for (int i = 0; i < gamescope::GAMESCOPE_SCREEN_TYPE_COUNT; i++) { From 0f5789f9a276f85e696fc72397471b52934f5705 Mon Sep 17 00:00:00 2001 From: sharkautarch <128002472+sharkautarch@users.noreply.github.com> Date: Sun, 8 Dec 2024 11:06:11 -0500 Subject: [PATCH 4/5] steamcompmgr: attempt to fix the OTHER hang at exit --- src/Utils/Lock.h | 66 ++++++++++++++++++++++++++++++++++++++++++++ src/steamcompmgr.cpp | 11 ++++++-- 2 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 src/Utils/Lock.h diff --git a/src/Utils/Lock.h b/src/Utils/Lock.h new file mode 100644 index 0000000000..9eec9e5ac4 --- /dev/null +++ b/src/Utils/Lock.h @@ -0,0 +1,66 @@ +#pragma once +#include "Utils/NonCopyable.h" +#include +#include +#include +#include + +namespace gamescope +{ + class CGlobalLockReference; + template + class ReferencedLock; + class CGlobalLockReference : public NonCopyable + { + template + friend class ReferencedLock; + public: + inline std::optional> popLock() const; + CGlobalLockReference() = default; + CGlobalLockReference(CGlobalLockReference&&) = delete; + CGlobalLockReference(CGlobalLockReference const&&) = delete; + protected: + mutable std::optional>* m_pLock; + mutable std::thread::id m_owningThreadID; + }; + + template //a reference to a static object is actually constexpr, which is nice, because putting the reference-to-static in the template param means this class takes up a bit less space + class ReferencedLock : public NonCopyable + { + using T = std::unique_lock; + friend class CGlobalLockReference; + public: + explicit ReferencedLock(std::mutex& mut) : m_lock{std::in_place_t{}, std::unique_lock{mut}} + { + StaticGlobLoc.m_pLock = &(this->m_lock); + StaticGlobLoc.m_owningThreadID = std::this_thread::get_id(); + } + + //should still be able to move ReferencedLock when move elision can be done, + //but otherwise don't want to risk messing something up + ReferencedLock(ReferencedLock&&) = delete; + ReferencedLock(ReferencedLock const&&) = delete; + + ~ReferencedLock() { + if (m_lock.has_value()) { + StaticGlobLoc.m_pLock = nullptr; + } + } + inline std::optional popLock() { + StaticGlobLoc.m_pLock = nullptr; + return std::move(m_lock); + } + protected: + mutable std::optional m_lock; + }; + + std::optional> CGlobalLockReference::popLock() const { + assert(m_owningThreadID == std::this_thread::get_id()); + + if (m_pLock) { + return std::move(*std::exchange(m_pLock,nullptr)); + } else { + return std::nullopt; + } + } +} \ No newline at end of file diff --git a/src/steamcompmgr.cpp b/src/steamcompmgr.cpp index 996458d945..e0feba9bd7 100644 --- a/src/steamcompmgr.cpp +++ b/src/steamcompmgr.cpp @@ -94,6 +94,7 @@ #include "BufferMemo.h" #include "Utils/Process.h" #include "Utils/Algorithm.h" +#include "Utils/Lock.h" #include "wlr_begin.hpp" #include "wlr/types/wlr_pointer_constraints_v1.h" @@ -5895,6 +5896,10 @@ error(Display *dpy, XErrorEvent *ev) return 0; } + +static const gamescope::CGlobalLockReference s_xwlLockReference{}; +using XwlLock = gamescope::ReferencedLock; + [[noreturn]] static void steamcompmgr_exit(std::optional> lock = std::nullopt) { @@ -5962,7 +5967,7 @@ static int handle_io_error(Display *dpy) { xwm_log.errorf("X11 I/O error"); - steamcompmgr_exit(); + steamcompmgr_exit( s_xwlLockReference.popLock() ); } static bool @@ -7512,7 +7517,7 @@ steamcompmgr_main(int argc, char **argv) init_runtime_info(); - std::unique_lock xwayland_server_guard(g_SteamCompMgrXWaylandServerMutex); + XwlLock xwayland_server_guard(g_SteamCompMgrXWaylandServerMutex); // Initialize any xwayland ctxs we have { @@ -8060,7 +8065,7 @@ steamcompmgr_main(int argc, char **argv) vblank = false; } - steamcompmgr_exit(std::optional> {std::in_place_t{}, std::move(xwayland_server_guard)} ); + steamcompmgr_exit( xwayland_server_guard.popLock() ); } void steamcompmgr_send_frame_done_to_focus_window() From bf16c7e0e4ce4a74a447f4ffd0ff18f679927016 Mon Sep 17 00:00:00 2001 From: sharkautarch <128002472+sharkautarch@users.noreply.github.com> Date: Fri, 13 Dec 2024 08:58:21 -0500 Subject: [PATCH 5/5] steamcompmgr: fix another coredump-at-exit --- src/backend.cpp | 22 +++++++++++----------- src/rendervulkan.hpp | 9 +++++++++ src/steamcompmgr.cpp | 27 ++++++++++++++++++++++----- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/backend.cpp b/src/backend.cpp index 8a6bbe8ed9..a15e0e5051 100644 --- a/src/backend.cpp +++ b/src/backend.cpp @@ -16,34 +16,34 @@ namespace gamescope // IBackend ///////////// - static IBackend *s_pBackend = nullptr; + static std::atomic s_pBackend = nullptr; IBackend *IBackend::Get() { return s_pBackend; } - + + bool IBackend::Set( IBackend *pBackend ) { - if ( s_pBackend ) - { - delete s_pBackend; - s_pBackend = nullptr; - } + //FIXME: calling the backend's destructor sometimes hangs gamescope for wayland backend + //if ( s_pBackend ) { + // std::destroy_at(GetBackend()); + //} if ( pBackend ) { s_pBackend = pBackend; - if ( !s_pBackend->Init() ) + if ( !GetBackend()->Init() ) { - delete s_pBackend; - s_pBackend = nullptr; + s_pBackend = nullptr; return false; } } return true; } + ///////////////// // CBaseBackendFb @@ -184,4 +184,4 @@ namespace gamescope GetBackend()->DumpDebugInfo(); }); -} +} \ No newline at end of file diff --git a/src/rendervulkan.hpp b/src/rendervulkan.hpp index b967e849fe..0aea36c515 100644 --- a/src/rendervulkan.hpp +++ b/src/rendervulkan.hpp @@ -479,6 +479,15 @@ struct gamescope_color_mgmt_luts bHasLut1D = false; bHasLut3D = false; } + ~gamescope_color_mgmt_luts() + { + //need to set these to nullptr when destructing + //because otherwise, ASAN reports a use-after-free when the exit-time dtor runs + //(first steamcompmgr_exit() destructs this class, + // then the class is destructed again w/ the exit-time dtor for static objects) + vk_lut3d = nullptr; + vk_lut1d = nullptr; + } }; struct gamescope_color_mgmt_tracker_t diff --git a/src/steamcompmgr.cpp b/src/steamcompmgr.cpp index 11a7cade6c..76fdefcbeb 100644 --- a/src/steamcompmgr.cpp +++ b/src/steamcompmgr.cpp @@ -5896,8 +5896,25 @@ error(Display *dpy, XErrorEvent *ev) } [[noreturn]] static void -steamcompmgr_exit(void) +steamcompmgr_exit(std::optional> lock = std::nullopt) { + + // Need to clear all the vk_lutxd references (which can be tied to backend-allocated memory) + // for the colormgmt globals/statics, to avoid coredump at exit from within the colormgmt exit-time destructors: + for (auto& colorMgmtArr : + { + std::ref(g_ColorMgmtLuts), + std::ref(g_ColorMgmtLutsOverride), + std::ref(g_ScreenshotColorMgmtLuts), + std::ref(g_ScreenshotColorMgmtLutsHDR) + }) + { + for (auto& colorMgmt : colorMgmtArr.get()) + { + colorMgmt.gamescope_color_mgmt_luts::~gamescope_color_mgmt_luts(); //dtor call also calls all the subobjects' dtors + } + } + g_ImageWaiter.Shutdown(); // Clean up any commits. @@ -5922,7 +5939,6 @@ steamcompmgr_exit(void) { g_ColorMgmt.pending.appHDRMetadata = nullptr; g_ColorMgmt.current.appHDRMetadata = nullptr; - s_scRGB709To2020Matrix = nullptr; for (int i = 0; i < gamescope::GAMESCOPE_SCREEN_TYPE_COUNT; i++) { @@ -5930,13 +5946,14 @@ steamcompmgr_exit(void) s_MuraCTMBlob[i] = nullptr; } } - + if (lock) + lock->unlock(); gamescope::IBackend::Set( nullptr ); wlserver_lock(); wlserver_shutdown(); wlserver_unlock(false); - + pthread_exit(NULL); } @@ -8042,7 +8059,7 @@ steamcompmgr_main(int argc, char **argv) vblank = false; } - steamcompmgr_exit(); + steamcompmgr_exit(std::optional> {std::in_place_t{}, std::move(xwayland_server_guard)} ); } void steamcompmgr_send_frame_done_to_focus_window()