ofproto: Avoid wasting memory malloc()'ing empty action sets for subrules.

GNU libc treats malloc(0) as malloc(1).  Subrules always have an n_actions
of 0, so this code was wasting time and memory for subrules.  This commit
stops doing that.

Also audits and fixes some very pedantic potential problems with null
pointers; e.g. the C standard says that NULL may not be compared with the
< operator, even if both arguments are null, and it also says that a null
pointer may not be passed to memcpy() or memcmp(), even if the length is
zero.

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7a2e17c..6d04586 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -619,9 +619,10 @@ int
 validate_actions(const union ofp_action *actions, size_t n_actions,
                  int max_ports)
 {
-    const union ofp_action *a;
+    size_t i;
 
-    for (a = actions; a < &actions[n_actions]; ) {
+    for (i = 0; i < n_actions; ) {
+        const union ofp_action *a = &actions[i];
         unsigned int len = ntohs(a->header.len);
         unsigned int n_slots = len / ACTION_ALIGNMENT;
         unsigned int slots_left = &actions[n_actions] - a;
@@ -645,7 +646,7 @@ validate_actions(const union ofp_action *actions, size_t n_actions,
         if (error) {
             return error;
         }
-        a += n_slots;
+        i += n_slots;
     }
     return 0;
 }
@@ -679,7 +680,7 @@ actions_first(struct actions_iterator *iter,
 const union ofp_action *
 actions_next(struct actions_iterator *iter)
 {
-    if (iter->pos < iter->end) {
+    if (iter->pos != iter->end) {
         const union ofp_action *a = iter->pos;
         unsigned int len = ntohs(a->header.len);
         iter->pos += len / ACTION_ALIGNMENT;

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3d2989a..9a5db34 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1871,8 +1871,10 @@ rule_create(struct ofproto *ofproto, struct rule *super,
     } else {
         list_init(&rule->list);
     }
-    rule->n_actions = n_actions;
-    rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+    if (n_actions > 0) {
+        rule->n_actions = n_actions;
+        rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+    }
     netflow_flow_clear(&rule->nf_flow);
     netflow_flow_update_time(ofproto->netflow, &rule->nf_flow, rule->created);
 
@@ -3266,7 +3268,9 @@ flow_stats_cb(struct cls_rule *rule_, void *cbdata_)
     memset(ofs->pad2, 0, sizeof ofs->pad2);
     ofs->packet_count = htonll(packet_count);
     ofs->byte_count = htonll(byte_count);
-    memcpy(ofs->actions, rule->actions, act_len);
+    if (rule->n_actions > 0) {
+        memcpy(ofs->actions, rule->actions, act_len);
+    }
 }
 
 static int
@@ -3335,7 +3339,9 @@ flow_stats_ds_cb(struct cls_rule *rule_, void *cbdata_)
     ds_put_format(results, "n_packets=%"PRIu64", ", packet_count);
     ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
     ofp_print_match(results, &match, true);
-    ofp_print_actions(results, &rule->actions->header, act_len);
+    if (act_len > 0) {
+        ofp_print_actions(results, &rule->actions->header, act_len);
+    }
     ds_put_cstr(results, "\n");
 }
 
@@ -3763,14 +3769,14 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
 
     /* If the actions are the same, do nothing. */
     if (n_actions == rule->n_actions
-        && !memcmp(ofm->actions, rule->actions, actions_len))
+        && (!n_actions || !memcmp(ofm->actions, rule->actions, actions_len)))
     {
         return 0;
     }
 
     /* Replace actions. */
     free(rule->actions);
-    rule->actions = xmemdup(ofm->actions, actions_len);
+    rule->actions = n_actions ? xmemdup(ofm->actions, actions_len) : NULL;
     rule->n_actions = n_actions;
 
     /* Make sure that the datapath gets updated properly. */