Re: [Qemu-devel] QEMU xen coverity issues

[Kevin Wolf <kwolf@redhat.com>]

Am 18.02.2019 um 11:28 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Sent: 18 February 2019 10:09
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: 'Peter Maydell' <peter.maydell@linaro.org>; QEMU Developers <qemu-
> > devel@nongnu.org>; Anthony Perard <anthony.perard@citrix.com>
> > Subject: Re: [Qemu-devel] QEMU xen coverity issues
> > 
> > Am 15.02.2019 um 17:20 hat Paul Durrant geschrieben:
> > > > -----Original Message-----
> > > [snip]
> > > > >
> > > > > (5) CID 1398649: resource leak in xen_block_drive_create():
> > > > >
> > > > > In hw/block/xen-block.c xen_block_drive_create() Coverity
> > > > > complains that the call "driver_layer = qdict_new()" allocates
> > > > > memory that's leaked because we don't save the pointer anywhere
> > > > > but don't deallocate it before the end of the function either.
> > > > > Coverity is not great at understanding our refcounting objects,
> > > > > but this does look like either we're missing a qobject_unref()
> > > > > or something should be keeping hold of the dictionary. Probably
> > > > > best to ask a block layer expert.
> > > >
> > > > AFAICT nothing will consume the dictionary so it does appear that
> > we're
> > > > missing an unref here.
> > >
> > > Testing proves me wrong... This one is a false positive.
> > 
> > Hm, but where is it freed?
> > 
> > xen_block_blockdev_add() only feeds it to an input visitor, which
> > doesn't take ownership of the QDict (and in the first error path, it
> > hasn't even done that yet).
> 
> Agreed that error path does not free things... that's definitely a
> leak... but attempting to free the QDict's on return from
> xen_block_blockdev_add() certainly causes a seg fault. My assumption
> was that, having been fed through the input visitor and then through
> the output visitor in qmp_blockdev_add() that the BlockDriverState
> eventually takes ownership... but maybe that's not true?

qmp_blockdev_add() only ever sees the QAPI object, not the original
QDict, so it should be able to take ownership of it. If anything, the
visitor could do so, but I don't think it does (it takes an extra
reference, which it frees at the end, but it doesn't free the reference
it was originally passed).

Maybe worth having another look at that segfault? It could point to a
related, but separate bug.

Kevin