Re: [Qemu-devel] [PULL 1/1] block/nvme: fix Coverity reports

[Peter Maydell]

On 1 March 2018 at 07:54, Fam Zheng <address@hidden> wrote:
> From: Paolo Bonzini <address@hidden>
>
> 1) string not null terminated in sysfs_find_group_file
>
> 2) NULL pointer dereference and dead local variable in nvme_init.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Fam Zheng <address@hidden>
>
> Message-Id: <address@hidden>
> Signed-off-by: Fam Zheng <address@hidden>

Hi. It looks like Coverity still doesn't like the error-exit
handling in nvme_init() (CID 1385847):

> ---
>  block/nvme.c        | 10 +++++++---
>  util/vfio-helpers.c |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 10bffbbf2f..75078022f6 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -645,7 +645,7 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>      aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
>                             false, nvme_handle_event, nvme_poll_cb);
>
> -    nvme_identify(bs, namespace, errp);
> +    nvme_identify(bs, namespace, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          ret = -EIO;
> @@ -666,8 +666,12 @@ fail_queue:
>      nvme_free_queue_pair(bs, s->queues[0]);
>  fail:
>      g_free(s->queues);
> -    qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
> -    qemu_vfio_close(s->vfio);
> +    if (s->regs) {
> +        qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, 
> NVME_BAR_SIZE);

We can get here with s->vfio being NULL and s->regs being
uninitialized, but qemu_vfio_pci_unmap_bar() will unconditionally
dereference s->vfio.

If you want to write the error path like this I think you need
to initialize s->regs = NULL before we do the qemu_vfio_open_pci()
call and error-check.

> +    }
> +    if (s->vfio) {
> +        qemu_vfio_close(s->vfio);
> +    }
>      event_notifier_cleanup(&s->irq_notifier);
>      return ret;
>  }

thanks
-- PMM