starred/komorebi
1
0
Fork 0
mirror of https://github.com/LGUG2Z/komorebi.git synced 2026-03-31 06:23:13 +02:00
Code Issues 87 Packages Projects Releases 10 Wiki Activity

fix(borders): prevent use-after-free take 4

Browse Source 
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<Border> 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<Border>, 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.
This commit is contained in:
LGUG2Z
2026-03-29 19:09:13 -07:00
parent d7580d2271
commit 26b1464381
2 changed files with 52 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
komorebi/src/border_manager/border.rs
View File

@@ -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() {

21
komorebi/src/border_manager/mod.rs
View File

@@ -451,8 +451,11 @@ pub fn handle_notifications(wm: Arc<Mutex<WindowManager>>) -> 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<Mutex<WindowManager>>) -> 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();