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

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/timer.h | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index bdecc5b41fe..1789a600525 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -609,17 +609,6 @@ static inline QEMUTimer *timer_new_ms(QEMUClockType type, QEMUTimerCB *cb,
  */
 void timer_deinit(QEMUTimer *ts);
 
-/**
- * timer_free:
- * @ts: the timer
- *
- * Free a timer (it must not be on the active list)
- */
-static inline void timer_free(QEMUTimer *ts)
-{
-    g_free(ts);
-}
-
 /**
  * timer_del:
  * @ts: the timer
@@ -631,6 +620,22 @@ static inline void timer_free(QEMUTimer *ts)
  */
 void timer_del(QEMUTimer *ts);
 
+/**
+ * timer_free:
+ * @ts: the timer
+ *
+ * Free a timer. This will call timer_del() for you to remove
+ * the timer from the active list if it was still active.
+ */
+static inline void timer_free(QEMUTimer *ts)
+{
+
+    if (ts->expire_time != -1) {
+        timer_del(ts);
+    }
+    g_free(ts);
+}
+
 /**
  * timer_mod_ns:
  * @ts: the timer
-- 
2.20.1

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

On 15/12/20 12:44, Peter Maydell wrote:
> 
>> +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.

It may not be thread-safe in principle, but any code that calls 
timer_mod, and isn't itself protected by a lock against timer_free, will 
be racing against the g_free immediately after.  That is, that code 
could run after g_free and have a use-after-free bug.

But yes, I agree it is also an unnecessary optimization.  It's better 
done in timer_del_locked, and removed from timer_mod_anticipate_ns. 
Since you are at it, you may also want to push the call to 
timer_del_locked down from timer_mod_ns and timer_mod_anticipate_ns to 
their callee, timer_mod_ns_locked.

Thanks,

Paolo

On Tue, 15 Dec 2020 at 11:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 15/12/20 12:44, Peter Maydell wrote:
> >
> >> +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.
>
> It may not be thread-safe in principle, but any code that calls
> timer_mod, and isn't itself protected by a lock against timer_free, will
> be racing against the g_free immediately after.  That is, that code
> could run after g_free and have a use-after-free bug.

I was thinking about potential races between the thread doing
the timer_free() and the iothread trying to run timers. Or
can that not happen ?

> But yes, I agree it is also an unnecessary optimization.  It's better
> done in timer_del_locked, and removed from timer_mod_anticipate_ns.
> Since you are at it, you may also want to push the call to
> timer_del_locked down from timer_mod_ns and timer_mod_anticipate_ns to
> their callee, timer_mod_ns_locked.

One thing at a time :-)

thanks
-- PMM