[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Coverity CID 1421984

From: Max Reitz
Subject: Re: Coverity CID 1421984
Date: Mon, 23 Mar 2020 14:26:13 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

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.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]