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

[Eduardo Habkost]

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.

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.

-- 
Eduardo