> -----Original Message----- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Wednesday, July 3, 2019 2:51 AM > To: Jason Wang <jasow...@redhat.com> > Cc: QEMU Developers <qemu-devel@nongnu.org>; Zhang, Chen > <chen.zh...@intel.com> > Subject: Re: [PULL 16/17] COLO-compare: Add colo-compare remote notify > support > > On Tue, 2 Jul 2019 at 03:32, Jason Wang <jasow...@redhat.com> wrote: > > > > From: Zhang Chen <chen.zh...@intel.com> > > > > This patch make colo-compare can send message to remote COLO frame(Xen) > when occur checkpoint. > > > > Signed-off-by: Zhang Chen <chen.zh...@intel.com> > > Signed-off-by: Jason Wang <jasow...@redhat.com> > > 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