On Fri, 05/18 19:01, Peter Maydell wrote:
> On 1 March 2018 at 07:54, Fam Zheng <f...@redhat.com> wrote:
> > From: Paolo Bonzini <pbonz...@redhat.com>
> >
> > 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 <pbonz...@redhat.com>
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> >
> > Message-Id: <20180213015240.9352-1-f...@redhat.com>
> > Signed-off-by: Fam Zheng <f...@redhat.com>
>
> 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.
Block layer uses g_malloc0 to allocate *s, so we expect s->regs is NULL to begin
with. But I can send a patch to explicitly initialize it to NULL to make the
code simpler to reason.
Fam
>
> > + }
> > + if (s->vfio) {
> > + qemu_vfio_close(s->vfio);
> > + }
> > event_notifier_cleanup(&s->irq_notifier);
> > return ret;
> > }
>
> thanks
> -- PMM
