On 23.03.20 14:11, BALATON Zoltan 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. Ahci does for example:
>
> qemu_irq *irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
> for (i = 0; i < s->ports; i++) {
> ide_init2(&ad->port, irqs[i]);
> }
> g_free(irqs);
>
> So it allocates an array of s->ports irqs then only frees a single
> element?It doesn’t free a single element, it frees the array. > Also aren't these needed after ide_init2 to actually raise the > irq or are these end up copied to the irq field of the BMDMAState > sonehow? Where will that be freed? I don’t know where the array elements end up, but they aren’t freed by the g_free(). (irqs is an array of pointers, so freeing the array does not touch its elements, specifically it doesn’t free what those pointers point to.) >> Ideally we should free such memory in the DeviceUnrealize handler, but >> we in the reality we only care for hotunpluggable devices. >> PCI devices usually are. There is a trick however, you can mark >> overwrite the DeviceClass::hotpluggable field in sii3112_pci_class_init: >> >> dc->hotpluggable = false; > > If the above is correct then simply adding g_free(irq) after the loop at > end of sii3112_pci_realize should be enough but I can't tell if that's > correct. Setting hotpluggable to false does not seem to be a good fix. Well, if other code just uses g_free(irqs), it sounds good to me. But again, I don’t know anything about this code so far. Max
signature.asc
Description: OpenPGP digital signature
