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
