ofproto: Fix memory leak and memory exhaustion bugs in group_mod.

In handle_group_mod() cases where adding a group failed, nothing freed the
list of buckets, causing a leak.  The same was true in every case of
modifying a group.  This commit fixes the problem by changing add_group()
to never steal or free the buckets (modify_group() already acted this way)
and then making handle_group_mod() always free the buckets when it's done.

This approach might at first raise objections, because it makes add_group()
copy the buckets instead of just take the existing ones.  But it actually
fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the
group_mod is reused for the request forwarding.  Until now, for a group_mod
that adds a new group and that has some buckets, the previous stealing of
buckets in add_group() meant that the group_mod's buckets were no longer
valid; in practice, the list of buckets became linked in a way that
iteration never terminated, which caused memory to be exhausted while
composing the requestforward message.  By making add_group() no longer
modify the group_mod, we also fix this problem.

The requestforward test in the testsuite did not find the latter problem
because it only added a group without any buckets.  This commit also
updates the testsuite to include a bucket in its group_mod, which would
have found the problem.

Found by pain and suffering.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Jarno Rajahalme <jarno@ovn.org>

ofproto/ofproto.c

@@ -297,7 +297,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto,
 static bool ofproto_group_exists(const struct ofproto *ofproto,
                                  uint32_t group_id)
     OVS_EXCLUDED(ofproto->groups_rwlock);
-static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
+static enum ofperr add_group(struct ofproto *,
+                             const struct ofputil_group_mod *);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr ofproto_flow_mod_start(struct ofproto *,
                                           struct ofproto_flow_mod *)
@@ -6280,7 +6281,7 @@ handle_queue_get_config_request(struct ofconn *ofconn,
 }
 
 static enum ofperr
-init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
+init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
            struct ofgroup **ofgroup)
 {
     enum ofperr error;
@@ -6306,7 +6307,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
     *CONST_CAST(long long int *, &((*ofgroup)->modified)) = now;
     ovs_refcount_init(&(*ofgroup)->ref_count);
 
-    list_move(&(*ofgroup)->buckets, &gm->buckets);
+    list_init(&(*ofgroup)->buckets);
+    ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL);
+
     *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) =
         list_size(&(*ofgroup)->buckets);
 
@@ -6326,7 +6329,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm,
  * 'ofproto''s group table.  Returns 0 on success or an OpenFlow error code on
  * failure. */
 static enum ofperr
-add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
+add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
 {
     struct ofgroup *ofgroup;
     enum ofperr error;
@@ -6474,7 +6477,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup,
  * ofproto's ofgroup hash map. Thus, the group is never altered while users of
  * the xlate module hold a pointer to the group. */
 static enum ofperr
-modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
+modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
 {
     struct ofgroup *ofgroup, *new_ofgroup, *retiring;
     enum ofperr error;
@@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
             VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d",
                          ofproto->name, gm.command);
         }
-        return OFPERR_OFPGMFC_BAD_COMMAND;
+        error = OFPERR_OFPGMFC_BAD_COMMAND;
     }
 
     if (!error) {
@@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
         rf.group_mod = &gm;
         connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf);
     }
+    ofputil_bucket_list_destroy(&gm.buckets);
+
     return error;
 }
 

tests/ofproto.at

@@ -3135,25 +3135,25 @@ check_async () {
     shift
 
     # OFPGC_ADD
-    ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001
+    ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001 00100000 ffffffffffffffff 00000000"
     if test X"$1" = X"OFPGC_ADD"; then shift;
         echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4):
- ADD group_id=1,type=all"
+ ADD group_id=1,type=all,bucket=actions=drop"
         echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod
- ADD group_id=1,type=all"
+ ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop"
         echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod
- ADD group_id=1,type=all"
+ ADD group_id=1,type=all,bucket=actions=drop"
     fi
 
     # OFPGC_MODIFY
-    ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001
+    ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001 00100000 ffffffffffffffff 00000000"
     if test X"$1" = X"OFPGC_MODIFY"; then shift;
         echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4):
- MOD group_id=1,type=select"
+ MOD group_id=1,type=select,bucket=weight:0,actions=drop"
         echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod
- MOD group_id=1,type=select"
+ MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop"
         echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod
- MOD group_id=1,type=select"
+ MOD group_id=1,type=select,bucket=weight:0,actions=drop"
     fi
 
     ovs-appctl -t `pwd`/c1 ofctl/barrier