From 57c46580213d20f18be672ead14bade5e468ce3d Mon Sep 17 00:00:00 2001 From: yuyoyuppe Date: Mon, 4 May 2020 17:29:48 +0300 Subject: [PATCH] FancyZones: prevent WinHookEventProc reentrancy bugs by serializing events to FancyZones::WndProc --- src/modules/fancyzones/dll/dllmain.cpp | 44 ++------- src/modules/fancyzones/lib/FancyZones.cpp | 89 +++++++++++++++---- src/modules/fancyzones/lib/FancyZones.h | 8 ++ .../fancyzones/lib/FancyZonesLib.vcxproj | 2 + .../lib/FancyZonesLib.vcxproj.filters | 6 ++ .../lib/FancyZonesWinHookEventIDs.cpp | 24 +++++ .../lib/FancyZonesWinHookEventIDs.h | 9 ++ src/modules/fancyzones/lib/pch.h | 2 + 8 files changed, 132 insertions(+), 52 deletions(-) create mode 100644 src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.cpp create mode 100644 src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.h diff --git a/src/modules/fancyzones/dll/dllmain.cpp b/src/modules/fancyzones/dll/dllmain.cpp index 62ce615737..c2c023884f 100644 --- a/src/modules/fancyzones/dll/dllmain.cpp +++ b/src/modules/fancyzones/dll/dllmain.cpp @@ -10,6 +10,7 @@ #include #include #include +#include extern "C" IMAGE_DOS_HEADER __ImageBase; @@ -74,6 +75,7 @@ public: { if (!m_app) { + initialize_winhook_event_ids(); Trace::FancyZones::EnableFancyZones(true); m_app = MakeFancyZones(reinterpret_cast(&__ImageBase), m_settings); @@ -186,9 +188,6 @@ private: intptr_t HandleKeyboardHookEvent(LowlevelKeyboardEvent* data) noexcept; void HandleWinHookEvent(WinHookEvent* data) noexcept; - void MoveSizeStart(HWND window, POINT const& ptScreen) noexcept; - void MoveSizeEnd(HWND window, POINT const& ptScreen) noexcept; - void MoveSizeUpdate(POINT const& ptScreen) noexcept; winrt::com_ptr m_app; winrt::com_ptr m_settings; @@ -242,14 +241,12 @@ intptr_t FancyZonesModule::HandleKeyboardHookEvent(LowlevelKeyboardEvent* data) void FancyZonesModule::HandleWinHookEvent(WinHookEvent* data) noexcept { - POINT ptScreen; - GetPhysicalCursorPos(&ptScreen); - + auto fzCallback = m_app.as(); switch (data->event) { case EVENT_SYSTEM_MOVESIZESTART: { - MoveSizeStart(data->hwnd, ptScreen); + fzCallback->HandleWinHookEvent(data); if (!m_objectLocationWinEventHook) { m_objectLocationWinEventHook = SetWinEventHook(EVENT_OBJECT_LOCATIONCHANGE, @@ -269,16 +266,13 @@ void FancyZonesModule::HandleWinHookEvent(WinHookEvent* data) noexcept { m_objectLocationWinEventHook = nullptr; } - MoveSizeEnd(data->hwnd, ptScreen); + fzCallback->HandleWinHookEvent(data); } break; case EVENT_OBJECT_LOCATIONCHANGE: { - if (m_app.as()->InMoveSize()) - { - MoveSizeUpdate(ptScreen); - } + fzCallback->HandleWinHookEvent(data); } break; @@ -298,10 +292,7 @@ void FancyZonesModule::HandleWinHookEvent(WinHookEvent* data) noexcept case EVENT_OBJECT_SHOW: case EVENT_OBJECT_CREATE: { - if (data->idObject == OBJID_WINDOW) - { - m_app.as()->WindowCreated(data->hwnd); - } + fzCallback->HandleWinHookEvent(data); } break; @@ -310,27 +301,6 @@ void FancyZonesModule::HandleWinHookEvent(WinHookEvent* data) noexcept } } -void FancyZonesModule::MoveSizeStart(HWND window, POINT const& ptScreen) noexcept -{ - if (auto monitor = MonitorFromPoint(ptScreen, MONITOR_DEFAULTTONULL)) - { - m_app.as()->MoveSizeStart(window, monitor, ptScreen); - } -} - -void FancyZonesModule::MoveSizeEnd(HWND window, POINT const& ptScreen) noexcept -{ - m_app.as()->MoveSizeEnd(window, ptScreen); -} - -void FancyZonesModule::MoveSizeUpdate(POINT const& ptScreen) noexcept -{ - if (auto monitor = MonitorFromPoint(ptScreen, MONITOR_DEFAULTTONULL)) - { - m_app.as()->MoveSizeUpdate(monitor, ptScreen); - } -} - extern "C" __declspec(dllexport) PowertoyModuleIface* __cdecl powertoy_create() { return new FancyZonesModule(); diff --git a/src/modules/fancyzones/lib/FancyZones.cpp b/src/modules/fancyzones/lib/FancyZones.cpp index d38a691e4c..ba55ea0bc2 100644 --- a/src/modules/fancyzones/lib/FancyZones.cpp +++ b/src/modules/fancyzones/lib/FancyZones.cpp @@ -1,6 +1,8 @@ #include "pch.h" -#include "common/dpi_aware.h" -#include "common/on_thread_executor.h" + +#include +#include +#include #include "FancyZones.h" #include "lib/Settings.h" @@ -8,13 +10,12 @@ #include "lib/JsonHelpers.h" #include "lib/ZoneSet.h" #include "lib/WindowMoveHandler.h" +#include "lib/FancyZonesWinHookEventIDs.h" +#include "lib/util.h" #include "trace.h" #include "VirtualDesktopUtils.h" -#include -#include -#include -#include +#include enum class DisplayChangeType { @@ -72,14 +73,46 @@ public: MoveSizeUpdate(HMONITOR monitor, POINT const& ptScreen) noexcept { std::unique_lock writeLock(m_lock); - m_windowMoveHandler.MoveSizeUpdate(monitor, ptScreen, m_zoneWindowMap); + m_windowMoveHandler.MoveSizeUpdate(monitor, ptScreen, m_zoneWindowMap); } IFACEMETHODIMP_(void) MoveSizeEnd(HWND window, POINT const& ptScreen) noexcept { std::unique_lock writeLock(m_lock); - m_windowMoveHandler.MoveSizeEnd(window, ptScreen, m_zoneWindowMap); + m_windowMoveHandler.MoveSizeEnd(window, ptScreen, m_zoneWindowMap); } + IFACEMETHODIMP_(void) + HandleWinHookEvent(const WinHookEvent* data) noexcept + { + const auto wparam = reinterpret_cast(data->hwnd); + const LONG lparam = 0; + std::shared_lock readLock(m_lock); + switch (data->event) + { + case EVENT_SYSTEM_MOVESIZESTART: + PostMessageW(m_window, WM_PRIV_MOVESIZESTART, wparam, lparam); + break; + case EVENT_SYSTEM_MOVESIZEEND: + PostMessageW(m_window, WM_PRIV_MOVESIZEEND, wparam, lparam); + break; + case EVENT_OBJECT_LOCATIONCHANGE: + PostMessageW(m_window, WM_PRIV_LOCATIONCHANGE, wparam, lparam); + break; + case EVENT_OBJECT_NAMECHANGE: + PostMessageW(m_window, WM_PRIV_NAMECHANGE, wparam, lparam); + break; + + case EVENT_OBJECT_UNCLOAKED: + case EVENT_OBJECT_SHOW: + case EVENT_OBJECT_CREATE: + if (data->idObject == OBJID_WINDOW) + { + PostMessageW(m_window, WM_PRIV_WINDOWCREATED, wparam, lparam); + } + break; + } + } + IFACEMETHODIMP_(void) VirtualDesktopChanged() noexcept; IFACEMETHODIMP_(void) @@ -189,7 +222,7 @@ private: void MoveWindowsOnDisplayChange() noexcept; void CycleActiveZoneSet(DWORD vkCode) noexcept; bool OnSnapHotkey(DWORD vkCode) noexcept; - + void RegisterVirtualDesktopUpdates(std::vector& ids) noexcept; void RegisterNewWorkArea(GUID virtualDesktopId, HMONITOR monitor) noexcept; bool IsNewWorkArea(GUID virtualDesktopId, HMONITOR monitor) noexcept; @@ -204,7 +237,7 @@ private: mutable std::shared_mutex m_lock; HWND m_window{}; WindowMoveHandler m_windowMoveHandler; - + std::map> m_zoneWindowMap; // Map of monitor to ZoneWindow (one per monitor) winrt::com_ptr m_settings{}; GUID m_currentVirtualDesktopId{}; // UUID of the current virtual desktop. Is GUID_NULL until first VD switch per session. @@ -263,8 +296,7 @@ FancyZones::Run() noexcept .wait(); m_terminateVirtualDesktopTrackerEvent.reset(CreateEvent(nullptr, FALSE, FALSE, nullptr)); - m_virtualDesktopTrackerThread.submit(OnThreadExecutor::task_t{ [&] { - VirtualDesktopUtils::HandleVirtualDesktopUpdates(m_window, WM_PRIV_VD_UPDATE, m_terminateVirtualDesktopTrackerEvent.get()); } }); + m_virtualDesktopTrackerThread.submit(OnThreadExecutor::task_t{ [&] { VirtualDesktopUtils::HandleVirtualDesktopUpdates(m_window, WM_PRIV_VD_UPDATE, m_terminateVirtualDesktopTrackerEvent.get()); } }); } // IFancyZones @@ -289,8 +321,8 @@ FancyZones::Destroy() noexcept IFACEMETHODIMP_(void) FancyZones::VirtualDesktopChanged() noexcept { - // VirtualDesktopChanged is called from another thread but results in new windows being created. - // Jump over to the UI thread to handle it. + // VirtualDesktopChanged is called from a reentrant WinHookProc function, therefore we must postpone the actual logic + // until we're in FancyZones::WndProc, which is not reentrant. std::shared_lock readLock(m_lock); PostMessage(m_window, WM_PRIV_VD_SWITCH, 0, 0); } @@ -557,6 +589,9 @@ LRESULT FancyZones::WndProc(HWND window, UINT message, WPARAM wparam, LPARAM lpa default: { + POINT ptScreen; + GetPhysicalCursorPos(&ptScreen); + if (message == WM_PRIV_VD_INIT) { OnDisplayChange(DisplayChangeType::Initialization); @@ -587,6 +622,31 @@ LRESULT FancyZones::WndProc(HWND window, UINT message, WPARAM wparam, LPARAM lpa m_terminateEditorEvent.release(); } } + else if (message == WM_PRIV_MOVESIZESTART) + { + auto hwnd = reinterpret_cast(wparam); + if (auto monitor = MonitorFromPoint(ptScreen, MONITOR_DEFAULTTONULL)) + { + MoveSizeStart(hwnd, monitor, ptScreen); + } + } + else if (message == WM_PRIV_MOVESIZEEND) + { + auto hwnd = reinterpret_cast(wparam); + MoveSizeEnd(hwnd, ptScreen); + } + else if (message == WM_PRIV_LOCATIONCHANGE && InMoveSize()) + { + if (auto monitor = MonitorFromPoint(ptScreen, MONITOR_DEFAULTTONULL)) + { + MoveSizeUpdate(monitor, ptScreen); + } + } + else if (message == WM_PRIV_WINDOWCREATED) + { + auto hwnd = reinterpret_cast(wparam); + WindowCreated(hwnd); + } else { return DefWindowProc(window, message, wparam, lparam); @@ -672,7 +732,6 @@ void FancyZones::AddZoneWindow(HMONITOR monitor, PCWSTR deviceId) noexcept } } - LRESULT CALLBACK FancyZones::s_WndProc(HWND window, UINT message, WPARAM wparam, LPARAM lparam) noexcept { auto thisRef = reinterpret_cast(GetWindowLongPtr(window, GWLP_USERDATA)); diff --git a/src/modules/fancyzones/lib/FancyZones.h b/src/modules/fancyzones/lib/FancyZones.h index 47b368098d..ce30d3f65b 100644 --- a/src/modules/fancyzones/lib/FancyZones.h +++ b/src/modules/fancyzones/lib/FancyZones.h @@ -4,6 +4,8 @@ interface IZoneWindow; interface IFancyZonesSettings; interface IZoneSet; +struct WinHookEvent; + interface __declspec(uuid("{50D3F0F5-736E-4186-BDF4-3D6BEE150C3A}")) IFancyZones : public IUnknown { /** @@ -54,6 +56,12 @@ interface __declspec(uuid("{2CB37E8F-87E6-4AEC-B4B2-E0FDC873343F}")) IFancyZones * Inform FancyZones that user has switched between virtual desktops. */ IFACEMETHOD_(void, VirtualDesktopChanged)() = 0; + /** + * Callback from WinEventHook to FancyZones + * + * @param data Handle of window being moved or resized. + */ + IFACEMETHOD_(void, HandleWinHookEvent)(const WinHookEvent* data){}; /** * Inform FancyZones that new window is created. FancyZones will try to assign it to the * zone insde active zone layout (if information about last zone, in which window was located diff --git a/src/modules/fancyzones/lib/FancyZonesLib.vcxproj b/src/modules/fancyzones/lib/FancyZonesLib.vcxproj index c4eaecd69a..55e9899215 100644 --- a/src/modules/fancyzones/lib/FancyZonesLib.vcxproj +++ b/src/modules/fancyzones/lib/FancyZonesLib.vcxproj @@ -95,6 +95,7 @@ + @@ -109,6 +110,7 @@ + Create diff --git a/src/modules/fancyzones/lib/FancyZonesLib.vcxproj.filters b/src/modules/fancyzones/lib/FancyZonesLib.vcxproj.filters index 2080ff54f3..de21273593 100644 --- a/src/modules/fancyzones/lib/FancyZonesLib.vcxproj.filters +++ b/src/modules/fancyzones/lib/FancyZonesLib.vcxproj.filters @@ -51,6 +51,9 @@ Header Files + + Header Files + @@ -86,6 +89,9 @@ Source Files + + Source Files + diff --git a/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.cpp b/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.cpp new file mode 100644 index 0000000000..631f80f43c --- /dev/null +++ b/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.cpp @@ -0,0 +1,24 @@ +#include "pch.h" + +#include + +#include "FancyZonesWinHookEventIDs.h" + +UINT WM_PRIV_MOVESIZESTART; +UINT WM_PRIV_MOVESIZEEND; +UINT WM_PRIV_LOCATIONCHANGE; +UINT WM_PRIV_NAMECHANGE; +UINT WM_PRIV_WINDOWCREATED; + +std::once_flag init_flag; + +void initialize_winhook_event_ids() +{ + std::call_once(init_flag, [&] { + WM_PRIV_MOVESIZESTART = RegisterWindowMessage(L"{f48def23-df42-4c0f-a13d-3eb4a9e204d4}"); + WM_PRIV_MOVESIZEEND = RegisterWindowMessage(L"{805d643c-804d-4728-b533-907d760ebaf0}"); + WM_PRIV_LOCATIONCHANGE = RegisterWindowMessage(L"{d56c5ee7-58e5-481c-8c4f-8844cf4d0347}"); + WM_PRIV_NAMECHANGE = RegisterWindowMessage(L"{b7b30c61-bfa0-4d95-bcde-fc4f2cbf6d76}"); + WM_PRIV_WINDOWCREATED = RegisterWindowMessage(L"{bdb10669-75da-480a-9ec4-eeebf09a02d7}"); + }); +} diff --git a/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.h b/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.h new file mode 100644 index 0000000000..51dd46363e --- /dev/null +++ b/src/modules/fancyzones/lib/FancyZonesWinHookEventIDs.h @@ -0,0 +1,9 @@ +#pragma once + +extern UINT WM_PRIV_MOVESIZESTART; +extern UINT WM_PRIV_MOVESIZEEND; +extern UINT WM_PRIV_LOCATIONCHANGE; +extern UINT WM_PRIV_NAMECHANGE; +extern UINT WM_PRIV_WINDOWCREATED; + +void initialize_winhook_event_ids(); diff --git a/src/modules/fancyzones/lib/pch.h b/src/modules/fancyzones/lib/pch.h index 700520d431..ac4132707f 100644 --- a/src/modules/fancyzones/lib/pch.h +++ b/src/modules/fancyzones/lib/pch.h @@ -17,6 +17,8 @@ #include #include #include +#include +#include #pragma comment(lib, "windowsapp")