On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan <address@hidden> wrote:
On Mon, 23 Mar 2020, Philippe Mathieu-Daudé wrote:
Cc'ing qemu-ppc since this is restricted to the aCube Sam460ex board.
On 3/23/20 12:46 PM, Max Reitz wrote:
Hi,
I was triaging new Coverity block layer reports today, and one that
seemed like a real bug was CID 1421984:
It complains about a memleak in sii3112_pci_realize() in
hw/ide/sii3112.c, specifically about @irq being leaked (it’s allocated
by qemu_allocate_irqs(), but never put anywhere or freed).
I’m not really well-versed in anything under hw/ide, so I was wondering
whether you agree it’s a bug and whether you know the correct way to fix
it. (I assume it’s just a g_free(irqs), but maybe there’s a more
specific way that’s applicable here.)
What does other devices is hold a reference in the DeviceState
(SiI3112PCIState) to make static analyzers happy.
Other IDE devices such as ahci and cmd646 seem to free it at the end of
the init function after calling ide_init2 with it. However it's not clear
to me how all this is supposed to work.
Anything that uses qemu_allocate_irqs() is old pre-qom/qdev code.
The qdev way of doing this kind of thing (ie "I am a device with
some inbound lines and this is the handler function to call when
the line is set") is to use qdev_init_gpio_in().
Coverity has flagged up a lot of leaks involving qemu_allocate_irqs();
most of them I've for the moment just set as "insignificant, fix
required" because they're in called-once functions like board init.
If this device can't be hot-unplugged and so we will only ever call
realize once, it would fall in that category too. Otherwise I'd
suggest conversion to qdev_init_gpio_in(). (This allocates arrays
of IRQs under the hood too, but the device_finalize() function will
automatically free them for you, so it's easier to use non-leakily.)
I think in the long term we should be thinking about getting rid
of all uses of qemu_allocate_irqs(): they seem to generally be
leaky.
The right way to free something allocated with qemu_allocate_irqs()
is to call qemu_free_irqs() on it, but that will free both the
array and all the IRQs within it, so you can't do that until the
device is destroyed. If the device can never be destroyed, we
usually don't write the unrealize function for it, so it would just
be a matter of storing the returned pointer from qemu_allocate_irqs()
in the device struct for a theoretical unrealize to be able to use.
If you just g_free() the pointer you got back then this leaves all
the IRQs themselves allocated, so you still have a nominal leak,
you've just swept it under the rug enough to stop Coverity seeing it :-)