Re: [PULL 10/14] net/virtio: add failover support

From: Peter Maydell <peter.maydell@linaro.org>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>,
	Jens Freimann <jfreimann@redhat.com>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PULL 10/14] net/virtio: add failover support
Date: Tue, 12 Nov 2019 10:08:55 +0000	[thread overview]
Message-ID: <CAFEAcA_LObSGtSBCd==L-qp4OyH8LgrpAPdBwzOkifk-kx8JSA@mail.gmail.com> (raw)
In-Reply-To: <20191029225932.14585-11-mst@redhat.com>

On Tue, 29 Oct 2019 at 23:01, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jens Freimann <jfreimann@redhat.com>
>
> This patch adds support to handle failover device pairs of a virtio-net
> device and a (vfio-)pci device, where the virtio-net acts as the standby
> device and the (vfio-)pci device as the primary.

Hi; Coverity reports some dereference-before-NULL-check errors
in this commit:

> +static bool failover_replug_primary(VirtIONet *n, Error **errp)
> +{
> +    HotplugHandler *hotplug_ctrl;
> +    PCIDevice *pdev = PCI_DEVICE(n->primary_dev);
> +
> +    if (!pdev->partially_hotplugged) {
> +        return true;
> +    }
> +    if (!n->primary_device_opts) {
> +        n->primary_device_opts = qemu_opts_from_qdict(
> +                qemu_find_opts("device"),
> +                n->primary_device_dict, errp);
> +    }
> +    if (n->primary_device_opts) {
> +        if (n->primary_dev) {

Here we check whether n->primary_dev is NULL...

> +            n->primary_bus = n->primary_dev->parent_bus;
> +        }
> +        qdev_set_parent_bus(n->primary_dev, n->primary_bus);

...but qdev_set_parent_bus unconditionally dereferences
its first argument, so it can't be NULL...

> +        n->primary_should_be_hidden = false;
> +        qemu_opt_set_bool(n->primary_device_opts,
> +                "partially_hotplugged", true, errp);
> +        hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
> +        if (hotplug_ctrl) {
> +            hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
> +            hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
> +        }
> +        if (!n->primary_dev) {

...and we do another NULL check here.

Either we don't need the NULL checks, or we need to avoid
calling qdev_set_parent_bus(NULL, ...).

(This is CID 1407224.)

> +            error_setg(errp, "virtio_net: couldn't find primary device");
> +        }
> +    }
> +    return *errp != NULL;
> +}

> +static int virtio_net_primary_should_be_hidden(DeviceListener *listener,
> +            QemuOpts *device_opts)
> +{
> +    VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
> +    bool match_found;
> +    bool hide;
> +
> +    n->primary_device_dict = qemu_opts_to_qdict(device_opts,
> +            n->primary_device_dict);

Here we pass device_optns to qemu_opts_to_qdict(), which must
take a non-NULL pointer (it always dereferences it)...

> +    if (n->primary_device_dict) {
> +        g_free(n->standby_id);
> +        n->standby_id = g_strdup(qdict_get_try_str(n->primary_device_dict,
> +                    "failover_pair_id"));
> +    }
> +    if (device_opts && g_strcmp0(n->standby_id, n->netclient_name) == 0) {

...but here we check whether device_opts is NULL.

Again, either the check or the call must be wrong.

(This is CID 1407222.)

> +        match_found = true;
> +    } else {
> +        match_found = false;
> +        hide = false;
> +        g_free(n->standby_id);
> +        n->primary_device_dict = NULL;
> +        goto out;
> +    }

thanks
-- PMM