WM: Fix invalid memory access in wmTimer handling code. #105380

Merged
Bastien Montagne merged 1 commits from mont29/blender:F-wmtimer-fix into blender-v3.5-release 2023-03-03 15:24:37 +01:00
4 changed files with 56 additions and 18 deletions

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

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 {

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) {
mont29 marked this conversation as resolved

Clearing wm->reports.reporttimer and event->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.

Clearing `wm->reports.reporttimer` and `event->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.
Review

Fair point, done.

Would keep MEM_freeN(wt->customdata) together with the actual timer freeing though, sounds safer to me?

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.

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.
Review

Ah OK I see now, done.

Ah OK I see now, done.
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

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)

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