ofp_actions: Fix set_mpls_tc formatting.

Apart from a cut-and-paste typo, the man page claims that mpls_labels
can be provided in hexadecimal format but that's currently not the case.

Fix mpls ofp-action formatting, add size checks on ofp-action parsing
and add some unit tests.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

3 files changed, 60 insertions, 4 deletions

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 0342a228b..6fb3da507 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3777,11 +3777,22 @@ parse_SET_MPLS_LABEL(char *arg, const struct ofpact_parse_params *pp)
 {
     struct ofpact_mpls_label *mpls_label
         = ofpact_put_SET_MPLS_LABEL(pp->ofpacts);
+    uint32_t label;
+    char *error;
+
     if (*arg == '\0') {
         return xstrdup("set_mpls_label: expected label.");
     }
 
-    mpls_label->label = htonl(atoi(arg));
+    error = str_to_u32(arg, &label);
+    if (error) {
+        return error;
+    }
+
+    if (label & ~0xfffff) {
+        return xasprintf("%s: not a valid MPLS label", arg);
+    }
+    mpls_label->label = htonl(label);
     return NULL;
 }
 
@@ -3837,12 +3848,22 @@ static char * OVS_WARN_UNUSED_RESULT
 parse_SET_MPLS_TC(char *arg, const struct ofpact_parse_params *pp)
 {
     struct ofpact_mpls_tc *mpls_tc = ofpact_put_SET_MPLS_TC(pp->ofpacts);
+    uint8_t tc;
+    char *error;
 
     if (*arg == '\0') {
         return xstrdup("set_mpls_tc: expected tc.");
     }
 
-    mpls_tc->tc = atoi(arg);
+    error = str_to_u8(arg, "MPLS TC", &tc);
+    if (error) {
+        return error;
+    }
+
+    if (tc & ~7) {
+        return xasprintf("%s: not a valid MPLS TC", arg);
+    }
+    mpls_tc->tc = tc;
     return NULL;
 }
 
@@ -3850,7 +3871,7 @@ static void
 format_SET_MPLS_TC(const struct ofpact_mpls_tc *a,
                    const struct ofpact_format_params *fp)
 {
-    ds_put_format(fp->s, "%sset_mpls_ttl(%s%"PRIu8"%s)%s",
+    ds_put_format(fp->s, "%sset_mpls_tc(%s%"PRIu8"%s)%s",
                   colors.paren, colors.end, a->tc,
                   colors.paren, colors.end);
 }
@@ -3889,12 +3910,18 @@ static char * OVS_WARN_UNUSED_RESULT
 parse_SET_MPLS_TTL(char *arg, const struct ofpact_parse_params *pp)
 {
     struct ofpact_mpls_ttl *mpls_ttl = ofpact_put_SET_MPLS_TTL(pp->ofpacts);
+    uint8_t ttl;
+    char *error;
 
     if (*arg == '\0') {
         return xstrdup("set_mpls_ttl: expected ttl.");
     }
 
-    mpls_ttl->ttl = atoi(arg);
+    error = str_to_u8(arg, "MPLS TTL", &ttl);
+    if (error) {
+        return error;
+    }
+    mpls_ttl->ttl = ttl;
     return NULL;
 }
 
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 199db8ed0..59093c03c 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -1007,12 +1007,21 @@ bad_action 'dec_ttl(,)' 'dec_ttl_cnt_ids: expected at least one controller id.'
 # set_mpls_label
 bad_action 'set_mpls_label' 'set_mpls_label: expected label.'
 
+# set_mpls_label oversized
+bad_action 'set_mpls_label(0x100000)' '0x100000: not a valid MPLS label'
+
 # set_mpls_tc
 bad_action 'set_mpls_tc' 'set_mpls_tc: expected tc.'
 
+# set_mpls_tc oversized
+bad_action 'set_mpls_tc(8)' '8: not a valid MPLS TC'
+
 # set_mpls_ttl
 bad_action 'set_mpls_ttl' 'set_mpls_ttl: expected ttl.'
 
+# set_mpls_ttl oversized
+bad_action 'set_mpls_ttl(256)' 'invalid MPLS TTL "256"'
+
 # fin_timeout
 bad_action 'fin_timeout(foo=bar)' "invalid key 'foo' in 'fin_timeout' argument"
 
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 5ddca67e7..604f15c2d 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -449,6 +449,16 @@ actions=output(max_len=100,port=123)
 actions=output(port=100,max_len=123)
 actions=output(port=LOCAL,max_len=123)
 actions=output(port=IN_PORT,max_len=123)
+mpls,mpls_label=1,actions=set_mpls_label(0)
+mpls,mpls_label=1,actions=set_mpls_label(10)
+mpls,mpls_label=1,actions=set_mpls_label(0x10)
+mpls,mpls_label=1,actions=set_mpls_label(0xfffff)
+mpls,mpls_tc=1,actions=set_mpls_tc(0)
+mpls,mpls_tc=1,actions=set_mpls_tc(3)
+mpls,mpls_tc=1,actions=set_mpls_tc(7)
+mpls,mpls_ttl=1,actions=set_mpls_ttl(0)
+mpls,mpls_ttl=1,actions=set_mpls_ttl(200)
+mpls,mpls_ttl=1,actions=set_mpls_ttl(255)
 ]])
 
 AT_CHECK([ovs-ofctl parse-flows flows.txt
@@ -506,6 +516,16 @@ NXT_FLOW_MOD: ADD table:255 actions=output(port=123,max_len=100)
 NXT_FLOW_MOD: ADD table:255 actions=output(port=100,max_len=123)
 NXT_FLOW_MOD: ADD table:255 actions=output(port=LOCAL,max_len=123)
 NXT_FLOW_MOD: ADD table:255 actions=output(port=IN_PORT,max_len=123)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_label=1 actions=set_mpls_label(0)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_label=1 actions=set_mpls_label(10)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_label=1 actions=set_mpls_label(16)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_label=1 actions=set_mpls_label(1048575)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_tc=1 actions=set_mpls_tc(0)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_tc=1 actions=set_mpls_tc(3)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_tc=1 actions=set_mpls_tc(7)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_ttl=1 actions=set_mpls_ttl(0)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_ttl=1 actions=set_mpls_ttl(200)
+NXT_FLOW_MOD: ADD table:255 mpls,mpls_ttl=1 actions=set_mpls_ttl(255)
 ]])
 AT_CLEANUP