diff mbox series

[ovs-dev,03/13] list: use multi-variable helpers for list loops

Message ID 20220124103445.2459400-4-amorenoz@redhat.com
State Superseded
Headers show
Series Fix undefined behavior in loop macros | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Adrian Moreno Jan. 24, 2022, 10:34 a.m. UTC 
Use multi-variable iteration helpers to rewrite non-safe loops.

There is an important behavior change compared with the previous
implementation: When the loop ends normally (i.e: not via "break;"), the
object pointer provided by the user is NULL. This is safer because it's
not guaranteed that it would end up pointing a valid address.

Clang-analyzer has successfully picked the potential null-pointer
dereference on the code that triggered this change (bond.c) and nothing
else has been detected.

For _SAFE loops, keep the (now unused) NEXT/PREV variable to maintain
backwards compatibility.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/openvswitch/list.h | 67 +++++++++++++++++++++-----------------
 ofproto/bond.c             |  2 +-
 tests/test-list.c          |  6 ++--
 3 files changed, 41 insertions(+), 34 deletions(-)

Comments

0-day Robot Jan. 24, 2022, 12:02 p.m. UTC | #1 
Bleep bloop.  Greetings Adrian Moreno, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Use ovs_assert() in place of assert()
#127 FILE: tests/test-list.c:64:
    assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#136 FILE: tests/test-list.c:73:
    assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#145 FILE: tests/test-list.c:151:
            assert(e == NULL);

Lines checked: 152, Warnings: 0, Errors: 3


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h
index 8ad5eeb32..263789b60 100644
--- a/include/openvswitch/list.h
+++ b/include/openvswitch/list.h
@@ -72,36 +72,43 @@  static inline bool ovs_list_is_empty(const struct ovs_list *);
 static inline bool ovs_list_is_singleton(const struct ovs_list *);
 static inline bool ovs_list_is_short(const struct ovs_list *);
 
-#define LIST_FOR_EACH(ITER, MEMBER, LIST)                               \
-    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);                    \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST)                      \
-    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);             \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST)                       \
-    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                    \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_REVERSE_SAFE(ITER, PREV, MEMBER, LIST)        \
-    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                \
-         (&(ITER)->MEMBER != (LIST)                                 \
-          ? INIT_CONTAINER(PREV, (ITER)->MEMBER.prev, MEMBER), 1    \
-          : 0);                                                     \
-         (ITER) = (PREV))
-#define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST)              \
-    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER);           \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)               \
-    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);               \
-         (&(ITER)->MEMBER != (LIST)                                \
-          ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1   \
-          : 0);                                                    \
-         (ITER) = (NEXT))
-#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST)                      \
-    while (!ovs_list_is_empty(LIST)                                    \
+#define LIST_FOR_EACH(VAR, MEMBER, LIST)                                      \
+    for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next);                            \
+         CONDITION_MULTIVAR(ITER_VAR(VAR) != (LIST), VAR, MEMBER);            \
+         UPDATE_MULTIVAR((ITER_VAR(VAR) = ITER_VAR(VAR)->next), VAR))
+
+#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST)                             \
+    for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next);                        \
+         CONDITION_MULTIVAR(ITER_VAR(VAR) != (LIST), VAR, MEMBER);            \
+         UPDATE_MULTIVAR((ITER_VAR(VAR) = ITER_VAR(VAR)->next), VAR))
+
+#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST)                              \
+    for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev);                            \
+         CONDITION_MULTIVAR(ITER_VAR(VAR) != (LIST), VAR, MEMBER);            \
+         UPDATE_MULTIVAR((ITER_VAR(VAR) = ITER_VAR(VAR)->prev), VAR))
+
+
+#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST)                     \
+    for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev);                        \
+         CONDITION_MULTIVAR(ITER_VAR(VAR) != (LIST), VAR, MEMBER);            \
+         UPDATE_MULTIVAR((ITER_VAR(VAR) = ITER_VAR(VAR)->prev), VAR))
+
+#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST)                   \
+    for (INIT_MULTIVAR_SAFE_EXP(VAR, MEMBER, (LIST)->prev, (void) PREV);      \
+         CONDITION_MULTIVAR_SAFE(ITER_VAR(VAR) != (LIST),                     \
+                                 ITER_NEXT_VAR(VAR) = ITER_VAR(VAR)->prev,    \
+                                 VAR,  MEMBER);                               \
+         UPDATE_MULTIVAR_SAFE(VAR))
+
+#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST)                           \
+    for (INIT_MULTIVAR_SAFE_EXP(VAR, MEMBER, (LIST)->next, (void) NEXT);      \
+         CONDITION_MULTIVAR_SAFE(ITER_VAR(VAR) != (LIST),                     \
+                                 ITER_NEXT_VAR(VAR) = ITER_VAR(VAR)->next,    \
+                                 VAR,  MEMBER);                               \
+         UPDATE_MULTIVAR_SAFE(VAR))
+
+#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST)                                 \
+    while (!ovs_list_is_empty(LIST)                                           \
            && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1))
 
 /* Inline implementations. */
diff --git a/ofproto/bond.c b/ofproto/bond.c
index cdfdf0b9d..6e95dfdff 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1258,7 +1258,7 @@  insert_bal(struct ovs_list *bals, struct bond_member *member)
             break;
         }
     }
-    ovs_list_insert(&pos->bal_node, &member->bal_node);
+    ovs_list_insert(pos? &pos->bal_node: bals, &member->bal_node);
 }
 
 /* Removes 'member' from its current list and then inserts it into 'bals' so
diff --git a/tests/test-list.c b/tests/test-list.c
index 6f1fb059b..30a0be17b 100644
--- a/tests/test-list.c
+++ b/tests/test-list.c
@@ -61,7 +61,7 @@  check_list(struct ovs_list *list, const int values[], size_t n)
         assert(e->value == values[i]);
         i++;
     }
-    assert(&e->node == list);
+    assert(e == NULL);
     assert(i == n);
 
     i = 0;
@@ -70,7 +70,7 @@  check_list(struct ovs_list *list, const int values[], size_t n)
         assert(e->value == values[n - i - 1]);
         i++;
     }
-    assert(&e->node == list);
+    assert(e == NULL);
     assert(i == n);
 
     assert(ovs_list_is_empty(list) == !n);
@@ -148,7 +148,7 @@  test_list_for_each_safe(void)
                 i++;
             }
             assert(i == n);
-            assert(&e->node == &list);
+            assert(e == NULL);
 
             for (i = 0; i < n; i++) {
                 if (pattern & (1ul << i)) {