From 4ad739ec63c9a11f0537a884ae2a4c56d6bab10b Mon Sep 17 00:00:00 2001 From: Vaxry Date: Sat, 20 Apr 2024 20:16:42 +0100 Subject: [PATCH] HookSystem: improve callback safety --- src/debug/HyprNotificationOverlay.cpp | 2 +- src/desktop/Workspace.hpp | 8 +-- src/helpers/WLClasses.hpp | 6 +-- src/hyprerror/HyprError.cpp | 4 +- src/managers/CursorManager.cpp | 2 +- src/managers/HookSystemManager.cpp | 54 ++++++++++--------- src/managers/HookSystemManager.hpp | 20 +++---- src/managers/KeybindManager.cpp | 2 +- src/managers/input/InputMethodRelay.cpp | 2 +- src/plugins/PluginAPI.cpp | 18 ++----- src/plugins/PluginAPI.hpp | 12 +---- src/plugins/PluginSystem.hpp | 24 ++++----- src/protocols/Screencopy.hpp | 22 ++++---- src/protocols/TearingControl.cpp | 2 +- src/protocols/XDGOutput.cpp | 6 +-- src/render/OpenGL.cpp | 2 +- src/render/Renderer.cpp | 4 +- .../decorations/DecorationPositioner.cpp | 4 +- 18 files changed, 89 insertions(+), 105 deletions(-) diff --git a/src/debug/HyprNotificationOverlay.cpp b/src/debug/HyprNotificationOverlay.cpp index dffa6889..2f00d978 100644 --- a/src/debug/HyprNotificationOverlay.cpp +++ b/src/debug/HyprNotificationOverlay.cpp @@ -3,7 +3,7 @@ #include CHyprNotificationOverlay::CHyprNotificationOverlay() { - g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) { + static auto P = g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) { if (m_dNotifications.size() == 0) return; diff --git a/src/desktop/Workspace.hpp b/src/desktop/Workspace.hpp index db2fe25e..0008699b 100644 --- a/src/desktop/Workspace.hpp +++ b/src/desktop/Workspace.hpp @@ -78,11 +78,11 @@ class CWorkspace { void markInert(); private: - void init(PHLWORKSPACE self); + void init(PHLWORKSPACE self); - HOOK_CALLBACK_FN* m_pFocusedWindowHook = nullptr; - bool m_bInert = true; - std::weak_ptr m_pSelf; + std::shared_ptr m_pFocusedWindowHook; + bool m_bInert = true; + std::weak_ptr m_pSelf; }; inline bool valid(const PHLWORKSPACE& ref) { diff --git a/src/helpers/WLClasses.hpp b/src/helpers/WLClasses.hpp index b85517e8..89af21c3 100644 --- a/src/helpers/WLClasses.hpp +++ b/src/helpers/WLClasses.hpp @@ -270,9 +270,9 @@ struct STabletPad { }; struct SIdleInhibitor { - wlr_idle_inhibitor_v1* pWlrInhibitor = nullptr; - CWindow* pWindow = nullptr; - HOOK_CALLBACK_FN* onWindowDestroy = nullptr; + wlr_idle_inhibitor_v1* pWlrInhibitor = nullptr; + CWindow* pWindow = nullptr; + std::shared_ptr onWindowDestroy; DYNLISTENER(Destroy); diff --git a/src/hyprerror/HyprError.cpp b/src/hyprerror/HyprError.cpp index de386097..7a9e9e87 100644 --- a/src/hyprerror/HyprError.cpp +++ b/src/hyprerror/HyprError.cpp @@ -6,7 +6,7 @@ CHyprError::CHyprError() { m_fFadeOpacity.create(AVARTYPE_FLOAT, g_pConfigManager->getAnimationPropertyConfig("fadeIn"), AVARDAMAGE_NONE); m_fFadeOpacity.registerVar(); - g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) { + static auto P = g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) { if (!m_bIsCreated) return; @@ -14,7 +14,7 @@ CHyprError::CHyprError() { m_bMonitorChanged = true; }); - g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any param) { + static auto P2 = g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any param) { if (!m_bIsCreated) return; diff --git a/src/managers/CursorManager.cpp b/src/managers/CursorManager.cpp index 627ca5e1..68f020c2 100644 --- a/src/managers/CursorManager.cpp +++ b/src/managers/CursorManager.cpp @@ -50,7 +50,7 @@ CCursorManager::CCursorManager() { updateTheme(); - g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateTheme(); }); + static auto P = g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateTheme(); }); } void CCursorManager::dropBufferRef(CCursorManager::CCursorBuffer* ref) { diff --git a/src/managers/HookSystemManager.cpp b/src/managers/HookSystemManager.cpp index 48c300cb..11007f1a 100644 --- a/src/managers/HookSystemManager.cpp +++ b/src/managers/HookSystemManager.cpp @@ -7,30 +7,28 @@ CHookSystemManager::CHookSystemManager() { } // returns the pointer to the function -HOOK_CALLBACK_FN* CHookSystemManager::hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle) { - const auto PVEC = getVecForEvent(event); - const auto PFN = &m_lCallbackFunctions.emplace_back(fn); - PVEC->emplace_back(SCallbackFNPtr{PFN, handle}); - return PFN; +std::shared_ptr CHookSystemManager::hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle) { + std::shared_ptr hookFN = std::make_shared(fn); + m_mRegisteredHooks[event].emplace_back(SCallbackFNPtr{.fn = hookFN, .handle = handle}); + return hookFN; } -void CHookSystemManager::hookStatic(const std::string& event, HOOK_CALLBACK_FN* fn, HANDLE handle) { - const auto PVEC = getVecForEvent(event); - PVEC->emplace_back(SCallbackFNPtr{fn, handle}); -} +void CHookSystemManager::unhook(std::shared_ptr fn) { + for (auto& [k, v] : m_mRegisteredHooks) { + std::erase_if(v, [&](const auto& other) { + std::shared_ptr fn_ = other.fn.lock(); -void CHookSystemManager::unhook(HOOK_CALLBACK_FN* fn) { - std::erase_if(m_lCallbackFunctions, [&](const auto& other) { return &other == fn; }); - for (auto& [k, v] : m_lpRegisteredHooks) { - std::erase_if(v, [&](const auto& other) { return other.fn == fn; }); + return fn_.get() == fn.get(); + }); } } -void CHookSystemManager::emit(const std::vector* callbacks, SCallbackInfo& info, std::any data) { +void CHookSystemManager::emit(std::vector* const callbacks, SCallbackInfo& info, std::any data) { if (callbacks->empty()) return; std::vector faultyHandles; + bool needsDeadCleanup = false; for (auto& cb : *callbacks) { @@ -38,7 +36,11 @@ void CHookSystemManager::emit(const std::vector* callbacks, SCal if (!cb.handle) { // we don't guard hl hooks - (*cb.fn)(cb.fn, info, data); + + if (std::shared_ptr fn = cb.fn.lock()) + (*fn)(fn.get(), info, data); + else + needsDeadCleanup = true; continue; } @@ -48,9 +50,12 @@ void CHookSystemManager::emit(const std::vector* callbacks, SCal continue; try { - if (!setjmp(m_jbHookFaultJumpBuf)) - (*cb.fn)(cb.fn, info, data); - else { + if (!setjmp(m_jbHookFaultJumpBuf)) { + if (std::shared_ptr fn = cb.fn.lock()) + (*fn)(fn.get(), info, data); + else + needsDeadCleanup = true; + } else { // this module crashed. throw std::exception(); } @@ -61,6 +66,9 @@ void CHookSystemManager::emit(const std::vector* callbacks, SCal } } + if (needsDeadCleanup) + std::erase_if(*callbacks, [](const auto& fn) { return !fn.fn.lock(); }); + if (!faultyHandles.empty()) { for (auto& h : faultyHandles) g_pPluginSystem->unloadPlugin(g_pPluginSystem->getPluginByHandle(h), true); @@ -68,12 +76,8 @@ void CHookSystemManager::emit(const std::vector* callbacks, SCal } std::vector* CHookSystemManager::getVecForEvent(const std::string& event) { - auto IT = std::find_if(m_lpRegisteredHooks.begin(), m_lpRegisteredHooks.end(), [&](const auto& other) { return other.first == event; }); + if (!m_mRegisteredHooks.contains(event)) + Debug::log(LOG, "[hookSystem] New hook event registered: {}", event); - if (IT != m_lpRegisteredHooks.end()) - return &IT->second; - - Debug::log(LOG, "[hookSystem] New hook event registered: {}", event); - - return &m_lpRegisteredHooks.emplace_back(std::make_pair<>(event, std::vector{})).second; + return &m_mRegisteredHooks[event]; } \ No newline at end of file diff --git a/src/managers/HookSystemManager.hpp b/src/managers/HookSystemManager.hpp index ca8e9006..4b27fadf 100644 --- a/src/managers/HookSystemManager.hpp +++ b/src/managers/HookSystemManager.hpp @@ -16,8 +16,8 @@ typedef std::function HOOK_CALLBACK_FN; struct SCallbackFNPtr { - HOOK_CALLBACK_FN* fn = nullptr; - HANDLE handle = nullptr; + std::weak_ptr fn; + HANDLE handle = nullptr; }; #define EMIT_HOOK_EVENT(name, param) \ @@ -40,21 +40,21 @@ class CHookSystemManager { public: CHookSystemManager(); - // returns the pointer to the function - HOOK_CALLBACK_FN* hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle = nullptr); - void hookStatic(const std::string& event, HOOK_CALLBACK_FN* fn, HANDLE handle = nullptr); - void unhook(HOOK_CALLBACK_FN* fn); + // returns the pointer to the function. + // losing this pointer (letting it get destroyed) + // will equal to unregistering the callback. + [[nodiscard("Losing this pointer instantly unregisters the callback")]] std::shared_ptr hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, + HANDLE handle = nullptr); + void unhook(std::shared_ptr fn); - void emit(const std::vector* callbacks, SCallbackInfo& info, std::any data = 0); + void emit(std::vector* const callbacks, SCallbackInfo& info, std::any data = 0); std::vector* getVecForEvent(const std::string& event); bool m_bCurrentEventPlugin = false; jmp_buf m_jbHookFaultJumpBuf; private: - // todo: this is slow. Maybe static ptrs should be somehow allowed. unique ptr for vec? - std::list>> m_lpRegisteredHooks; - std::list m_lCallbackFunctions; + std::unordered_map> m_mRegisteredHooks; }; inline std::unique_ptr g_pHookSystem; \ No newline at end of file diff --git a/src/managers/KeybindManager.cpp b/src/managers/KeybindManager.cpp index 1758f333..39ede5d4 100644 --- a/src/managers/KeybindManager.cpp +++ b/src/managers/KeybindManager.cpp @@ -85,7 +85,7 @@ CKeybindManager::CKeybindManager() { m_tScrollTimer.reset(); - g_pHookSystem->hookDynamic("configReloaded", [this](void* hk, SCallbackInfo& info, std::any param) { + static auto P = g_pHookSystem->hookDynamic("configReloaded", [this](void* hk, SCallbackInfo& info, std::any param) { // clear cuz realloc'd m_pActiveKeybind = nullptr; m_vPressedSpecialBinds.clear(); diff --git a/src/managers/input/InputMethodRelay.cpp b/src/managers/input/InputMethodRelay.cpp index a2d75f20..17da87ea 100644 --- a/src/managers/input/InputMethodRelay.cpp +++ b/src/managers/input/InputMethodRelay.cpp @@ -3,7 +3,7 @@ #include "../../Compositor.hpp" CInputMethodRelay::CInputMethodRelay() { - g_pHookSystem->hookDynamic("keyboardFocus", [&](void* self, SCallbackInfo& info, std::any param) { onKeyboardFocus(std::any_cast(param)); }); + static auto P = g_pHookSystem->hookDynamic("keyboardFocus", [&](void* self, SCallbackInfo& info, std::any param) { onKeyboardFocus(std::any_cast(param)); }); } void CInputMethodRelay::onNewIME(wlr_input_method_v2* pIME) { diff --git a/src/plugins/PluginAPI.cpp b/src/plugins/PluginAPI.cpp index 78870666..c89918f5 100644 --- a/src/plugins/PluginAPI.cpp +++ b/src/plugins/PluginAPI.cpp @@ -13,30 +13,18 @@ APICALL const char* __hyprland_api_get_hash() { return GIT_COMMIT_HASH; } -APICALL bool HyprlandAPI::registerCallbackStatic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN* fn) { - auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle); - - if (!PLUGIN) - return false; - - g_pHookSystem->hookStatic(event, fn, handle); - PLUGIN->registeredCallbacks.emplace_back(std::make_pair<>(event, fn)); - - return true; -} - -APICALL HOOK_CALLBACK_FN* HyprlandAPI::registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn) { +APICALL std::shared_ptr HyprlandAPI::registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn) { auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle); if (!PLUGIN) return nullptr; - auto* const PFN = g_pHookSystem->hookDynamic(event, fn, handle); + auto PFN = g_pHookSystem->hookDynamic(event, fn, handle); PLUGIN->registeredCallbacks.emplace_back(std::make_pair<>(event, PFN)); return PFN; } -APICALL bool HyprlandAPI::unregisterCallback(HANDLE handle, HOOK_CALLBACK_FN* fn) { +APICALL bool HyprlandAPI::unregisterCallback(HANDLE handle, std::shared_ptr fn) { auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle); if (!PLUGIN) diff --git a/src/plugins/PluginAPI.hpp b/src/plugins/PluginAPI.hpp index 75dca24e..62db8e3b 100644 --- a/src/plugins/PluginAPI.hpp +++ b/src/plugins/PluginAPI.hpp @@ -132,28 +132,20 @@ namespace HyprlandAPI { */ APICALL Hyprlang::CConfigValue* getConfigValue(HANDLE handle, const std::string& name); - /* - Register a static (pointer) callback to a selected event. - Pointer must be kept valid until unregisterCallback() is called. - - returns: true on success, false on fail - */ - APICALL bool registerCallbackStatic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN* fn); - /* Register a dynamic (function) callback to a selected event. Pointer will be free'd by Hyprland on unregisterCallback(). returns: a pointer to the newly allocated function. nullptr on fail. */ - APICALL HOOK_CALLBACK_FN* registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn); + APICALL std::shared_ptr registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn); /* Unregisters a callback. If the callback was dynamic, frees the memory. returns: true on success, false on fail */ - APICALL bool unregisterCallback(HANDLE handle, HOOK_CALLBACK_FN* fn); + APICALL bool unregisterCallback(HANDLE handle, std::shared_ptr fn); /* Calls a hyprctl command. diff --git a/src/plugins/PluginSystem.hpp b/src/plugins/PluginSystem.hpp index ce2c2bb2..c618489a 100644 --- a/src/plugins/PluginSystem.hpp +++ b/src/plugins/PluginSystem.hpp @@ -8,22 +8,22 @@ class IHyprWindowDecoration; class CPlugin { public: - std::string name = ""; - std::string description = ""; - std::string author = ""; - std::string version = ""; + std::string name = ""; + std::string description = ""; + std::string author = ""; + std::string version = ""; - std::string path = ""; + std::string path = ""; - bool m_bLoadedWithConfig = false; + bool m_bLoadedWithConfig = false; - HANDLE m_pHandle = nullptr; + HANDLE m_pHandle = nullptr; - std::vector registeredLayouts; - std::vector registeredDecorations; - std::vector> registeredCallbacks; - std::vector registeredDispatchers; - std::vector> registeredHyprctlCommands; + std::vector registeredLayouts; + std::vector registeredDecorations; + std::vector>> registeredCallbacks; + std::vector registeredDispatchers; + std::vector> registeredHyprctlCommands; }; class CPluginSystem { diff --git a/src/protocols/Screencopy.hpp b/src/protocols/Screencopy.hpp index 0c709c7b..8facc53e 100644 --- a/src/protocols/Screencopy.hpp +++ b/src/protocols/Screencopy.hpp @@ -20,21 +20,21 @@ class CScreencopyClient { CScreencopyClient(); ~CScreencopyClient(); - int ref = 0; - wl_resource* resource = nullptr; + int ref = 0; + wl_resource* resource = nullptr; - eClientOwners clientOwner = CLIENT_SCREENCOPY; + eClientOwners clientOwner = CLIENT_SCREENCOPY; - int frameCounter = 0; - int framesInLastHalfSecond = 0; - CTimer lastMeasure; - CTimer lastFrame; - bool sentScreencast = false; + int frameCounter = 0; + int framesInLastHalfSecond = 0; + CTimer lastMeasure; + CTimer lastFrame; + bool sentScreencast = false; - void onTick(); - HOOK_CALLBACK_FN* tickCallback = nullptr; + void onTick(); + std::shared_ptr tickCallback; - bool operator==(const CScreencopyClient& other) const { + bool operator==(const CScreencopyClient& other) const { return resource == other.resource; } }; diff --git a/src/protocols/TearingControl.cpp b/src/protocols/TearingControl.cpp index 679f2404..21d37e3c 100644 --- a/src/protocols/TearingControl.cpp +++ b/src/protocols/TearingControl.cpp @@ -4,7 +4,7 @@ #include "../Compositor.hpp" CTearingControlProtocol::CTearingControlProtocol(const wl_interface* iface, const int& ver, const std::string& name) : IWaylandProtocol(iface, ver, name) { - g_pHookSystem->hookDynamic("destroyWindow", [this](void* self, SCallbackInfo& info, std::any param) { this->onWindowDestroy(std::any_cast(param)); }); + static auto P = g_pHookSystem->hookDynamic("destroyWindow", [this](void* self, SCallbackInfo& info, std::any param) { this->onWindowDestroy(std::any_cast(param)); }); } void CTearingControlProtocol::bindManager(wl_client* client, void* data, uint32_t ver, uint32_t id) { diff --git a/src/protocols/XDGOutput.cpp b/src/protocols/XDGOutput.cpp index 265b88ed..551d9d90 100644 --- a/src/protocols/XDGOutput.cpp +++ b/src/protocols/XDGOutput.cpp @@ -33,9 +33,9 @@ void CXDGOutputProtocol::bindManager(wl_client* client, void* data, uint32_t ver } CXDGOutputProtocol::CXDGOutputProtocol(const wl_interface* iface, const int& ver, const std::string& name) : IWaylandProtocol(iface, ver, name) { - g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); }); - g_pHookSystem->hookDynamic("configReloaded", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); }); - g_pHookSystem->hookDynamic("monitorRemoved", [this](void* self, SCallbackInfo& info, std::any param) { + static auto P = g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); }); + static auto P2 = g_pHookSystem->hookDynamic("configReloaded", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); }); + static auto P3 = g_pHookSystem->hookDynamic("monitorRemoved", [this](void* self, SCallbackInfo& info, std::any param) { const auto PMONITOR = std::any_cast(param); for (auto& o : m_vXDGOutputs) { if (o->monitor == PMONITOR) diff --git a/src/render/OpenGL.cpp b/src/render/OpenGL.cpp index 9e3fb47b..981541b2 100644 --- a/src/render/OpenGL.cpp +++ b/src/render/OpenGL.cpp @@ -52,7 +52,7 @@ CHyprOpenGLImpl::CHyprOpenGLImpl() { Debug::log(WARN, "!RENDERER: Using the legacy GLES2 renderer!"); #endif - g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any data) { preRender(std::any_cast(data)); }); + static auto P = g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any data) { preRender(std::any_cast(data)); }); RASSERT(eglMakeCurrent(wlr_egl_get_display(g_pCompositor->m_sWLREGL), EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT), "Couldn't unset current EGL!"); diff --git a/src/render/Renderer.cpp b/src/render/Renderer.cpp index 672adb1d..a51901b4 100644 --- a/src/render/Renderer.cpp +++ b/src/render/Renderer.cpp @@ -60,7 +60,7 @@ CHyprRenderer::CHyprRenderer() { // cursor hiding stuff - g_pHookSystem->hookDynamic("keyPress", [&](void* self, SCallbackInfo& info, std::any param) { + static auto P = g_pHookSystem->hookDynamic("keyPress", [&](void* self, SCallbackInfo& info, std::any param) { if (m_sCursorHiddenConditions.hiddenOnKeyboard) return; @@ -68,7 +68,7 @@ CHyprRenderer::CHyprRenderer() { ensureCursorRenderingMode(); }); - g_pHookSystem->hookDynamic("mouseMove", [&](void* self, SCallbackInfo& info, std::any param) { + static auto P2 = g_pHookSystem->hookDynamic("mouseMove", [&](void* self, SCallbackInfo& info, std::any param) { if (!m_sCursorHiddenConditions.hiddenOnKeyboard && m_sCursorHiddenConditions.hiddenOnTouch == g_pInputManager->m_bLastInputTouch && !m_sCursorHiddenConditions.hiddenOnTimeout) return; diff --git a/src/render/decorations/DecorationPositioner.cpp b/src/render/decorations/DecorationPositioner.cpp index 145a697b..4837382a 100644 --- a/src/render/decorations/DecorationPositioner.cpp +++ b/src/render/decorations/DecorationPositioner.cpp @@ -2,12 +2,12 @@ #include "../../Compositor.hpp" CDecorationPositioner::CDecorationPositioner() { - g_pHookSystem->hookDynamic("closeWindow", [this](void* call, SCallbackInfo& info, std::any data) { + static auto P = g_pHookSystem->hookDynamic("closeWindow", [this](void* call, SCallbackInfo& info, std::any data) { auto* const PWINDOW = std::any_cast(data); this->onWindowUnmap(PWINDOW); }); - g_pHookSystem->hookDynamic("openWindow", [this](void* call, SCallbackInfo& info, std::any data) { + static auto P2 = g_pHookSystem->hookDynamic("openWindow", [this](void* call, SCallbackInfo& info, std::any data) { auto* const PWINDOW = std::any_cast(data); this->onWindowMap(PWINDOW); });