bartvdbraak/blender
1
0
Fork 4
Code Issues 4 Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

WM: Fix invalid memory access in wmTimer handling code.

Browse Source 
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
This commit is contained in:
Bastien Montagne 2023-03-02 17:21:29 +01:00 committed by Gitea
parent 24f3cb9b5c
commit d66672e17a
4 changed files with 56 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
source/blender/windowmanager/WM_api.h
View File

@ -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);

5
source/blender/windowmanager/WM_types.h
View File

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

63
source/blender/windowmanager/intern/wm_window.c
View File

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

4
source/blender/windowmanager/wm_window.h
View File

@ -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);