Re: [Qemu-devel] QEMU xen coverity issues
Am 19.02.2019 um 17:17 hat Paul Durrant geschrieben: > > -Original Message- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 18 February 2019 10:59 > > To: Paul Durrant > > Cc: Anthony Perard ; 'Peter Maydell' > > ; QEMU Developers > > 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:kw...@redhat.com] > > > > Sent: 18 February 2019 10:09 > > > > To: Paul Durrant > > > > Cc: 'Peter Maydell' ; QEMU Developers > > > de...@nongnu.org>; Anthony Perard > > > > 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
Re: [Qemu-devel] QEMU xen coverity issues
> -Original Message- > From: Kevin Wolf [mailto:kw...@redhat.com] > Sent: 18 February 2019 10:59 > To: Paul Durrant > Cc: Anthony Perard ; 'Peter Maydell' > ; QEMU Developers > 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:kw...@redhat.com] > > > Sent: 18 February 2019 10:09 > > > To: Paul Durrant > > > Cc: 'Peter Maydell' ; QEMU Developers > > de...@nongnu.org>; Anthony Perard > > > 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. Paul > > Kevin
Re: [Qemu-devel] QEMU xen coverity issues
Am 18.02.2019 um 11:28 hat Paul Durrant geschrieben: > > -Original Message- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 18 February 2019 10:09 > > To: Paul Durrant > > Cc: 'Peter Maydell' ; QEMU Developers > de...@nongnu.org>; Anthony Perard > > 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
Re: [Qemu-devel] QEMU xen coverity issues
> -Original Message- > From: Kevin Wolf [mailto:kw...@redhat.com] > Sent: 18 February 2019 10:09 > To: Paul Durrant > Cc: 'Peter Maydell' ; QEMU Developers de...@nongnu.org>; Anthony Perard > 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? Paul > > Kevin
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). Kevin
Re: [Qemu-devel] QEMU xen coverity issues
> -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. Paul
Re: [Qemu-devel] QEMU xen coverity issues
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: 14 February 2019 18:29
> To: QEMU Developers
> Cc: Paul Durrant ; Anthony Perard
>
> Subject: QEMU xen coverity issues
>
> Hi; we've just done another Coverity run, and it's pulled up some
> issues in the recently changed Xen code. Rather than track them
> back to exactly which patches in the recent refactorings resulted
> in them, I figured I'd just list them here. Could you take a
> look at them, please ?
>
> (1) CID 1398635: xen_block_complete_aio(): identical code in two branches
>
> In hw/block/dataplane/xen_block_complete_aio():
>
> the first switch has this code:
> case BLKIF_OP_WRITE:
> case BLKIF_OP_FLUSH_DISKCACHE:
> if (!request->req.nr_segments) {
> break;
> }
I think this situation arose in the old xen_disk.c when grant map/unmap was
removed. There used to be cleanup to do on the write path (i.e. when
nr_segments != 0) but that went away with the move the copy-only. I'll get rid
of the if().
> break;
>
> so the if() doesn't do anything. What was this supposed to be?
>
> (2) Not spotted by coverity, but in a later switch in the same function:
>
> switch (request->req.operation) {
> case BLKIF_OP_WRITE:
> case BLKIF_OP_FLUSH_DISKCACHE:
> if (!request->req.nr_segments) {
> break;
> }
> case BLKIF_OP_READ:
>
> If a switch case is supposed to fall through it should be
> explicitly marked with a "/* fall through */" comment.
Yes, this was an oversight copied from the old code too.
>
> (3) CID 1398638: unused value in xen_block_set_vdev():
>
> In hw/block/xen-block.c xen_block_set_vdev():
>
> if (vdev->type == XEN_BLOCK_VDEV_TYPE_DP) {
> if (qemu_strtoul(p, , 10, >disk)) {
> goto invalid;
> }
>
> if (*end == 'p') {
> p = (char *) ++end;/* this assignment is unused */
> if (*end == '\0') {
> goto invalid;
> }
> }
> } else {
> vdev->disk = vbd_name_to_disk(p, );
> }
>
> if (*end != '\0') {
> p = (char *)end;
> [...]
>
> The assignment to p which I've marked with a comment above
> is never used, because we will either goto 'invalid' (which never
> uses 'p') or we will take the "if (*end != '\0')" path which
> overwrites 'p'. What is the intention here ?
It's sufficient to simply increment end before testing against '\0' so I'll
fold the pre-increment into the if().
>
> (4) CID 1398640: vbd_name_to_disk() integer overflow:
>
> In hw/block/xen-block.c vbd_name_to_disk(), if the name string
> passed in is empty or doesn't start with a lowercase alphabetic
> character, then we end the while loop with disk == 0. Then
> we return "disk - 1" which underflows to UINT_MAX. This isn't
> documented as being an error return for the function and the
> caller doesn't check for it.
Ok, I'll re-work the function to check for such an error condition.
>
> (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.
I'll prepare a patch with fixes for all these and send it shortly.
Paul
>
> thanks
> -- PMM
Re: [Qemu-devel] QEMU xen coverity issues
> -Original Message- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: 14 February 2019 18:29 > To: QEMU Developers > Cc: Paul Durrant ; Anthony Perard > > Subject: QEMU xen coverity issues > > Hi; we've just done another Coverity run, and it's pulled up some > issues in the recently changed Xen code. Rather than track them > back to exactly which patches in the recent refactorings resulted > in them, I figured I'd just list them here. Could you take a > look at them, please ? Yes. Thanks Peter, I'll take a look at them and get back to you. Paul
[Qemu-devel] QEMU xen coverity issues
Hi; we've just done another Coverity run, and it's pulled up some
issues in the recently changed Xen code. Rather than track them
back to exactly which patches in the recent refactorings resulted
in them, I figured I'd just list them here. Could you take a
look at them, please ?
(1) CID 1398635: xen_block_complete_aio(): identical code in two branches
In hw/block/dataplane/xen_block_complete_aio():
the first switch has this code:
case BLKIF_OP_WRITE:
case BLKIF_OP_FLUSH_DISKCACHE:
if (!request->req.nr_segments) {
break;
}
break;
so the if() doesn't do anything. What was this supposed to be?
(2) Not spotted by coverity, but in a later switch in the same function:
switch (request->req.operation) {
case BLKIF_OP_WRITE:
case BLKIF_OP_FLUSH_DISKCACHE:
if (!request->req.nr_segments) {
break;
}
case BLKIF_OP_READ:
If a switch case is supposed to fall through it should be
explicitly marked with a "/* fall through */" comment.
(3) CID 1398638: unused value in xen_block_set_vdev():
In hw/block/xen-block.c xen_block_set_vdev():
if (vdev->type == XEN_BLOCK_VDEV_TYPE_DP) {
if (qemu_strtoul(p, , 10, >disk)) {
goto invalid;
}
if (*end == 'p') {
p = (char *) ++end;/* this assignment is unused */
if (*end == '\0') {
goto invalid;
}
}
} else {
vdev->disk = vbd_name_to_disk(p, );
}
if (*end != '\0') {
p = (char *)end;
[...]
The assignment to p which I've marked with a comment above
is never used, because we will either goto 'invalid' (which never
uses 'p') or we will take the "if (*end != '\0')" path which
overwrites 'p'. What is the intention here ?
(4) CID 1398640: vbd_name_to_disk() integer overflow:
In hw/block/xen-block.c vbd_name_to_disk(), if the name string
passed in is empty or doesn't start with a lowercase alphabetic
character, then we end the while loop with disk == 0. Then
we return "disk - 1" which underflows to UINT_MAX. This isn't
documented as being an error return for the function and the
caller doesn't check for it.
(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.
thanks
-- PMM
