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

Ben Pfaff blp at ovn.org
Fri Jun 7 16:41:48 UTC 2019


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.

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


More information about the dev mailing list