Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Peter Maydell]

On Thu, 20 Feb 2020 at 18:52, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> On 2/20/20 6:56 PM, Peter Maydell wrote:
> > On Mon, 17 Feb 2020 at 03:22, <address@hidden> wrote:
> >>
> >> From: Pan Nengyuan <address@hidden>
> >>
> >> There are some memleaks when we call 'device_list_properties'. This patch 
> >> move timer_new from init into realize to fix it.
> >> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we 
> >> move timer_new into realize().
> >>
> >> Reported-by: Euler Robot <address@hidden>
> >> Signed-off-by: Pan Nengyuan <address@hidden>
> >> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> >
> >
> >> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> >> index 19e154b870..980eda7599 100644
> >> --- a/hw/misc/mos6522.c
> >> +++ b/hw/misc/mos6522.c
> >> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
> >>       s->timers[0].frequency = s->frequency;
> >>       s->timers[0].latch = 0xffff;
> >>       set_counter(s, &s->timers[0], 0xffff);
> >> -    timer_del(s->timers[0].timer);
> >> +    if (s->timers[0].timer) {
> >> +        timer_del(s->timers[0].timer);
> >> +    }
> >>
> >>       s->timers[1].frequency = s->frequency;
> >>       s->timers[1].latch = 0xffff;
> >> -    timer_del(s->timers[1].timer);
> >> +    if (s->timers[1].timer) {
> >> +        timer_del(s->timers[1].timer);
> >> +    }
> >>   }
> >
> > What code path calls a device 'reset' method on a device
> > that has not yet been realized ? I wasn't expecting that
> > to be valid...
>
> This is not valid. What I understood while reviewing this patch is on
> reset the timer is removed from the timers list. But this patch miss
> setting timer = NULL in case the device is reset multiple times, here
> can happen a NULL deref.

I should have checked the APIs here.

timer_new() allocates memory and initialises a timer.
timer_del() removes a timer from any list it is on, but
does not deallocate memory. It's the function you call
to stop a timer (and arguably timer_stop() would be a
better name for it).
If you created the timer with timer_init(), then the
code to clean it up is:
 (1) call timer_del() to make sure it's not on any
list of active timers
 (2) call timer_free()

So:
 * the mos6522_reset code is fine as it is
 * if we wanted cleanup code that undoes the timer_new
   then that would be a timer_del() + timer_free().
   This would go in unrealize if the timer_new is put
   in realize, but...
 * ...like the other devices touched in this patch,
   mos6522 isn't user-creatable, so if realize succeeds
   it won't ever be destroyed; so we don't need to
   do that. (This is a little harder to check than
   with most of these devices, since mos6522 is an
   abstract base class for some other devices, but
   I think it's correct.)

Side notes:
 * for new code, rather than using timer_new() or one
   of its sibling functions, prefer timer_init(),
   timer_init_ns(), etc. These take a pointer to a
   pre-existing QEMUTimer, typically one you have
   directly embedded in the device state struct. So
   they don't need to be freed on unrealize (though
   you do still want to make sure the timer is not
   on an active list with timer_del() before the memory
   in the device state struct goes away).
 * maybe timer_free() should call timer_del(),
   rather than obliging the caller to?

thanks
-- PMM