[ovs-dev] [PATCH] ofp-parse: Correctly update bucket lists if they are empty

[Simon Horman]

list_moved() appears to only work reliably if its argument is a non-empty
list. This is because list_moved dereferences the next and prev fields
of the list, which for an empty list are the address of the list itself,
and list_moved() is intended to be called when the address of the list has
changed.

This patch updates the only user of list_moved() that I was able to find
my temporarily setting the next field to NULL if it is empty and then
re-initialising the list rather than calling list_moved() after
reallocation has occurred.

An test update is also provided to exercise this change. Without
the code change the updated test cases a segmentation when the
buckets parameter of ofputil_put_ofp11_bucket() is dereferenced.

Signed-off-by: Simon Horman <simon.horman at netronome.com>

---

* Although somewhat cure the approach of setting the next field to NULL
  seems far less dangerous than trying to update the list infrastructure
  to handle this case.

  I also toyed with using a temporary bitmap to track empty buckets.
  That approach worked but seemed to be more overhead and no cleaner
  than the one taken by this patch.
---
 lib/ofp-parse.c  | 11 ++++++++++-
 tests/ofproto.at | 13 +++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 9acf6a4..9a5df3b 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1415,9 +1415,18 @@ parse_ofp_group_mod_file(const char *file_name, uint16_t command,
         if (*n_gms >= allocated_gms) {
             size_t i;
 
+            for (i = 0; i < *n_gms; i++) {
+                if (list_is_empty(&(*gms)[i].buckets)) {
+                    (*gms)[i].buckets.next = NULL;
+                }
+            }
             *gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
             for (i = 0; i < *n_gms; i++) {
-                list_moved(&(*gms)[i].buckets);
+                if ((*gms)[i].buckets.next) {
+                    list_moved(&(*gms)[i].buckets);
+                } else {
+                    list_init(&(*gms)[i].buckets);
+                }
             }
         }
         error = parse_ofp_group_mod_str(&(*gms)[*n_gms], command, ds_cstr(&s),
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 2a2111f..39978e4 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -286,16 +286,25 @@ dnl Actions definition listed in both supported formats (w/ actions=)
 AT_SETUP([ofproto - del group (OpenFlow 1.1)])
 OVS_VSWITCHD_START
 AT_DATA([groups.txt], [dnl
+group_id=1231,type=all
+group_id=1232,type=all
+group_id=1233,type=all
 group_id=1234,type=all,bucket=output:10
 group_id=1235,type=all,bucket=actions=output:10
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-groups br0 groups.txt])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0 ], [0], [stdout])
-AT_CHECK([STRIP_XIDS stdout], [0], [dnl
-OFPST_GROUP_DESC reply (OF1.1):
+AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl
+ group_id=1231,type=all
+ group_id=1232,type=all
+ group_id=1233,type=all
  group_id=1234,type=all,bucket=actions=output:10
  group_id=1235,type=all,bucket=actions=output:10
+OFPST_GROUP_DESC reply (OF1.1):
 ])
+AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1231])
+AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1232])
+AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1233])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1234])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
-- 
2.1.4