`timerlist_run_timers` provides no mechanism to make sure the data pointed by `opaque` is valid when calling timer's callback: there could be another thread running which is destroying timer's opaque data.
With this change `timer_del` becomes blocking if timer's callback is running and it will be safe to destroy timer's data once `timer_del` returns. Signed-off-by: Roman Kiryanov <r...@google.com> --- v2: rebased to the right branch and removed Google specific tags from the commit message. include/qemu/timer.h | 4 ++++ util/qemu-timer.c | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/include/qemu/timer.h b/include/qemu/timer.h index 5ce83c7911..efd0e95d20 100644 --- a/include/qemu/timer.h +++ b/include/qemu/timer.h @@ -3,6 +3,7 @@ #include "qemu/bitops.h" #include "qemu/notify.h" +#include "qemu/thread.h" #include "qemu/host-utils.h" #define NANOSECONDS_PER_SECOND 1000000000LL @@ -86,9 +87,12 @@ struct QEMUTimer { QEMUTimerList *timer_list; QEMUTimerCB *cb; void *opaque; + QemuMutex opaque_lock; + QemuCond cb_done; QEMUTimer *next; int attributes; int scale; + bool cb_running; }; extern QEMUTimerListGroup main_loop_tlg; diff --git a/util/qemu-timer.c b/util/qemu-timer.c index 213114be68..95af255519 100644 --- a/util/qemu-timer.c +++ b/util/qemu-timer.c @@ -369,7 +369,10 @@ void timer_init_full(QEMUTimer *ts, ts->opaque = opaque; ts->scale = scale; ts->attributes = attributes; + qemu_mutex_init(&ts->opaque_lock); + qemu_cond_init(&ts->cb_done); ts->expire_time = -1; + ts->cb_running = false; } void timer_deinit(QEMUTimer *ts) @@ -394,6 +397,12 @@ static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts) } pt = &t->next; } + + qemu_mutex_lock(&ts->opaque_lock); + while (ts->cb_running) { + qemu_cond_wait(&ts->cb_done, &ts->opaque_lock); + } + qemu_mutex_unlock(&ts->opaque_lock); } static bool timer_mod_ns_locked(QEMUTimerList *timer_list, @@ -571,11 +580,23 @@ bool timerlist_run_timers(QEMUTimerList *timer_list) cb = ts->cb; opaque = ts->opaque; + /* Mark the callback as running to prevent + * destroying `opaque` in another thread. + */ + qemu_mutex_lock(&ts->opaque_lock); + ts->cb_running = true; + qemu_mutex_unlock(&ts->opaque_lock); + /* run the callback (the timer list can be modified) */ qemu_mutex_unlock(&timer_list->active_timers_lock); cb(opaque); qemu_mutex_lock(&timer_list->active_timers_lock); + qemu_mutex_lock(&ts->opaque_lock); + ts->cb_running = false; + qemu_cond_broadcast(&ts->cb_done); + qemu_mutex_unlock(&ts->opaque_lock); + progress = true; } qemu_mutex_unlock(&timer_list->active_timers_lock); -- 2.45.2.741.gdbec12cfda-goog