[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.
Max
signature.asc
Description: OpenPGP digital signature
- Coverity CID 1421984, Max Reitz, 2020/03/23
- Re: Coverity CID 1421984, Philippe Mathieu-Daudé, 2020/03/23
- Re: Coverity CID 1421984, BALATON Zoltan, 2020/03/23
- Re: Coverity CID 1421984,
Max Reitz <=
- Re: Coverity CID 1421984, Peter Maydell, 2020/03/23
- Re: Coverity CID 1421984, BALATON Zoltan, 2020/03/23
- Re: Coverity CID 1421984, Peter Maydell, 2020/03/23
- Re: Coverity CID 1421984, BALATON Zoltan, 2020/03/23
- Re: Coverity CID 1421984, Peter Maydell, 2020/03/23
- Re: Coverity CID 1421984, BALATON Zoltan, 2020/03/23
- Re: Coverity CID 1421984, Peter Maydell, 2020/03/23
- Re: Coverity CID 1421984, Philippe Mathieu-Daudé, 2020/03/23