ofp-util: Fix parsing of parenthesized values within key-value pairs.

Reported-by: james hopper <[email protected]>
Reported-at: http://openvswitch.org/pipermail/discuss/2016-June/021662.html
Signed-off-by: Ben Pfaff <[email protected]>

AUTHORS

@@ -452,6 +452,7 @@ Ziyou Wang              [email protected]
 Zoltán Balogh           [email protected]
 ankur dwivedi           [email protected]
 chen zhang              [email protected]
+james hopper            [email protected]
 kk yap                  [email protected]
 likunyun                [email protected]
 meishengxin             [email protected]

lib/ofp-util.c

@@ -7355,6 +7355,38 @@ ofputil_normalize_match_quiet(struct match *match)
     ofputil_normalize_match__(match, false);
 }
 
+static size_t
+parse_value(const char *s, const char *delimiters)
+{
+    size_t n = 0;
+
+    /* Iterate until we reach a delimiter.
+     *
+     * strchr(s, '\0') returns s+strlen(s), so this test handles the null
+     * terminator at the end of 's'.  */
+    while (!strchr(delimiters, s[n])) {
+        if (s[n] == '(') {
+            int level = 0;
+            do {
+                switch (s[n]) {
+                case '\0':
+                    return n;
+                case '(':
+                    level++;
+                    break;
+                case ')':
+                    level--;
+                    break;
+                }
+                n++;
+            } while (level > 0);
+        } else {
+            n++;
+        }
+    }
+    return n;
+}
+
 /* Parses a key or a key-value pair from '*stringp'.
  *
  * On success: Stores the key into '*keyp'.  Stores the value, if present, into
@@ -7368,58 +7400,49 @@ ofputil_normalize_match_quiet(struct match *match)
 bool
 ofputil_parse_key_value(char **stringp, char **keyp, char **valuep)
 {
-    char *pos, *key, *value;
-    size_t key_len;
-
-    pos = *stringp;
-    pos += strspn(pos, ", \t\r\n");
-    if (*pos == '\0') {
+    /* Skip white space and delimiters.  If that brings us to the end of the
+     * input string, we are done and there are no more key-value pairs. */
+    *stringp += strspn(*stringp, ", \t\r\n");
+    if (**stringp == '\0') {
         *keyp = *valuep = NULL;
         return false;
     }
 
-    key = pos;
-    key_len = strcspn(pos, ":=(, \t\r\n");
-    if (key[key_len] == ':' || key[key_len] == '=') {
-        /* The value can be separated by a colon. */
-        size_t value_len;
-
-        value = key + key_len + 1;
-        value_len = strcspn(value, ", \t\r\n");
-        pos = value + value_len + (value[value_len] != '\0');
-        value[value_len] = '\0';
-    } else if (key[key_len] == '(') {
-        /* The value can be surrounded by balanced parentheses.  The outermost
-         * set of parentheses is removed. */
-        int level = 1;
-        size_t value_len;
-
-        value = key + key_len + 1;
-        for (value_len = 0; level > 0; value_len++) {
-            switch (value[value_len]) {
-            case '\0':
-                level = 0;
-                break;
-
-            case '(':
-                level++;
-                break;
+    /* Extract the key and the delimiter that ends the key-value pair or begins
+     * the value.  Advance the input position past the key and delimiter. */
+    char *key = *stringp;
+    size_t key_len = strcspn(key, ":=(, \t\r\n");
+    char key_delim = key[key_len];
+    key[key_len] = '\0';
+    *stringp += key_len + (key_delim != '\0');
 
-            case ')':
-                level--;
-                break;
-            }
-        }
-        value[value_len - 1] = '\0';
-        pos = value + value_len;
+    /* Figure out what delimiter ends the value:
+     *
+     *     - If key_delim is ":" or "=", the value extends until white space
+     *       or a comma.
+     *
+     *     - If key_delim is "(", the value extends until ")".
+     *
+     * If there is no value, we are done. */
+    const char *value_delims;
+    if (key_delim == ':' || key_delim == '=') {
+        value_delims = ", \t\r\n";
+    } else if (key_delim == '(') {
+        value_delims = ")";
     } else {
-        /* There might be no value at all. */
-        value = key + key_len;  /* Will become the empty string below. */
-        pos = key + key_len + (key[key_len] != '\0');
+        *keyp = key;
+        *valuep = key + key_len; /* Empty string. */
+        return true;
     }
-    key[key_len] = '\0';
 
-    *stringp = pos;
+    /* Extract the value.  Advance the input position past the value and
+     * delimiter. */
+    char *value = *stringp;
+    size_t value_len = parse_value(value, value_delims);
+    char value_delim = value[value_len];
+    value[value_len] = '\0';
+    *stringp += value_len + (value_delim != '\0');
+
     *keyp = key;
     *valuep = value;
     return true;

tests/ofp-util.at

@@ -50,3 +50,46 @@ OFPT_HELLO (OF1.1) (xid=0x1):
  version bitmap: 0x02
 ])
 AT_CLEANUP
+
+AT_SETUP([parsing key-value pairs])
+dnl Key-only basics.
+AT_CHECK([ovs-ofctl parse-key-value a a,b 'a b' 'a	b' 'a
+b'], 0, [a
+a, b
+a, b
+a, b
+a, b
+])
+
+dnl Key-value basics.
+AT_CHECK([ovs-ofctl parse-key-value a:b a=b a:b,c=d 'a=b c' 'a(b)' 'a(b),c(d)'], 0,
+[a=b
+a=b
+a=b, c=d
+a=b, c
+a=b
+a=b, c=d
+])
+
+dnl Values that contain nested delimiters.
+AT_CHECK([ovs-ofctl parse-key-value 'a:(b,c)' 'a:b(c,d)e' 'a(b,c(d,e),f)'], 0,
+[a=(b,c)
+a=b(c,d)e
+a=b,c(d,e),f
+])
+
+dnl Extraneous delimiters.
+AT_CHECK([ovs-ofctl parse-key-value a,,b ',a  b' ' a b ,'], 0, [a, b
+a, b
+a, b
+])
+
+dnl Missing right parentheses.
+dnl
+dnl m4 can't handle unbalanced parentheses so we use @{:@, which
+dnl Autotest replaces by a left parenthesis.
+AT_CHECK([ovs-ofctl parse-key-value 'a@{:@b' 'a@{:@b(c)' 'a=b@{:@c'], 0, [a=b
+a=b(c)
+a=b@{:@c
+])
+AT_CLEANUP

tests/ofproto.at

@@ -783,6 +783,10 @@ OVS_VSWITCHD_START
 AT_DATA([groups.txt], [dnl
 group_id=1234,type=all,bucket=output:10
 group_id=1235,type=all,bucket=output:10
+
+dnl This checks for regression against a parser bug such that
+dnl "actions=resbmit(,1)" etc. was rejected as a syntax error.
+group_id=2345,type=select,bucket=weight:10,actions=resubmit(,1),bucket=weight:10,actions=resubmit(,2),bucket=weight:1,actions=resubmit(,3)
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt])
 AT_CHECK([ovs-vsctl del-br br0])

utilities/ovs-ofctl.c

@@ -3969,6 +3969,26 @@ ofctl_encode_hello(struct ovs_cmdl_context *ctx)
     ofpbuf_delete(hello);
 }
 
+static void
+ofctl_parse_key_value(struct ovs_cmdl_context *ctx)
+{
+    for (size_t i = 1; i < ctx->argc; i++) {
+        char *s = ctx->argv[i];
+        char *key, *value;
+        int j = 0;
+        while (ofputil_parse_key_value(&s, &key, &value)) {
+            if (j++) {
+                fputs(", ", stdout);
+            }
+            fputs(key, stdout);
+            if (value[0]) {
+                printf("=%s", value);
+            }
+        }
+        putchar('\n');
+    }
+}
+
 static const struct ovs_cmdl_command all_commands[] = {
     { "show", "switch",
       1, 1, ofctl_show },
@@ -4094,6 +4114,7 @@ static const struct ovs_cmdl_command all_commands[] = {
     { "encode-error-reply", NULL, 2, 2, ofctl_encode_error_reply },
     { "ofp-print", NULL, 1, 2, ofctl_ofp_print },
     { "encode-hello", NULL, 1, 1, ofctl_encode_hello },
+    { "parse-key-value", NULL, 1, INT_MAX, ofctl_parse_key_value },
 
     { NULL, NULL, 0, 0, NULL },
 };