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

[Corey Minyard]

On Sat, Feb 15, 2020 at 04:47:05PM +0100, Philippe Mathieu-Daudé 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>

This looks ok to me:

Acked-by: Corey Minyard <address@hidden>

I can take it into my tree, if you like.

-corey

> ---
> 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);
>  }
>  
> @@ -528,6 +535,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, 
> void *data)
>      bk->handle_reset = ipmi_bmc_extern_handle_reset;
>      dc->hotpluggable = false;
>      dc->realize = ipmi_bmc_extern_realize;
> +    dc->unrealize = ipmi_bmc_extern_unrealize;
>      device_class_set_props(dc, ipmi_bmc_extern_properties);
>  }
>  
> -- 
> 2.21.1
>