Re: [Qemu-devel] [PATCH 0/4] Fix memory leaks in QEMU

[Markus Armbruster]

Kirill Batuzov <address@hidden> writes:

> I tried running QEMU under Valgrind's Memcheck tool and managed to find
> some memory leaks.  I only checked "definitely lost" reports.  I ignored
> reports related to SDL/GTK because it is hard to tell if memory leak occurred
> in QEMU or in the library.
>
> All found errors followed one pattern:
>  1) Callee allocates some memory and returns pointer to the caller expecting 
> it
>     to free this memory later.
>  2) Caller knows nothing about the nature of the pointer returned and does not
>     perform the required cleanup.
>
> In actual code callee often does not allocate memory directly but calls some
> function that calls some other function ... that calls some other
> function that dynamically allocates memory.  Tracking pointers through this
> stacktrace is not an easy task without detailed knowledge of QEMU core.
>
> The things get even more complicated because QEMU has 4 different ways to
> allocate and deallocate memory mixed together:
>  1) malloc and free,

Should be used only when working with external APIs, e.g. when a library
function returns memory allocated with malloc(), and we free() it.

>  2) g_malloc and g_free,
>  3) tcg_malloc and tcg_pool_reset,
>  4) QObjects with reference counting.

Your list may not be exhaustive...

> List of culprits.
[...]
> qemu_allocate_irqs:
>   The most troublesome case.  It will need its own patch series and I need
>   some advices on how to deal with it.

I had a stab at these some time ago, but gave up.  As Peter noted, the
leaks are (mostly?) bounded, and therefore fairly harmless.

They're still annoying, because they pollute static and dynamic checker
results.

>   qemu_allocate_irqs allocates two arrays: one for actual IRQState structures
>   and one for pointers to IRQState structures also known as qemu_irq.  While
>   the first array is used during emulation process, the second one is only
>   needed to return pointers to the caller.  Elements of the second array are
>   eventually passed to sysbus_connect_irq or qdev_connect_gpio_out and get
>   copied in the process (they are passed by value).  After that the second
>   array is not needed anymore and all pointers to it get lost.  At that moment
>   memory leak occurs if the array is not free'd.  And it almost never does.

Yup.

>   qemu_allocate_irqs has poor interface.

Indeed.

>                                           Not only it returns pointer to
>   dynamically allocated memory and expects caller to free it, but it does so
>   in a very counterintuitive manner.  Caller still needs IRQs allocated by
>   qemu_allocate_irqs yet it must free returned memory at the same time.
>
>   Each emulated board needs interrupts so nearly every board calls
>   qemu_allocate_irqs at least once.  There are more than 90 occurrences in 
> QEMU
>   sources.  Changing it will affect every target.  Testing all of them will
>   be very hard. But I believe it should be done nevertheless.
>
>   Peculiar fact: out of 90+ calls to qemu_allocate_irqs 47 allocate 1
>   interrupt.  27 of them use the following weird code snippet:
>     some_function(qemu_allocate_irqs(handler, opaque, 1)[0]);
>   instead of using simpler qemu_allocate_irq.
>
>   The proper solution would be to introduce qemu_init_irqs function which
>   initializes already allocated array of qemu_irq, then rewrite every piece of
>   code calling qemu_allocate_irqs with either qemu_init_irqs or
>   qemu_allocate_irq, and then remove qemu_allocate_irqs completely.
>
>   Pros: we get rid of qemu_allocate_irqs in one go.
>   Cons: literally ever board gets affected. 
>
>   Another approach is to introduce new function qemu_init_irqs, mark
>   qemu_allocate_irqs as deprecated and gradually rewrite code over period of
>   time.  But I do not think it is viable.  QEMU core has complex API which is
>   not well documented.  As a result people learn it from examples.  And all
>   examples of interrupt allocation will be bad at that moment.  As a result
>   maintainers will need to keep very close watch to not allow new "bad" code
>   to slip into upstream.

Yes.

>   Any thoughts on how to deal with qemu_allocate_irqs? Is there any archive
>   of guest system images for testing purposes? The list on the wiki page
>   covers only small part of supported boards. 

As Peter mentioned already, we've been discussing more extensive changes
to the IRQ infrastructure, to integrate with QOM.  Those should also
take care of the poor interface, and its leaking misuses.