From 26b14643819cdee38c3663515a730e56d365f4e4 Mon Sep 17 00:00:00 2001 From: LGUG2Z Date: Sun, 29 Mar 2026 19:09:13 -0700 Subject: [PATCH] fix(borders): prevent use-after-free take 4 This commit fixes a cross-thread use-after-free crash with exception code 0xc000041d (FATAL_USER_CALLBACK_EXCEPTION), identified via WinDbg analysis of a minidump where rax=0xfeeefeeefeeefeee at the crash site in d2d1!HwndPresenter::Present - the Windows heap freed-memory fill pattern confirming Direct2D dereferenced a previously freed object. The root cause was a data race between the border manager thread and the border's own message loop thread. Border::create() spawns a dedicated thread (Thread B) for the HWND message loop and sends Box back to the border manager thread (Thread A) via a channel. After this point both threads accessed Border::render_target concurrently without any synchronisation: Thread A called update_brushes() directly on the Box, replacing render_target with a new ID2D1HwndRenderTarget and dropping the old one. Dropping the old RenderTarget decremented the COM refcount to zero, causing D2D to free its internal HwndPresenter. Thread B was concurrently mid-render in a WM_PAINT or EVENT_OBJECT_LOCATIONCHANGE handler, holding a reference to that same old render target obtained via the GWLP_USERDATA raw pointer. Calling EndDraw() after HwndPresenter was freed produced the crash. The process uptime of two seconds in the dump confirmed this happened during startup workspace initialisation, when a ForceUpdate notification triggered update_brushes() while the newly-shown border window was processing its first WM_PAINT. The fix routes all brush update requests through the border's own message loop by posting a custom WM_UPDATE_BRUSHES (WM_USER + 1) message instead of calling update_brushes() cross-thread. The three call sites in the border manager that previously called border.update_brushes()? directly now call border.request_brush_update(), which posts the message via PostMessageW. The WndProc handler for WM_UPDATE_BRUSHES calls update_brushes() and invalidate() entirely on Thread B, eliminating the race. A secondary bug in destroy() was also fixed: it was clearing GWLP_USERDATA before posting WM_CLOSE, which caused WM_DESTROY's null- pointer guard to skip the render_target = None cleanup. This left the ID2D1HwndRenderTarget alive past HWND destruction, and D2D freed its HwndPresenter during WM_NCDESTROY while the COM wrapper still held a reference - a second path to the same crash for any message queued between WM_NCDESTROY and WM_QUIT. The premature GWLP_USERDATA clear has been removed; WM_DESTROY already handles it correctly after releasing the render target. --- komorebi/src/border_manager/border.rs | 43 +++++++++++++++++++++++---- komorebi/src/border_manager/mod.rs | 21 +++++++++---- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/komorebi/src/border_manager/border.rs b/komorebi/src/border_manager/border.rs index e8054d81..ccad526f 100644 --- a/komorebi/src/border_manager/border.rs +++ b/komorebi/src/border_manager/border.rs @@ -58,6 +58,7 @@ use windows::Win32::UI::WindowsAndMessaging::GetWindowLongPtrW; use windows::Win32::UI::WindowsAndMessaging::IDC_ARROW; use windows::Win32::UI::WindowsAndMessaging::LoadCursorW; use windows::Win32::UI::WindowsAndMessaging::MSG; +use windows::Win32::UI::WindowsAndMessaging::PostMessageW; use windows::Win32::UI::WindowsAndMessaging::PostQuitMessage; use windows::Win32::UI::WindowsAndMessaging::SM_CXVIRTUALSCREEN; use windows::Win32::UI::WindowsAndMessaging::SetCursor; @@ -67,11 +68,16 @@ use windows::Win32::UI::WindowsAndMessaging::WM_CREATE; use windows::Win32::UI::WindowsAndMessaging::WM_DESTROY; use windows::Win32::UI::WindowsAndMessaging::WM_PAINT; use windows::Win32::UI::WindowsAndMessaging::WM_SETCURSOR; +use windows::Win32::UI::WindowsAndMessaging::WM_USER; use windows::Win32::UI::WindowsAndMessaging::WNDCLASSW; use windows_core::BOOL; use windows_core::PCWSTR; use windows_numerics::Matrix3x2; +/// Custom WM_USER message that tells the border window thread to call update_brushes() on itself, +/// avoiding a data race between the border manager thread and the border's message loop thread. +pub const WM_UPDATE_BRUSHES: u32 = WM_USER + 1; + pub struct RenderFactory(ID2D1Factory); unsafe impl Sync for RenderFactory {} unsafe impl Send for RenderFactory {} @@ -318,20 +324,31 @@ impl Border { } pub fn destroy(&self) -> color_eyre::Result<()> { - // signal that we're destroying - prevents new render operations + // signal that we're destroying - prevents new render operations from starting self.is_destroying.store(true, Ordering::Release); // small delay to allow in-flight render operations to complete std::thread::sleep(std::time::Duration::from_millis(10)); - // clear user data **BEFORE** closing window - // pending messages will see a null pointer and exit early - unsafe { - SetWindowLongPtrW(self.hwnd(), GWLP_USERDATA, 0); - } + // WM_DESTROY will clear GWLP_USERDATA and drop the render target before D2D + // frees its internal HwndPresenter during WM_NCDESTROY WindowsApi::close_window(self.hwnd) } + /// Post a message to the border's own message loop thread requesting a brush update. + /// This ensures update_brushes() always runs on the window thread that owns the D2D + /// render target, preventing a data race with concurrent WndProc render operations. + pub fn request_brush_update(&self) { + let _ = unsafe { + PostMessageW( + Option::from(self.hwnd()), + WM_UPDATE_BRUSHES, + WPARAM(0), + LPARAM(0), + ) + }; + } + pub fn set_position(&self, rect: &Rect, reference_hwnd: isize) -> color_eyre::Result<()> { let mut rect = *rect; rect.add_margin(self.width); @@ -576,6 +593,20 @@ impl Border { let _ = ValidateRect(Option::from(window), None); LRESULT(0) } + WM_UPDATE_BRUSHES => { + let border_pointer: *mut Border = GetWindowLongPtrW(window, GWLP_USERDATA) as _; + if border_pointer.is_null() { + return LRESULT(0); + } + if (*border_pointer).is_destroying.load(Ordering::Acquire) { + return LRESULT(0); + } + if let Err(error) = (*border_pointer).update_brushes() { + tracing::error!("failed to update brushes: {error}"); + } + (*border_pointer).invalidate(); + LRESULT(0) + } WM_DESTROY => { let border_pointer: *mut Border = GetWindowLongPtrW(window, GWLP_USERDATA) as _; if !border_pointer.is_null() { diff --git a/komorebi/src/border_manager/mod.rs b/komorebi/src/border_manager/mod.rs index 6326f826..a4f452f1 100644 --- a/komorebi/src/border_manager/mod.rs +++ b/komorebi/src/border_manager/mod.rs @@ -451,8 +451,11 @@ pub fn handle_notifications(wm: Arc>) -> color_eyre::Result } else if matches!(notification, Notification::ForceUpdate) { // Update the border brushes if there was a forced update // notification and this is not a new border (new border's - // already have their brushes updated on creation) - border.update_brushes()?; + // already have their brushes updated on creation). + // Post to the border's own thread to avoid a data race between + // this thread dropping the old render target and the window + // thread mid-render holding a reference to it. + border.request_brush_update(); } border.invalidate(); @@ -616,8 +619,11 @@ pub fn handle_notifications(wm: Arc>) -> color_eyre::Result if forced_update && !new_border { // Update the border brushes if there was a forced update // notification and this is not a new border (new border's - // already have their brushes updated on creation) - border.update_brushes()?; + // already have their brushes updated on creation). + // Post to the border's own thread to avoid a data race between + // this thread dropping the old render target and the window + // thread mid-render holding a reference to it. + border.request_brush_update(); } border.set_position(&rect, focused_window_hwnd)?; border.invalidate(); @@ -699,8 +705,11 @@ fn handle_floating_borders( if forced_update && !new_border { // Update the border brushes if there was a forced update // notification and this is not a new border (new border's - // already have their brushes updated on creation) - border.update_brushes()?; + // already have their brushes updated on creation). + // Post to the border's own thread to avoid a data race between + // this thread dropping the old render target and the window + // thread mid-render holding a reference to it. + border.request_brush_update(); } border.set_position(&rect, window.hwnd)?; border.invalidate();