[ovs-dev] [PATCH v3 ovn] ovn-sbctl: Prevent core dump from ovn-sbctl lflow-list [datpath] 0xflow

[Mark Michelson]

Excellent! Looks good to me!

Acked-by: Mark Michelson <mmichels at redhat.com>

On 4/21/21 12:17 PM, Alexey Roytman wrote:
> From: Alexey Roytman <roytman at il.ibm.com>
> 
> When ovn-sbctl lflow-list gets lflow argument with 0x prefix, e.g. 0x8131c8a8,
> it prints correct output, but fails with coredump.
> For example:
> ovn-sbctl --uuid lflow-list sw1 0x8131c8a8
>                                                                    
> Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda)  Pipeline: egress
>      uuid=0x8131c8a8, table=10(ls_out_port_sec_l2 ), priority=100  ,
> match=(eth.mcast), action=(output;)
> free(): invalid pointer
> [2]    616553 abort (core dumped)  ovn-sbctl --uuid dump-flows sw1 0x8131c8a8
>   
>   This patch fixes it.
> 
> Signed-off-by: Alexey Roytman <roytman at il.ibm.com>
> ---
>   utilities/ovn-sbctl.c | 28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index e3aa7a68e..99c112358 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -764,23 +764,28 @@ sbctl_lflow_cmp(const void *a_, const void *b_)
>       return cmp ? cmp : strcmp(a->actions, b->actions);
>   }
>   
> -static char *
> +static bool
> +is_uuid_with_prefix(const char *uuid)
> +{
> +     return uuid[0] == '0' && (uuid[1] == 'x' || uuid[1] == 'X');
> +}
> +
> +static bool
>   parse_partial_uuid(char *s)
>   {
>       /* Accept a full or partial UUID. */
>       if (uuid_is_partial_string(s)) {
> -        return s;
> +        return true;
>       }
>   
>       /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl
>        * dump-flows" prints cookies prefixed by 0x. */
> -    if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X')
> -        && uuid_is_partial_string(s + 2)) {
> -        return s + 2;
> +    if (is_uuid_with_prefix(s) && uuid_is_partial_string(s + 2)) {
> +        return true;
>       }
>   
>       /* Not a (partial) UUID. */
> -    return NULL;
> +    return false;
>   }
>   
>   static const char *
> @@ -799,8 +804,11 @@ is_partial_uuid_match(const struct uuid *uuid, const char *match)
>        * from UUIDs, and cookie values are printed without leading zeros because
>        * they're just numbers. */
>       const char *s1 = strip_leading_zero(uuid_s);
> -    const char *s2 = strip_leading_zero(match);
> -
> +    const char *s2 = match;
> +    if (is_uuid_with_prefix(s2)) {
> +        s2 = s2 + 2;
> +    }
> +    s2 = strip_leading_zero(s2);
>       return !strncmp(s1, s2, strlen(s2));
>   }
>   
> @@ -1134,12 +1142,10 @@ cmd_lflow_list(struct ctl_context *ctx)
>       }
>   
>       for (size_t i = 1; i < ctx->argc; i++) {
> -        char *s = parse_partial_uuid(ctx->argv[i]);
> -        if (!s) {
> +        if (!parse_partial_uuid(ctx->argv[i])) {
>               ctl_fatal("%s is not a UUID or the beginning of a UUID",
>                         ctx->argv[i]);
>           }
> -        ctx->argv[i] = s;
>       }
>   
>       struct vconn *vconn = sbctl_open_vconn(&ctx->options);
>