From d66672e17ac160957552bf2343407501cd0e127e Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 2 Mar 2023 17:21:29 +0100 Subject: [PATCH] WM: Fix invalid memory access in wmTimer handling code. Timer management code often loops over the list of timers, calling independant callbacks that end up freeing other timers in the list. That would result in potentail access-after-free errors, as reported in #105160. The typical identified scenario is wmTimer calling wmJob code, which calls some of the job's callbacks (`update` or `end` e.g.), which call `WM_report`, which removes and add another timer. To address this issue on a general level, the deletion of timers is now deferred, with the public API `WM_event_remove_timer` only marking the timer for deletion, and the private new function `wm_window_delete_removed_timers` effectively removing and deleting all marked timers. This implements design task #105369. Pull Request #105380 --- source/blender/windowmanager/WM_api.h | 2 + source/blender/windowmanager/WM_types.h | 5 ++ .../blender/windowmanager/intern/wm_window.c | 63 +++++++++++++------ source/blender/windowmanager/wm_window.h | 4 ++ 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/source/blender/windowmanager/WM_api.h b/source/blender/windowmanager/WM_api.h index d66cf407b55..6c3d72e5406 100644 --- a/source/blender/windowmanager/WM_api.h +++ b/source/blender/windowmanager/WM_api.h @@ -537,6 +537,8 @@ struct wmTimer *WM_event_add_timer_notifier(struct wmWindowManager *wm, struct wmWindow *win, unsigned int type, double timestep); +/** Mark the given `timer` to be removed, actual removal and deletion is deferred and handled + * internally by the window manager code. */ void WM_event_remove_timer(struct wmWindowManager *wm, struct wmWindow *win, struct wmTimer *timer); diff --git a/source/blender/windowmanager/WM_types.h b/source/blender/windowmanager/WM_types.h index 777793fb471..06c738c83a3 100644 --- a/source/blender/windowmanager/WM_types.h +++ b/source/blender/windowmanager/WM_types.h @@ -848,6 +848,11 @@ typedef struct wmXrActionData { typedef enum { /** Do not attempt to free custom-data pointer even if non-NULL. */ WM_TIMER_NO_FREE_CUSTOM_DATA = 1 << 0, + + /* Internal falgs, should not be used outside of WM code. */ + /** This timer has been tagged for removal and deletion, handled by WM code to ensure timers are + * deleted in a safe context. */ + WM_TIMER_TAGGED_FOR_REMOVAL = 1 << 16, } wmTimerFlags; typedef struct wmTimer { diff --git a/source/blender/windowmanager/intern/wm_window.c b/source/blender/windowmanager/intern/wm_window.c index ca0e3355557..d569ca0ecfc 100644 --- a/source/blender/windowmanager/intern/wm_window.c +++ b/source/blender/windowmanager/intern/wm_window.c @@ -231,6 +231,9 @@ void wm_window_free(bContext *C, wmWindowManager *wm, wmWindow *win) /* end running jobs, a job end also removes its timer */ LISTBASE_FOREACH_MUTABLE (wmTimer *, wt, &wm->timers) { + if (wt->flags & WM_TIMER_TAGGED_FOR_REMOVAL) { + continue; + } if (wt->win == win && wt->event_type == TIMERJOBS) { wm_jobs_timer_end(wm, wt); } @@ -238,10 +241,14 @@ void wm_window_free(bContext *C, wmWindowManager *wm, wmWindow *win) /* timer removing, need to call this api function */ LISTBASE_FOREACH_MUTABLE (wmTimer *, wt, &wm->timers) { + if (wt->flags & WM_TIMER_TAGGED_FOR_REMOVAL) { + continue; + } if (wt->win == win) { WM_event_remove_timer(wm, win, wt); } } + wm_window_delete_removed_timers(wm); if (win->eventstate) { MEM_freeN(win->eventstate); @@ -1547,6 +1554,9 @@ static bool wm_window_timer(const bContext *C) /* Mutable in case the timer gets removed. */ LISTBASE_FOREACH_MUTABLE (wmTimer *, wt, &wm->timers) { + if (wt->flags & WM_TIMER_TAGGED_FOR_REMOVAL) { + continue; + } wmWindow *win = wt->win; if (wt->sleep != 0) { @@ -1588,6 +1598,10 @@ static bool wm_window_timer(const bContext *C) } } } + + /* Effectively delete all timers marked for removal. */ + wm_window_delete_removed_timers(wm); + return has_event; } @@ -1832,6 +1846,9 @@ void WM_event_timer_sleep(wmWindowManager *wm, bool do_sleep) { LISTBASE_FOREACH (wmTimer *, wt, &wm->timers) { + if (wt->flags & WM_TIMER_TAGGED_FOR_REMOVAL) { + continue; + } if (wt == timer) { wt->sleep = do_sleep; break; @@ -1878,38 +1895,48 @@ wmTimer *WM_event_add_timer_notifier(wmWindowManager *wm, return wt; } +void wm_window_delete_removed_timers(wmWindowManager *wm) +{ + LISTBASE_FOREACH_MUTABLE (wmTimer *, wt, &wm->timers) { + if ((wt->flags & WM_TIMER_TAGGED_FOR_REMOVAL) == 0) { + continue; + } + + /* Actual removal and freeing of the timer. */ + BLI_remlink(&wm->timers, wt); + MEM_freeN(wt); + } +} + void WM_event_remove_timer(wmWindowManager *wm, wmWindow *UNUSED(win), wmTimer *timer) { - /* extra security check */ - wmTimer *wt = NULL; - LISTBASE_FOREACH (wmTimer *, timer_iter, &wm->timers) { - if (timer_iter == timer) { - wt = timer_iter; - } - } - if (wt == NULL) { + /* Extra security check. */ + if (BLI_findindex(&wm->timers, timer) < 0) { return; } - if (wm->reports.reporttimer == wt) { + timer->flags |= WM_TIMER_TAGGED_FOR_REMOVAL; + + /* Clear existing references to the timer. */ + if (wm->reports.reporttimer == timer) { wm->reports.reporttimer = NULL; } - - BLI_remlink(&wm->timers, wt); - if (wt->customdata != NULL && (wt->flags & WM_TIMER_NO_FREE_CUSTOM_DATA) == 0) { - MEM_freeN(wt->customdata); - } - MEM_freeN(wt); - - /* there might be events in queue with this timer as customdata */ + /* There might be events in queue with this timer as customdata. */ LISTBASE_FOREACH (wmWindow *, win, &wm->windows) { LISTBASE_FOREACH (wmEvent *, event, &win->event_queue) { - if (event->customdata == wt) { + if (event->customdata == timer) { event->customdata = NULL; event->type = EVENT_NONE; /* Timer users customdata, don't want `NULL == NULL`. */ } } } + + /* Immediately free customdata if requested, so that invalid usages of that data after + * calling `WM_event_remove_timer` can be easily spotted (through ASAN errors e.g.). */ + if (timer->customdata != NULL && (timer->flags & WM_TIMER_NO_FREE_CUSTOM_DATA) == 0) { + MEM_freeN(timer->customdata); + timer->customdata = NULL; + } } void WM_event_remove_timer_notifier(wmWindowManager *wm, wmWindow *win, wmTimer *timer) diff --git a/source/blender/windowmanager/wm_window.h b/source/blender/windowmanager/wm_window.h index 036a34a5140..599de3eb874 100644 --- a/source/blender/windowmanager/wm_window.h +++ b/source/blender/windowmanager/wm_window.h @@ -112,6 +112,10 @@ void wm_window_IME_begin(wmWindow *win, int x, int y, int w, int h, bool complet void wm_window_IME_end(wmWindow *win); #endif +/** Effectively remove timers from the list and delete them. Calling this should only be done by + * internal WM management code, from specific, safe places. */ +void wm_window_delete_removed_timers(wmWindowManager *wm); + /* *************** window operators ************** */ int wm_window_close_exec(bContext *C, struct wmOperator *op);