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

[Peter Maydell]

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.

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

thanks
-- PMM