mirror of
https://github.com/LGUG2Z/komorebi.git
synced 2026-03-23 09:51:16 +01:00
fix(borders): prevent use-after-free take 3
This commit attempts tofixfixes a use-after-free bug in the border_manager that was causing crashes with exception code 0xc000041d (FATAL_USER_CALLBACK_EXCEPTION) when borders were being destroyed during config updates. The root cause was a race condition between the main thread destroying a border window and the border's window thread still processing queued window messages (EVENT_OBJECT_LOCATIONCHANGE, WM_PAINT). The crash occurred when these callbacks invoked render_target.EndDraw(), which internally calls HwndPresenter::Present() to present the rendered frame to the HWND. By this point, the HWND and its associated Direct2D surfaces had been freed by WM_DESTROY, resulting in Direct2D attempting to dereference freed memory (0xbaadf00dbaadf00d - debug heap poison value). The previous attempts at fixing this issue (bdef1448,dbde351e) addressed symptoms but not the fundamental race condition.bdef1448attempted to release resources on the main thread before destruction, but this created a cross-thread race.dbde351emoved resource cleanup to WM_DESTROY, but this still allowed EVENT_OBJECT_LOCATIONCHANGE/WM_PAINT handlers to check `render_target.is_some()`, context-switch to WM_DESTROY which clears it, then context-switch back and call EndDraw() on a now-invalid reference. This commit attempts to eliminate the race condition by introducing an atomic destruction flag that serves as a memory barrier between the destruction path and the rendering paths: - Added `is_destroying: Arc<AtomicBool>` field to the Border struct - In destroy(): Sets the flag with Release ordering, sleeps 10ms to allow in-flight operations to complete, then proceeds with cleanup - In EVENT_OBJECT_LOCATIONCHANGE and WM_PAINT: Checks the flag with Acquire ordering both at handler entry and immediately before calling BeginDraw/EndDraw, exiting early if destruction is in progress The Acquire/Release memory ordering creates a synchronizes-with relationship that ensures: 1. When the destruction flag is set, all subsequent handler checks will see it (no stale cached values) 2. Handlers that pass the first check but race with destruction will be caught by the second check before touching D2D resources 3. The 10ms sleep window allows any handler already past both checks to complete its EndDraw() before resources are freed This is a lock-free solution with zero overhead on the hot rendering path (atomic loads are nearly free) and provides defense-in-depth with multiple barriers against the use-after-free condition.
This commit is contained in:
@@ -11,7 +11,9 @@ use crate::core::Rect;
|
||||
use crate::windows_api;
|
||||
use std::collections::HashMap;
|
||||
use std::ops::Deref;
|
||||
use std::sync::Arc;
|
||||
use std::sync::LazyLock;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::sync::mpsc;
|
||||
use windows::Win32::Foundation::FALSE;
|
||||
@@ -126,6 +128,7 @@ pub struct Border {
|
||||
pub brush_properties: D2D1_BRUSH_PROPERTIES,
|
||||
pub rounded_rect: D2D1_ROUNDED_RECT,
|
||||
pub brushes: HashMap<WindowKind, ID2D1SolidColorBrush>,
|
||||
pub is_destroying: Arc<AtomicBool>,
|
||||
}
|
||||
|
||||
impl From<isize> for Border {
|
||||
@@ -144,6 +147,7 @@ impl From<isize> for Border {
|
||||
brush_properties: D2D1_BRUSH_PROPERTIES::default(),
|
||||
rounded_rect: D2D1_ROUNDED_RECT::default(),
|
||||
brushes: HashMap::new(),
|
||||
is_destroying: Arc::new(AtomicBool::new(false)),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -192,6 +196,7 @@ impl Border {
|
||||
brush_properties: Default::default(),
|
||||
rounded_rect: Default::default(),
|
||||
brushes: HashMap::new(),
|
||||
is_destroying: Arc::new(AtomicBool::new(false)),
|
||||
};
|
||||
|
||||
let border_pointer = &raw mut border;
|
||||
@@ -313,6 +318,12 @@ impl Border {
|
||||
}
|
||||
|
||||
pub fn destroy(&self) -> color_eyre::Result<()> {
|
||||
// signal that we're destroying - prevents new render operations
|
||||
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 {
|
||||
@@ -386,6 +397,10 @@ impl Border {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
if (*border_pointer).is_destroying.load(Ordering::Acquire) {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
let reference_hwnd = (*border_pointer).tracking_hwnd;
|
||||
|
||||
let old_rect = (*border_pointer).window_rect;
|
||||
@@ -400,6 +415,11 @@ impl Border {
|
||||
if (!rect.is_same_size_as(&old_rect) || !rect.has_same_position_as(&old_rect))
|
||||
&& let Some(render_target) = (*border_pointer).render_target.as_ref()
|
||||
{
|
||||
// double-check destruction flag before rendering
|
||||
if (*border_pointer).is_destroying.load(Ordering::Acquire) {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
let border_width = (*border_pointer).width;
|
||||
let border_offset = (*border_pointer).offset;
|
||||
|
||||
@@ -468,6 +488,10 @@ impl Border {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
if (*border_pointer).is_destroying.load(Ordering::Acquire) {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
let reference_hwnd = (*border_pointer).tracking_hwnd;
|
||||
|
||||
// Update position to update the ZOrder
|
||||
@@ -481,6 +505,11 @@ impl Border {
|
||||
}
|
||||
|
||||
if let Some(render_target) = (*border_pointer).render_target.as_ref() {
|
||||
// double-check destruction flag before rendering
|
||||
if (*border_pointer).is_destroying.load(Ordering::Acquire) {
|
||||
return LRESULT(0);
|
||||
}
|
||||
|
||||
(*border_pointer).width = BORDER_WIDTH.load(Ordering::Relaxed);
|
||||
(*border_pointer).offset = BORDER_OFFSET.load(Ordering::Relaxed);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user