Coverity CID 1421984

Coverity CID 1421984

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

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.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Coverity CID 1421984

[Philippe Mathieu-Daudé]

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.

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;

Re: Coverity CID 1421984

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]

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? 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?

> 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.

Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2604 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan <balaton@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

Re: Coverity CID 1421984

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

On Mon, 23 Mar 2020, Peter Maydell wrote:
> On Mon, 23 Mar 2020 at 13:12, BALATON Zoltan <balaton@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 I can't do that in sii3112 becuase I need to pass irq to this func:

void ide_init2(IDEBus *bus, qemu_irq irq);

That's what all ide devices do so you probably mean ide emulation should 
be qdevified in QEMU but that's way beyond fixing this Coverity warning.

> 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 :-)

That means other ide devices are likely also leaking just not noticed due 
to g_free-ing the array. For sii3112 I can implement an unrealize function 
that frees the allocated irqs which should fix it according the above.

Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > 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 I can't do that in sii3112 becuase I need to pass irq to this func:
>
> void ide_init2(IDEBus *bus, qemu_irq irq);

 ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);

should do what you want, I think.

thanks
-- PMM

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:
> On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Mon, 23 Mar 2020, Peter Maydell wrote:
>>> 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 I can't do that in sii3112 becuase I need to pass irq to this func:
>>
>> void ide_init2(IDEBus *bus, qemu_irq irq);
>
> ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);
>
> should do what you want, I think.

I don't understand what you mean. Sent a patch that I think might fix this 
warning for now. I'd leave qdevifying ide code to someone else.

Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Mon, 23 Mar 2020, Peter Maydell wrote:
> > On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >> On Mon, 23 Mar 2020, Peter Maydell wrote:
> >>> 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 I can't do that in sii3112 becuase I need to pass irq to this func:
> >>
> >> void ide_init2(IDEBus *bus, qemu_irq irq);
> >
> > ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);
> >
> > should do what you want, I think.
>
> I don't understand what you mean.

I mean that if you allocate the IRQs with qdev_init_gpio_in()
then the way to get a qemu_irq from within them to pass
to another function is to call qdev_get_gpio_in(). So you
just want to make your call to ide_init2() be the line I
suggest above.

> Sent a patch that I think might fix this
> warning for now. I'd leave qdevifying ide code to someone else.

There's no need to qdevify IDE for this.

thanks
-- PMM

Re: Coverity CID 1421984

[BALATON Zoltan]

On Mon, 23 Mar 2020, Peter Maydell wrote:
> On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Mon, 23 Mar 2020, Peter Maydell wrote:
>>> On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> On Mon, 23 Mar 2020, Peter Maydell wrote:
>>>>> 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 I can't do that in sii3112 becuase I need to pass irq to this func:
>>>>
>>>> void ide_init2(IDEBus *bus, qemu_irq irq);
>>>
>>> ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);
>>>
>>> should do what you want, I think.
>>
>> I don't understand what you mean.
>
> I mean that if you allocate the IRQs with qdev_init_gpio_in()
> then the way to get a qemu_irq from within them to pass
> to another function is to call qdev_get_gpio_in(). So you
> just want to make your call to ide_init2() be the line I
> suggest above.

Is this documented anywhere? Even after this reply I did not understand 
until I saw your patch. (And even after that I'm not quite sure when and 
how to use gpios so probably this should be made a bit more clear in 
docs or somewhere.)

Problem is that there are no complete docs so people are directed to look 
at the code. But the code is full of bad examples (like ahci and cmd646 in 
this case) so can't find out what's the latest preferred way. At least a 
list of preferred vs. legacy functions/APIs should exist somewhere and the 
preferred ones should be documented. Irq handling and gpios is one of the 
mysteries that are hard to get so some info on that would help.

Thanks for fixing this.

Regards,
BALATON Zoltan

Re: Coverity CID 1421984

[Peter Maydell]

On Mon, 23 Mar 2020 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Is this documented anywhere?

Unfortunately not. You're quite right that we should document this
(I hadn't realized/had forgotten that the qdev gpio APIs are
entirely undocumented -- they date from a time when we were
much less strict about asking for at least a doc-comment documentation
of new globally visible functions.)

thanks
-- PMM

Re: Coverity CID 1421984

[Philippe Mathieu-Daudé]

On 3/23/20 3:46 PM, Peter Maydell wrote:
> On Mon, 23 Mar 2020 at 14:43, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Mon, 23 Mar 2020, Peter Maydell wrote:
>>> On Mon, 23 Mar 2020 at 14:06, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> On Mon, 23 Mar 2020, Peter Maydell wrote:
>>>>> 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 I can't do that in sii3112 becuase I need to pass irq to this func:
>>>>
>>>> void ide_init2(IDEBus *bus, qemu_irq irq);
>>>
>>> ide_init2(bus, qdev_get_gpio_in(DEVICE(dev), i);
>>>
>>> should do what you want, I think.
>>
>> I don't understand what you mean.
> 
> I mean that if you allocate the IRQs with qdev_init_gpio_in()
> then the way to get a qemu_irq from within them to pass
> to another function is to call qdev_get_gpio_in(). So you
> just want to make your call to ide_init2() be the line I
> suggest above.

I understand Zoltan can have hard time understanding qdev_get_gpio_in() 
because it has no documentation. What would be the best example to follow?

> 
>> Sent a patch that I think might fix this
>> warning for now. I'd leave qdevifying ide code to someone else.
> 
> There's no need to qdevify IDE for this.
> 
> thanks
> -- PMM
>