From: Ben Pfaff <blp@nicira.com>
Date: Thu, 11 Feb 2010 20:26:45 +0000 (-0800)
Subject: vconn: When validating OpenFlow actions always check for bad length.
X-Git-Tag: v0.99.2~19
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=77512e6a11657a5353477a59325031f1c0609116;p=sliver-openvswitch.git

vconn: When validating OpenFlow actions always check for bad length.

This code was previously unreachable, but it makes sense to check the
action length before looking at the action any further.

This doesn't fix an actual bug--actions were always properly validated.

Also, rephrase a few of the switch cases to make it even more obvious
that they always return.

Reported-by: Jean Tourrilhes <jt@hpl.hp.com>
---

diff --git a/lib/vconn.c b/lib/vconn.c
index f72dbe3ed..d2015897e 100644
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -1255,10 +1255,7 @@ check_action(const union ofp_action *a, unsigned int len, int max_ports)
     switch (ntohs(a->type)) {
     case OFPAT_OUTPUT:
         error = check_action_port(ntohs(a->output.port), max_ports);
-        if (error) {
-            return error;
-        }
-        return check_action_exact_len(a, len, 8);
+        return error ? error : check_action_exact_len(a, len, 8);
 
     case OFPAT_SET_VLAN_VID:
     case OFPAT_SET_VLAN_PCP:
@@ -1274,29 +1271,15 @@ check_action(const union ofp_action *a, unsigned int len, int max_ports)
         return check_action_exact_len(a, len, 16);
 
     case OFPAT_VENDOR:
-        if (a->vendor.vendor == htonl(NX_VENDOR_ID)) {
-            return check_nicira_action(a, len);
-        } else {
-            return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_VENDOR);
-        }
-        break;
+        return (a->vendor.vendor == htonl(NX_VENDOR_ID)
+                ? check_nicira_action(a, len)
+                : ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_VENDOR));
 
     default:
         VLOG_WARN_RL(&bad_ofmsg_rl, "unknown action type %"PRIu16,
                 ntohs(a->type));
         return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_TYPE);
     }
-
-    if (!len) {
-        VLOG_DBG_RL(&bad_ofmsg_rl, "action has invalid length 0");
-        return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN);
-    }
-    if (len % ACTION_ALIGNMENT) {
-        VLOG_DBG_RL(&bad_ofmsg_rl, "action length %u is not a multiple of %d",
-                    len, ACTION_ALIGNMENT);
-        return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN);
-    }
-    return 0;
 }
 
 int
@@ -1316,7 +1299,15 @@ validate_actions(const union ofp_action *actions, size_t n_actions,
                         "action requires %u slots but only %u remain",
                         n_slots, slots_left);
             return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN);
+        } else if (!len) {
+            VLOG_DBG_RL(&bad_ofmsg_rl, "action has invalid length 0");
+            return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN);
+        } else if (len % ACTION_ALIGNMENT) {
+            VLOG_DBG_RL(&bad_ofmsg_rl, "action length %u is not a multiple "
+                        "of %d", len, ACTION_ALIGNMENT);
+            return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_LEN);
         }
+
         error = check_action(a, len, max_ports);
         if (error) {
             return error;