On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan <bala...@eik.bme.hu> 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 :-) thanks -- PMM
