Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> In commit f3a508eb4e the Euler Robot reported calling timer_new()
> in instance_init() can leak heap memory. The easier fix is to
> delay the timer creation at instance realize(). Similarly move
> timer_del() into a new instance unrealize() method.
>
> This case was found with the following coccinelle script:
>
>     @ match @
>     identifier instance_init;
>     typedef Object;
>     identifier obj;
>     expression val, scale;
>     identifier clock_type, callback, opaque;
>     position pos;
>     @@
>     static void instance_init(Object *obj)
>     {
>       <...
>     (
>       val = timer_new@pos(clock_type, scale, callback, opaque);
>     |
>       val = timer_new_ns@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_us@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_ms@pos(clock_type, callback, opaque);
>     )
>       ...>
>     }
>
>     @ script:python @
>     f << match.instance_init;
>     p << match.pos;
>     @@
>     print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> Cc: Pan Nengyuan <address@hidden>
> ---
>  hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index f9a13e0a44..9144ac6c38 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, 
> Error **errp)
>
>      qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
>                               chr_event, NULL, ibe, NULL, true);
> +
> +    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, 
> ibe);
> +}
> +
> +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
> +
> +    timer_del(ibe->extern_timer);
>  }
>
>  static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
> @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>
> -    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, 
> ibe);
>      vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
>  }
>
> @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>
> -    timer_del(ibe->extern_timer);
>      timer_free(ibe->extern_timer);
>  }

So we now call timer_new in realize, and timer_del in unrealize,
but timer_free in finalize. This seems unbalanced -- why not
call timer_free in unrealize too, if we're moving things?

Also, this is a case of code that's actually doing things right:
we free the memory that we allocated in init in finalize. So
we're not fixing any leak here, we're just moving code around
(which is reasonable if we're trying to standardize on "call
timer_new in realize, not init", but should be noted in the
commit message).

thanks
-- PMM