[ovs-dev] [PATCH] ofp-util: Fix parsing of parenthesized values within key-value pairs.
Andy Zhou
azhou at ovn.org
Mon Jun 13 19:47:02 UTC 2016
On Sun, Jun 12, 2016 at 5:43 PM, Ben Pfaff <blp at ovn.org> wrote:
> Reported-by: james hopper <jameshopper at email.com>
> Reported-at:
> http://openvswitch.org/pipermail/discuss/2016-June/021662.html
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> AUTHORS | 1 +
> lib/ofp-util.c | 109
> ++++++++++++++++++++++++++++++--------------------
> tests/ofp-util.at | 43 ++++++++++++++++++++
> tests/ofproto.at | 4 ++
> utilities/ovs-ofctl.c | 21 ++++++++++
> 5 files changed, 134 insertions(+), 44 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 9fda4c1..e2ac267 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -452,6 +452,7 @@ Ziyou Wang ziyouw at vmware.com
> Zoltán Balogh zoltan.balogh at ericsson.com
> ankur dwivedi ankurengg2003 at gmail.com
> chen zhang 3zhangchen9211 at gmail.com
> +james hopper jameshopper at email.com
> kk yap yapkke at stanford.edu
> likunyun kunyunli at hotmail.com
> meishengxin meishengxin at huawei.com
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 2c6fb1f..2c93dd4 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -7355,6 +7355,36 @@ 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;
> + for (;;) {
>
Would it be easier to read with a while loop? Of course, there is nothing
wrong
using the for loop logically.
e.g.
size_t n = 0;
While (!strchr(delimiters, s[n]) {
if (s[n] == '(')
...
}
return n;
> + if (strchr(delimiters, s[n])) {
> + /* strchr(s, '\0') returns s+strlen(s), so this case handles
> the
> + * null terminator at the end of 's'. */
> + return n;
> + } else if (s[n] == '(') {
> + int level = 0;
> + do {
> + switch (s[n]) {
> + case '\0':
> + return n;
> + case '(':
> + level++;
> + break;
> + case ')':
> + level--;
> + break;
>
Is it possible for n to advance beyond end of string, in case of imbalanced
")" ?
I have not played with the test cases, so this is not a full review.
> + }
> + n++;
> + } while (level > 0);
> + } else {
> + 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 +7398,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;
> diff --git a/tests/ofp-util.at b/tests/ofp-util.at
> index 248faf4..700173a 100644
> --- a/tests/ofp-util.at
> +++ b/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
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 1ddee43..c89c641 100644
> --- a/tests/ofproto.at
> +++ b/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])
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 207588b..3d9cb73 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3955,6 +3955,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 },
> @@ -4075,6 +4095,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 },
> };
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list