[Qemu-devel] could somebody who understands the block refcounting look at CID 1395870, CID 1395871?

[Qemu-devel] could somebody who understands the block refcounting look at CID 1395870, CID 1395871?

[Peter Maydell]

Hi; could somebody who understands the block layer refcounting have
a look at Coverity issues CID 1395870 and 1395871, please? In both
cases, Coverity reports a use-after-free because it thinks that a
sequence where a code path might (conditionally) end up calling
blk_deref() twice could be freeing the memory in the first call
and using it after. I'm not sure whether these are false positives
because the refcounting has confused Coverity, or genuine issues where
we have got refcounting logic wrong, so I don't know if we need a
fix or if we should squash the coverity bug as a false-positive...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-block] could somebody who understands the block refcounting look at CID 1395870, CID 1395871?

[Max Reitz]

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

On 29.10.18 11:51, Peter Maydell wrote:
> Hi; could somebody who understands the block layer refcounting have
> a look at Coverity issues CID 1395870 and 1395871, please?

Don't forget 1395869.

> In both
> cases, Coverity reports a use-after-free because it thinks that a
> sequence where a code path might (conditionally) end up calling
> blk_deref() twice could be freeing the memory in the first call
> and using it after. I'm not sure whether these are false positives
> because the refcounting has confused Coverity, or genuine issues where
> we have got refcounting logic wrong, so I don't know if we need a
> fix or if we should squash the coverity bug as a false-positive...

It looks basically reasonable to me (just like 1395869).  All of these
block devices have two refcounts, one from the device state, and one
from the monitor.  These three places drop both refcounts after one another.

On first glance I found the order in qdev-properties-system.c a bit
weird because it unrefs the monitor reference first (which is definitely
there), and the device state reference only afterwards (which piix.c
implies may or may not be there).  However, @dev cannot be NULL
(otherwise "*ptr" would have segfaulted), so the device state reference
is guaranteed to be there.  OTOH, it appears that in this case the
monitor reference may be missing, so it's correct to try to drop that
reference first here, in case there is none.

So all looks good to me, I'll mark them as false positives (like Paolo
has done for 1395869 already).

Max


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