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

[Philippe Mathieu-Daudé]

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()?

> > > -
> > >        /* memory mapping */
> > >        memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
> > >                              "exynos4210.uart",
> > > EXYNOS4210_UART_REGS_MEM_SIZE); @@ -691,6 +687,10 @@ static void
> > exynos4210_uart_realize(DeviceState *dev, Error **errp)
> > >    {
> > >        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;
> > > +
> > >        qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
> > >                                 exynos4210_uart_receive, exynos4210_uart_event,
> > >                                 NULL, s, NULL, true);