Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory leak and code style issue.

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang [mailto:address@hidden]
> Sent: Thursday, July 11, 2019 4:18 AM
> To: Zhang, Chen <address@hidden>; Li Zhijian <address@hidden>;
> Peter Maydell <address@hidden>; qemu-dev <qemu-
> address@hidden>
> Cc: Zhang Chen <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory leak
> and code style issue.
> 
> 
> On 2019/7/10 下午3:50, Zhang, Chen wrote:
> >
> >> -----Original Message-----
> >> From: Jason Wang [mailto:address@hidden]
> >> Sent: Tuesday, July 9, 2019 10:48 PM
> >> To: Zhang, Chen <address@hidden>; Li Zhijian
> >> <address@hidden>; Peter Maydell <address@hidden>;
> >> qemu-dev <qemu- address@hidden>
> >> Cc: Zhang Chen <address@hidden>
> >> Subject: Re: [Qemu-devel] [PATCH V2] net/colo-compare.c: Fix memory
> >> leak and code style issue.
> >>
> >>
> >> On 2019/7/4 下午4:36, Zhang Chen wrote:
> >>> From: Zhang Chen <address@hidden>
> >>>
> >>> This patch to fix the origin "char *data" menory leak, code style
> >>> issue and add necessary check here.
> >>> Reported-by: Coverity (CID 1402785)
> >>>
> >>> Signed-off-by: Zhang Chen <address@hidden>
> >>> ---
> >>>    net/colo-compare.c | 28 +++++++++++++++++++++-------
> >>>    1 file changed, 21 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> >>> 909dd6c6eb..ed349f5f6a 100644
> >>> --- a/net/colo-compare.c
> >>> +++ b/net/colo-compare.c
> >>> @@ -127,6 +127,17 @@ static int compare_chr_send(CompareState *s,
> >>>                                uint32_t vnet_hdr_len,
> >>>                                bool notify_remote_frame);
> >>>
> >>> +static bool packet_matches_str(const char *str,
> >>> +                               uint8_t *buf,
> >>> +                               uint32_t packet_len) {
> >>> +    if (packet_len <= strlen(str)) {
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    return !memcmp(str, buf, strlen(str) + 1);
> >>
> >> This assumes buf is NULL terminated (you pass notify_rs->buf) which
> >> is not correct I think?
> > Yes, you are right.
> > How about this:
> >
> > static bool packet_matches_str(const char *str,
> >                                 uint8_t *buf,
> >                                 uint32_t packet_len) {
> >      if (packet_len != strlen(str) || !buf) {
> >          return false;
> >      }
> 
> 
> When can we hit !buf?

I re-checked the code, looks the "net_fill_rstate()" can ensure buf is not 
empty,
I will remove the !buf to send next version.

Thanks
Zhang Chen

> 
> Thanks
> 
> 
> >
> >      return !memcmp(str, buf, strlen(str)); }
> >
> > Thanks
> > Zhang Chen
> >
> >
> >> Thanks
> >>
> >>
> >>> +}
> >>> +
> >>>    static void notify_remote_frame(CompareState *s)
> >>>    {
> >>>        char msg[] = "DO_CHECKPOINT"; @@ -1008,21 +1019,24 @@ 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";
> >>> +    const char msg[] = "COLO_COMPARE_GET_XEN_INIT";
> >>>        int ret;
> >>>
> >>> -    if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) {
> >>> +    if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
> >>> +                           notify_rs->buf,
> >>> +                           notify_rs->packet_len)) {
> >>>            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")) {
> >>> +    } else if (packet_matches_str("COLO_CHECKPOINT",
> >>> +                                  notify_rs->buf,
> >>> +                                  notify_rs->packet_len)) {
> >>>            /* colo-compare do checkpoint, flush pri packet and
> >>> remove sec packet
> >> */
> >>>            g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> >>> +    } else {
> >>> +        error_report("COLO compare got unsupported instruction '%s'",
> >>> +                     (char *)notify_rs->buf);
> >>>        }
> >>>    }
> >>>