| Message ID | 1592486508-6135-30-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,V2,01/33] virtio-net: implement RSS configuration command | expand |
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
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
On Fri, 3 Jul 2020 at 17:10, Lukas Straub <lukasstraub2@web.de> wrote: > > 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): > > 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. Did you send a fix for this? I couldn't find one in a quick search of my mail archive... thanks -- PMM
diff --git a/net/colo-compare.c b/net/colo-compare.c index c30dbfb..ed1f3d0 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -1283,15 +1283,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) s->vnet_hdr); } - qemu_mutex_lock(&colo_compare_mutex); - if (!colo_compare_active) { - qemu_mutex_init(&event_mtx); - qemu_cond_init(&event_complete_cond); - colo_compare_active = true; - } - QTAILQ_INSERT_TAIL(&net_compares, s, next); - qemu_mutex_unlock(&colo_compare_mutex); - s->out_sendco.s = s; s->out_sendco.chr = &s->chr_out; s->out_sendco.notify_remote_frame = false; @@ -1314,6 +1305,16 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) connection_destroy); colo_compare_iothread(s); + + qemu_mutex_lock(&colo_compare_mutex); + if (!colo_compare_active) { + qemu_mutex_init(&event_mtx); + qemu_cond_init(&event_complete_cond); + colo_compare_active = true; + } + QTAILQ_INSERT_TAIL(&net_compares, s, next); + qemu_mutex_unlock(&colo_compare_mutex); + return; } @@ -1382,19 +1383,6 @@ static void colo_compare_finalize(Object *obj) CompareState *s = COLO_COMPARE(obj); CompareState *tmp = NULL; - 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) { - colo_compare_timer_del(s); - } - - qemu_bh_delete(s->event_bh); - qemu_mutex_lock(&colo_compare_mutex); QTAILQ_FOREACH(tmp, &net_compares, next) { if (tmp == s) { @@ -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) { + colo_compare_timer_del(s); + } + + qemu_bh_delete(s->event_bh); + AioContext *ctx = iothread_get_aio_context(s->iothread); aio_context_acquire(ctx); AIO_WAIT_WHILE(ctx, !s->out_sendco.done);