Re: [PULL V2 29/33] net/colo-compare.c: Correct ordering in complete and finalize

[Lukas Straub]

On Thu, 25 Jun 2020 10:30:24 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 18 Jun 2020 at 14:23, Jason Wang <jasowang@redhat.com> wrote:
> >
> > From: Lukas Straub <lukasstraub2@web.de>
> >
> > In colo_compare_complete, insert CompareState into net_compares
> > only after everything has been initialized.
> > In colo_compare_finalize, remove CompareState from net_compares
> > before anything is deinitialized.  
> 
> Hi; this code-motion seems to have prompted Coverity to
> discover a possible deref-of-NULL-pointer (cID 1429969):
> 
> 
> > @@ -1409,6 +1397,19 @@ static void colo_compare_finalize(Object *obj)
> >      }
> >      qemu_mutex_unlock(&colo_compare_mutex);
> >
> > +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > +    qemu_chr_fe_deinit(&s->chr_out, false);
> > +    if (s->notify_dev) {
> > +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > +    }
> > +
> > +    if (s->iothread) {  
> 
> Here we check s->iothread, which implies that it could be NULL...
> 
> > +        colo_compare_timer_del(s);
> > +    }
> > +
> > +    qemu_bh_delete(s->event_bh);
> > +
> >      AioContext *ctx = iothread_get_aio_context(s->iothread);  
> 
> ...but here we pass it to iothread_get_aio_context(), which
> unconditionally dereferences it, so will crash if it is NULL.
> 
> Either we need to avoid calling this if s->iothread is NULL,
> or if it can't ever be NULL then the earlier NULL check was
> pointless and can be removed.

I'll look into it.

Regards,
Lukas Straub

> 
> >      aio_context_acquire(ctx);
> >      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > --
> > 2.5.0  
> 
> thanks
> -- PMM

pgpZbLkcMTIOo.pgp
Description: OpenPGP digital signature