Re: [PATCH 1/3] util/qemu-timer: Make timer_free() imply timer_del()

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 14 Dec 2020 at 20:30, Peter Maydell <peter.maydell@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