Re: [Qemu-devel] [PULL 16/17] COLO-compare: Add colo-compare remote notify support

[Zhang, Chen]

> -----Original Message-----
> From: Peter Maydell [mailto:address@hidden]
> Sent: Wednesday, July 3, 2019 2:51 AM
> To: Jason Wang <address@hidden>
> Cc: QEMU Developers <address@hidden>; Zhang, Chen
> <address@hidden>
> Subject: Re: [PULL 16/17] COLO-compare: Add colo-compare remote notify
> support
> 
> On Tue, 2 Jul 2019 at 03:32, Jason Wang <address@hidden> wrote:
> >
> > From: Zhang Chen <address@hidden>
> >
> > This patch make colo-compare can send message to remote COLO frame(Xen)
> when occur checkpoint.
> >
> > Signed-off-by: Zhang Chen <address@hidden>
> > Signed-off-by: Jason Wang <address@hidden>
> 
> Hi; Coverity reports a problem (CID 1402785) with this function:
> 
> > @@ -989,7 +1006,24 @@ static void
> > compare_sec_rs_finalize(SocketReadState *sec_rs)
> >
> >  static void compare_notify_rs_finalize(SocketReadState *notify_rs)  {
> > +    CompareState *s = container_of(notify_rs, CompareState,
> > + notify_rs);
> > +
> >      /* Get Xen colo-frame's notify and handle the message */
> > +    char *data = g_memdup(notify_rs->buf, notify_rs->packet_len);
> > +    char msg[] = "COLO_COMPARE_GET_XEN_INIT";
> > +    int ret;
> > +
> > +    if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
> > +        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > +        if (ret < 0) {
> > +            error_report("Notify Xen COLO-frame INIT failed");
> > +        }
> > +    }
> > +
> > +    if (!strcmp(data, "COLO_CHECKPOINT")) {
> > +        /* colo-compare do checkpoint, flush pri packet and remove sec 
> > packet
> */
> > +        g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > +    }
> >  }
> 
> We allocate data using g_memdup(), but we never free it before returning, so
> the function has a memory leak. It's not clear to me why we're duplicating the
> string at all -- it would be cleaner to do the check of the packet contents to
> identify in-place. That would be the best way to fix/avoid the leak.
> 
> Some other things I notice reading the function:
> 
> (1) after the first if() we will go ahead and check the second if().
> This means we'll unnecessarily check whether the data string matches
> COLO_CHECKPOINT, when we know already it cannot. I think an if (!strcmp(...))
> { ... } else if (!strcmp(...)) { ... } structure would be more normal C here.
> 
> (2) the g_memdup() call is treating the data as a buffer-and-length, and we
> don't enforce that it is NUL-terminated. But then we do a
> strcmp() against it, which assumes that the data is a NUL-terminated string. 
> Is
> this safe ?
> 
> (3) More minor point: you could mark 'msg' as const here, since I think we
> never need to modify it.

OK, I will fix it in next version.
Thank you for your review.


Thanks
Zhang Chen

> 
> thanks
> -- PMM