ofp-parse: Correctly update bucket lists if they are empty.

Previously, list_moved() only worked with non-empty lists, but this was a
caveat that was really easy to miss.  parse_ofp_group_mod_file() had a bug
because it didn't honor that restriction.  This commit fixes the problem,
by modifying the list_moved() interface to be harder to use incorrectly
and then updating the callers.

Reported-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Thomas Graf <tgraf@noironetworks.com>

diff --git a/lib/list.c b/lib/list.c
index e341d45..0bfa902 100644 (file)
--- a/lib/list.c
+++ b/lib/list.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -90,15 +90,21 @@ list_replace(struct list *element, const struct list *position)
 }
 
 /* Adjusts pointers around 'list' to compensate for 'list' having been moved
- * around in memory (e.g. as a consequence of realloc()).
+ * around in memory (e.g. as a consequence of realloc()), with original
+ * location 'orig'.
  *
- * This always works if 'list' is a member of a list, or if 'list' is the head
- * of a non-empty list.  It fails badly, however, if 'list' is the head of an
- * empty list; just use list_init() in that case. */
+ * ('orig' likely points to freed memory, but this function does not
+ * dereference 'orig', it only compares it to 'list'.  In a very pedantic
+ * language lawyer sense, this still yields undefined behavior, but it works
+ * with actual compilers.) */
 void
-list_moved(struct list *list)
+list_moved(struct list *list, const struct list *orig)
 {
-    list->prev->next = list->next->prev = list;
+    if (list->next == orig) {
+        list_init(list);
+    } else {
+        list->prev->next = list->next->prev = list;
+    }
 }
 
 /* Initializes 'dst' with the contents of 'src', compensating for moving it
@@ -107,12 +113,8 @@ list_moved(struct list *list)
 void
 list_move(struct list *dst, struct list *src)
 {
-    if (!list_is_empty(src)) {
-        *dst = *src;
-        list_moved(dst);
-    } else {
-        list_init(dst);
-    }
+    *dst = *src;
+    list_moved(dst, src);
 }
 
 /* Removes 'elem' from its list and returns the element that followed it.

diff --git a/lib/list.h b/lib/list.h
index 0da082e..a257f8a 100644 (file)
--- a/lib/list.h
+++ b/lib/list.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2013, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -39,7 +39,7 @@ void list_splice(struct list *before, struct list *first, struct list *last);
 void list_push_front(struct list *, struct list *);
 void list_push_back(struct list *, struct list *);
 void list_replace(struct list *, const struct list *);
-void list_moved(struct list *);
+void list_moved(struct list *, const struct list *orig);
 void list_move(struct list *dst, struct list *src);
 
 /* List removal. */

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index c759f03..6f8f50d 100644 (file)
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -2340,12 +2340,14 @@ parse_ofp_group_mod_file(const char *file_name, uint16_t command,
         char *error;
 
         if (*n_gms >= allocated_gms) {
+            struct ofputil_group_mod *new_gms;
             size_t i;
 
-            *gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
+            new_gms = x2nrealloc(*gms, &allocated_gms, sizeof **gms);
             for (i = 0; i < *n_gms; i++) {
-                list_moved(&(*gms)[i].buckets);
+                list_moved(&new_gms[i].buckets, &(*gms)[i].buckets);
             }
+            *gms = new_gms;
         }
         error = parse_ofp_group_mod_str(&(*gms)[*n_gms], command, ds_cstr(&s),
                                         &usable);