qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] QEMU xen coverity issues
@ 2019-02-14 18:29 Peter Maydell
  2019-02-15  9:21 ` Paul Durrant
  2019-02-15 15:36 ` Paul Durrant
  0 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2019-02-14 18:29 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Paul Durrant, Anthony PERARD

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, &end, 10, &vdev->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, &end);
    }

    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

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-19 16:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-14 18:29 [Qemu-devel] QEMU xen coverity issues Peter Maydell
2019-02-15  9:21 ` Paul Durrant
2019-02-15 15:36 ` Paul Durrant
2019-02-15 16:20   ` Paul Durrant
2019-02-18 10:09     ` Kevin Wolf
2019-02-18 10:28       ` Paul Durrant
2019-02-18 10:58         ` Kevin Wolf
2019-02-19 16:17           ` Paul Durrant
2019-02-19 16:34             ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).