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

[Peter Maydell]

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.

thanks
-- PMM