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

[Philippe Mathieu-Daudé]

On 2/17/20 2:25 PM, Peter Maydell wrote:
> 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).

While I understand your point, I am confused because the documentation 
on unrealize() and finalize() is rather scarce (and not obvious for 
non-native english speaker). I think I'm not understanding QOM instance 
lifetime well (in particular hotplug devices) so I will let this series go.

Similar confusions:

* https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg03079.html

Eduardo Habkost wrote:
> Philippe Mathieu-Daudé wrote:
>> IIUC when we use both init() and realize(), realize() should
>> only contains on code that consumes the object properties...
>> But maybe the design is not clear. Then why not move all the
>> init() code to realize()?
>
> Normally I would recommend the opposite: delay as much as
> possible to realize(), to avoid unwanted side effects when
> (e.g.) running qom-list-properties.

* https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04143.html

David Hildenbrand wrote:
>> Philippe Mathieu-Daudé wrote:
>> Similarly to your other cleanups, shouldn't we move the
>> timer_new_ns() into a realize() function, then do the
>> timer_del() in unrealize()?
>
> include/hw/qdev-core.h
>
> "Trivial field initializations should go into
> #TypeInfo.instance_init. Operations depending on @props
> static properties should go into @realize."

> thanks
> -- PMM