On Mon, 14 Dec 2020 at 20:30, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> Currently timer_free() is a simple wrapper for g_free(). This means
> that the timer being freed must not be currently active, as otherwise
> QEMU might crash later when the active list is processed and still
> has a pointer to freed memory on it. As a result almost all calls to
> timer_free() are preceded by a timer_del() call, as can be seen in
> the output of
> git grep -B1 '\<timer_free\>'
>
> This is unfortunate API design as it makes it easy to accidentally
> misuse (by forgetting the timer_del()), and the correct use is
> annoyingly verbose.
>
> Make timer_free() imply a timer_del(). We use the same check as
> timer_deinit() ("ts->expire_time == -1") to determine whether the
> timer is already deleted (although this is only saving the effort of
> re-iterating through the active list, as timer_del() on an
> already-deactivated timer is safe).
> +static inline void timer_free(QEMUTimer *ts)
> +{
> +
> + if (ts->expire_time != -1) {
> + timer_del(ts);
> + }
> + g_free(ts);
> +}
I was thinking about this again this morning, and I'm not sure
this is thread-safe. timer_del() itself is, and the timer code
only updates ts->expire_time with the timer's timer_list's
active_timers_lock held, but here we're reading expire_time
with no lock. So I think the right thing would be just to
drop the attempt at optimisation, and just
timer_del(ts);
g_free(ts);
I find it hard to imagine that timer_free() is going to be
in a code path where the slight overhead of checking the
active timer list is going to matter. (If it *did* matter,
the right place to put this "is the expire time -1?" check
would be in timer_del() itself, because that gets called in
a lot more places and it already takes the appropriate lock.)
thanks
-- PMM
