[ovs-dev] [PATCH v3] ovn-controller: Fix parsing of OVN tunnel IDs

[Dumitru Ceara]

On Fri, Jun 7, 2019 at 6:42 PM Ben Pfaff <blp at ovn.org> wrote:
>
> On Mon, May 27, 2019 at 10:55:52AM +0200, Dumitru Ceara wrote:
> > Encap tunnel-ids are of the form:
> > <chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
> > In physical_run we were checking if a tunnel-id corresponds
> > to the local chassis-id by searching if the chassis-id string
> > is included in the tunnel-id (strstr). This can break quite
> > easily, for example, if the local chassis-id is a substring
> > of a remote chassis-id. In that case we were wrongfully
> > skipping the tunnel creation.
> >
> > To fix that new tunnel-id creation and parsing functions are added in
> > encaps.[ch]. These functions are now used everywhere where applicable.
> >
> > Acked-by: Venu Iyer <iyervl at ymail.com>
> > Reported-at: https://bugzilla.redhat.com/1708131
> > Reported-by: Haidong Li <haili at redhat.com>
> > Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > ---
> >
> > v3: Rebase
> > v2: Update commit message
>
> This seems about right.
>
> Here's an incremental with some suggestions for encaps_tunnel_id_parse()
> and encaps_tunnel_id_match().  I'm pretty happy with the
> encaps_tunnel_id_parse() change; the encaps_tunnel_id_match() one might
> be going over the top.
>
> Let me know what you think.

Thanks for the suggestions.
I like the new encaps_tunnel_id_parse() as it's more succinct and even
though encaps_tunnel_id_match() might be going a bit over the top it's
the most efficient way to do it indeed.

Thanks,
Dumitru

>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index 61b3eab3971f..3b3921a736e2 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -102,64 +102,51 @@ encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip)
>   * If the 'encap_ip' argument is not NULL the function will allocate memory
>   * and store the encapsulation IP part of the tunnel-id at '*encap_ip'.
>   */
> -bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> -                            char **encap_ip)
> +bool
> +encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id,
> +                       char **encap_ip)
>  {
> -    const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> -
> -    if (!match) {
> +    /* Find the delimiter.  Fail if there is no delimiter or if <chassis_name>
> +     * or <IP> is the empty string.*/
> +    const char *d = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
> +    if (d == tunnel_id || !d || !d[1]) {
>          return false;
>      }
>
>      if (chassis_id) {
> -        size_t chassis_id_len = (match - tunnel_id);
> -
> -        *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
> +        *chassis_id = xmemdup0(tunnel_id, d - tunnel_id);
>      }
> -
> -    /* Consume the tunnel-id delimiter. */
> -    match++;
> -
>      if (encap_ip) {
> -        /*
> -         * If the value has morphed into something other than
> -         * <chassis-id><delim><encap-ip>, fail and free already allocated
> -         * memory (i.e., chassis_id).
> -         */
> -        if (*match == 0) {
> -            if (chassis_id) {
> -                free(*chassis_id);
> -            }
> -            return false;
> -        }
> -        *encap_ip = xstrdup(match);
> +        *encap_ip = xstrdup(d + 1);
>      }
> -
>      return true;
>  }
>
>  /*
> - * Returns true if a given tunnel_id contains 'chassis_id' and, if specified,
> - * the given 'encap_ip'. Returns false otherwise.
> + * Returns true if 'tunnel_id' contains 'chassis_id' and, if specified, the
> + * given 'encap_ip'. Returns false otherwise.
>   */
> -bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> -                            const char *encap_ip)
> +bool
> +encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id,
> +                       const char *encap_ip)
>  {
> -    if (strstr(tunnel_id, chassis_id) != tunnel_id) {
> -        return false;
> -    }
> -
> -    size_t chassis_id_len = strlen(chassis_id);
> -
> -    if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
> -        return false;
> -    }
> -
> -    if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
> -        return false;
> +    while (*tunnel_id == *chassis_id) {
> +        if (!*tunnel_id) {
> +            /* 'tunnel_id' and 'chassis_id' are equal strings.  This is a
> +             * mismatch because 'tunnel_id' is missing the delimiter and IP. */
> +            return false;
> +        }
> +        tunnel_id++;
> +        chassis_id++;
>      }
>
> -    return true;
> +    /* We found the first byte that disagrees between 'tunnel_id' and
> +     * 'chassis_id'.  If we consumed all of 'chassis_id' and arrived at the
> +     * delimiter in 'tunnel_id' (and if 'encap_ip' is correct, if it was
> +     * supplied), it's a match. */
> +    return (*tunnel_id == OVN_MVTEP_CHASSISID_DELIM
> +            && *chassis_id == '\0'
> +            && (!encap_ip || !strcmp(tunnel_id + 1, encap_ip)));
>  }
>
>  static void