Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init

[Philippe Mathieu-Daudé]

On 2/13/20 3:28 PM, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
>
> > On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
> > > Cc'ing Eduardo & Markus.
> > >
> > > On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
> > > > > -----Original Message-----
> > > > > From: Philippe Mathieu-Daudé [mailto:address@hidden]
> > > > > Sent: Wednesday, February 12, 2020 2:16 PM
> > > > > To: Chenqun (kuhn) <address@hidden>; qemu-
> > > > > address@hidden; address@hidden; address@hidden
> > > > > Cc: address@hidden; Zhanghailiang
> > > > > <address@hidden>
> > > > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
> > > > > exynos4210_uart_init
> > > > >
> > > > > On 2/12/20 4:36 AM, address@hidden wrote:
> > > > > > From: Chen Qun <address@hidden>
> > > > > >
> > > > > > It's easy to reproduce as follow:
> > > > > > virsh qemu-monitor-command vm1 --pretty '{"execute":
> > > > > > "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
> > > > > >
> > > > > > ASAN shows memory leak stack:
> > > > > >      #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
> > > > > >      #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
> > > > > >      #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
> > > > > >      #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
> > > > > >      #5 0xaaad270beee3 in exynos4210_uart_init
> > > > > /qemu/hw/char/exynos4210_uart.c:677
> > > > > >      #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
> > > > > >      #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
> > > > > >      #8 0xaaad2755df2f in qmp_device_list_properties
> > > > > > /qemu/qom/qom-qmp-cmds.c:152
> > > > > >
> > > > > > Reported-by: Euler Robot <address@hidden>
> > > > > > Signed-off-by: Chen Qun <address@hidden>
> > > > > > ---
> > > > > >     hw/char/exynos4210_uart.c | 8 ++++----
> > > > > >     1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > > > > > index 25d6588e41..5048db5410 100644
> > > > > > --- a/hw/char/exynos4210_uart.c
> > > > > > +++ b/hw/char/exynos4210_uart.c
> > > > > > @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
> > > > > >         SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > > > > >         Exynos4210UartState *s = EXYNOS4210_UART(dev);
> > > > > >
> > > > > > -    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > > > -                                         exynos4210_uart_timeout_int, s);
> > > > > > -    s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
> > > > >
> > > > > Why are you moving s->wordtime from init() to realize()?
> > > > Hi  Philippe,  thanks for your reply!
> > > >
> > > > Because I found the variable wordtime is usually used with fifo_timeout_timer.
> > > > Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
> > > >
> > > > I didn't find anything wrong with wordtime in the realize().
> > > > Does it have any other effects?
> > >
> > > 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.
>
> Sadly, our documentation on device initialization and realization is
> rather sparse, and developers are left guessing.  Their guesses are
> often based on what existing code does.  Some of the existing code even
> gets things right.
>
> A few rules from the top of my head:

Worth a new thread...

> * Creating and immediately destroying an object must be safe and free of
>    side effects: initialization may only touch the object itself, and
>    finalization must clean up everything initialization creates.
>
> * unrealize() must clean up everything realize() creates.

Hmm I guess remember someone once said "only for hot-pluggable objects, 
else don't bother". But then we make a non-hot-pluggable object as 
hot-pluggable and have to fix leaks. Or we start a new hot-pluggable 
device based on some code without unrealize()...

> * Since initialization cannot fail, code that needs to fail gracefully
>    must live in realize().
>
> * Since property values get set between initialization and realization,
>    code that uses property values must live in realize().
>
> * Dynamic properties have to be created in initialization to be visible
>    in introspection.
>
> > But as s->wordtime is a simple struct field (that we could even
> > decide to expose to the outside as a read-only QOM property), it
> > doesn't really matter.  Personally, I would keep it where it is
> > just to avoid churn.