Re: [Qemu-devel] QEMU xen coverity issues

[Kevin Wolf]

Am 19.02.2019 um 17:17 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:address@hidden
> > Sent: 18 February 2019 10:59
> > To: Paul Durrant <address@hidden>
> > Cc: Anthony Perard <address@hidden>; 'Peter Maydell'
> > <address@hidden>; QEMU Developers <address@hidden>
> > Subject: Re: [Qemu-devel] QEMU xen coverity issues
> > 
> > Am 18.02.2019 um 11:28 hat Paul Durrant geschrieben:
> > > > -----Original Message-----
> > > > From: Kevin Wolf [mailto:address@hidden
> > > > Sent: 18 February 2019 10:09
> > > > To: Paul Durrant <address@hidden>
> > > > Cc: 'Peter Maydell' <address@hidden>; QEMU Developers <qemu-
> > > > address@hidden>; Anthony Perard <address@hidden>
> > > > 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.
> 
> What I'd failed to realize that was, having done a qdict_put_obj() to
> include the file_layer QDict in the driver_layer QDict, that doing a
> qobject_unref() on driver_layer would also implicitly unref the
> file_layer. I now have a patch that just unrefs driver_layer and that
> seems to be fine. I'll send it shortly.

Yes, that sounds right. qdict_put_obj() takes ownership of the reference
that you pass it, so we should only unref driver_layer.

Kevin