Re: [PATCH v2 2/2] virtio: use Error for queue size validation in virtio_add_queue()

Am 30.07.2026 um 22:58 hat Stefan Hajnoczi geschrieben:
> Coverity is unhappy with the code path where the x-override-queue-size
> property value is passed to g_new0() since it is a signed int rather
> than an unsigned int:
> 
>       *** CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
>       /builds/qemu-project/qemu/hw/virtio/virtio.c: 2595             in virtio_add_queue()
>       2589         }
>       2590
>       2591         vdev->vq[i].vring.num = queue_size;
>       2592         vdev->vq[i].vring.num_default = queue_size;
>       2593         vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
>       2594         vdev->vq[i].handle_output = handle_output;
>       >>>     CID 1664271:         Error handling issues  (NEGATIVE_RETURNS)
>       >>>     "__n" is passed to a parameter that cannot be negative.
>       2595         vdev->vq[i].used_elems = g_new0(VirtQueueElement, queue_size);
> 
> Introduce an Error **errp argument to virtio_add_queue() and set it when
> the queue_size argument or the x-override-queue-size property value are
> invalid.
> 
> At the moment none of the callers propagate the Error object. Instead
> they are all modified to pass &error_abort so that the error message is
> printed and the program terminates (it also terminated before). Further
> work, especially in new device emulation code, could actually propagate
> the Error object but is left for the future.
> 
> Signed-off-by: Stefan Hajnoczi <[email protected]>

What's the point of adding an Error object when not even a single caller
makes use of it?

At least the semantic change in virtio_add_queue() (checking if override
is valid) should be a separate patch from the refactoring that pushes
the existing &error_abort into all callers.

> @@ -2574,15 +2575,43 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, unsigned int queue_size,
>              break;
>      }
>  
> -    if (i == VIRTIO_QUEUE_MAX || queue_size > VIRTQUEUE_MAX_SIZE)
> -        abort();
> +    if (i == VIRTIO_QUEUE_MAX) {
> +        error_setg(errp, "Exceeded maximum number of virtqueues (%d)", i);
> +        return NULL;
> +    }
> +
> +    if (queue_size > VIRTQUEUE_MAX_SIZE) {
> +        error_setg(errp, "Virtqueue size %u exceeds the max (%u)",
> +                   queue_size, VIRTQUEUE_MAX_SIZE);
> +        return NULL;
> +    }
>  
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      if (qbus && qbus->parent &&
>          object_property_find(OBJECT(qbus->parent), VIRTIO_QUEUE_SIZE_OVERRIDE)) {
>          int override = object_property_get_int(OBJECT(qbus->parent),
>                                                 VIRTIO_QUEUE_SIZE_OVERRIDE,
> -                                               &error_abort);
> +                                               errp);
> +
> +        if (*errp) {
> +            return NULL;
> +        }
> +        if (override < 0) {
> +            /*
> +             * The property type should be UINT16, so this can't happen, but
> +             * help out Coverity.
> +             */
> +            error_setg(errp, "%s (%d) cannot be negative",
> +                       VIRTIO_QUEUE_SIZE_OVERRIDE, override);
> +            return NULL;
> +        }

Wouldn't it be more proper to use object_property_get_uint() instead of
getting an unsigned property as signed and then checking for negative
values?

> +        if (override > VIRTQUEUE_MAX_SIZE) {
> +            error_setg(errp, "%s (%d) exceeds the max (%d)",
> +                       VIRTIO_QUEUE_SIZE_OVERRIDE, override,
> +                       VIRTQUEUE_MAX_SIZE);
> +            return NULL;
> +        }
> +
>          if (override) {
>              queue_size = override;
>          }

Kevin