diff mbox

[ovs-dev] ofproto: Fix inserting buckets at the end of an empty group.

Message ID 1444867974-6175-1-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 15, 2015, 12:12 a.m. UTC 
This caused a segfault.

Reported-by: Ray Li <rayli1107@gmail.com>
Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018746.html
Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 AUTHORS           |  1 +
 ofproto/ofproto.c | 16 +++++++++-------
 tests/ofproto.at  | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 8 deletions(-)

Comments

Simon Horman Nov. 25, 2015, 8:36 a.m. UTC | #1 
On Wed, Oct 14, 2015 at 05:12:54PM -0700, Ben Pfaff wrote:
> This caused a segfault.
> 
> Reported-by: Ray Li <rayli1107@gmail.com>
> Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018746.html
> Signed-off-by: Ben Pfaff <blp@nicira.com>

"Déjà vu all over again"

Reviewed-by: Simon Horman <simon.horman@netronome.com>
Ben Pfaff Nov. 29, 2015, 7:18 p.m. UTC | #2 
On Wed, Nov 25, 2015 at 05:36:39PM +0900, Simon Horman wrote:
> On Wed, Oct 14, 2015 at 05:12:54PM -0700, Ben Pfaff wrote:
> > This caused a segfault.
> > 
> > Reported-by: Ray Li <rayli1107@gmail.com>
> > Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018746.html
> > Signed-off-by: Ben Pfaff <blp@nicira.com>
> 
> "Déjà vu all over again"
> 
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

Thanks Simon, applied to master and branch-2.4.
diff mbox

Patch

diff --git a/AUTHORS b/AUTHORS
index f4e1ca9..04794fb 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -343,6 +343,7 @@  Pratap Reddy            preddy@nicira.com
 Ralf Heiringhoff        ralf@frosty-geek.net
 Ram Jothikumar          rjothikumar@nicira.com
 Ramana Reddy            gtvrreddy@gmail.com
+Ray Li                  rayli1107@gmail.com
 RishiRaj Maulick        rishi.raj2509@gmail.com
 Rob Sherwood            rob.sherwood@bigswitch.com
 Robert Strickler        anomalyst@gmail.com
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c7dd8a2..389f853 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6375,15 +6375,17 @@  copy_buckets_for_insert_bucket(const struct ofgroup *ofgroup,
 
     /* Rearrange list according to command_bucket_id */
     if (command_bucket_id == OFPG15_BUCKET_LAST) {
-        struct ofputil_bucket *new_first;
-        const struct ofputil_bucket *first;
+        if (!list_is_empty(&ofgroup->buckets)) {
+            struct ofputil_bucket *new_first;
+            const struct ofputil_bucket *first;
 
-        first = ofputil_bucket_list_front(&ofgroup->buckets);
-        new_first = ofputil_bucket_find(&new_ofgroup->buckets,
-                                        first->bucket_id);
+            first = ofputil_bucket_list_front(&ofgroup->buckets);
+            new_first = ofputil_bucket_find(&new_ofgroup->buckets,
+                                            first->bucket_id);
 
-        list_splice(new_ofgroup->buckets.next, &new_first->list_node,
-                    &new_ofgroup->buckets);
+            list_splice(new_ofgroup->buckets.next, &new_first->list_node,
+                        &new_ofgroup->buckets);
+        }
     } else if (command_bucket_id <= OFPG15_BUCKET_MAX && last) {
         struct ofputil_bucket *after;
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 5e4441c..cead710 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -417,33 +417,61 @@  dnl It at least checks request and reply serialization and deserialization.
 dnl Actions definition listed in both supported formats (w/ actions=)
 AT_SETUP([ofproto - insert buckets])
 OVS_VSWITCHD_START
+# Add group with no buckets.
 AT_DATA([groups.txt], [dnl
-group_id=1234,type=all,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11
+group_id=1234,type=all
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt])
 AT_CHECK([ovs-ofctl -F OXM-OpenFlow15 -O OpenFlow15 -vwarn dump-groups br0 1234], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
+ group_id=1234,type=all
+])
+
+# Add two buckets, using "last".
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=last,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11])
+AT_CHECK([ovs-ofctl -F OXM-OpenFlow15 -O OpenFlow15 -vwarn dump-groups br0 1234], [0], [stdout])
+AT_CHECK([STRIP_XIDS stdout], [0], [dnl
+OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11
 ])
+
+# Start over again, then add two buckets using "first".
+AT_CHECK([ovs-ofctl -O OpenFlow15 --strict del-groups br0 group_id=1234])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-group br0 group_id=1234,type=all])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=first,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11])
+AT_CHECK([ovs-ofctl -F OXM-OpenFlow15 -O OpenFlow15 -vwarn dump-groups br0 1234], [0], [stdout])
+AT_CHECK([STRIP_XIDS stdout], [0], [dnl
+OFPST_GROUP_DESC reply (OF1.5):
+ group_id=1234,type=all,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11
+])
+
+# Add two more buckets before the existing ones.
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=first,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11
 ])
+
+# Add another bucket at the end.
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=last,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15
 ])
+
+# Add another bucket just before bucket 15.
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=15])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15
 ])
+
+# Add two more buckets just before bucket 11,
+# getting the command from a file.
 AT_DATA([buckets.txt], [dnl
 group_id=1234,command_bucket_id=11,bucket=bucket_id:12,actions=output:12,bucket=bucket_id:13,actions=output:13
 ])
@@ -453,6 +481,8 @@  AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:12,actions=output:12,bucket=bucket_id:13,actions=output:13,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15
 ])
+
+# Add yet two more buckets.
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=15,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl