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

[Peter Maydell]

On Wed, 3 Jul 2019 at 02:42, Zhang Chen <address@hidden> wrote:
>
> From: Zhang Chen <address@hidden>
>
> Address Peter's comments in patch "COLO-compare:Add colo-compare
> remote notify support".
>
> Signed-off-by: Zhang Chen <address@hidden>
> ---
>  net/colo-compare.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 909dd6c6eb..363b1edd11 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1008,21 +1008,20 @@ 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 (!strcmp((char *)notify_rs->buf, "COLO_USERSPACE_PROXY_INIT")) {

This is (still) assuming that the buffer you're passed in has a NUL-terminated
string: if not, it could run off the end of it. What you want to check is:
(1) is the packet_len long enough for the string we're looking for
(including the terminating NUL) and
(2) if so, does a simple "compare these N bytes" check match?

Something like

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);
}

might be a useful utility function. (notice that we are including the NUL
byte in our comparison check).

In general this code doesn't seem to have been written with an eye
to the packet contents being possibly-malicious. For instance
colo_compare_packet_payload() doesn't seem to check that the packets
actually are both long enough for the length being compared. This
could perhaps do with some review/audit by somebody.

thanks
-- PMM