WM: Fix invalid memory access in wmTimer handling code. #105380
|
@ -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);
|
||||
|
|
|
@ -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 {
|
||||
|
|
|
@ -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) {
|
||||
mont29 marked this conversation as resolved
|
||||
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) {
|
||||
mont29 marked this conversation as resolved
Brecht Van Lommel
commented
I'd set this to NULL for clarity, rather not have any dangling pointer even if it should not be accessed. I'd set this to NULL for clarity, rather not have any dangling pointer even if it should not be accessed.
|
||||
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)
|
||||
|
|
|
@ -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);
|
||||
|
|
Loading…
Reference in New Issue
Clearing
wm->reports.reporttimer
andevent->customdata
should still happen immediately, not deferred.Not sure if it causes any actual issues, but no reason to take the risk.
MEM_freeN(wt->customdata)
could be done immediately as well.Fair point, done.
Would keep
MEM_freeN(wt->customdata)
together with the actual timer freeing though, sounds safer to me?I think it's better to be freed immediately so ASAN will catch any use of this memory after free. I don't think we have to hang onto memory in case there is buggy code somewhere using it.
Ah OK I see now, done.