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 <[email protected]> --- include/openvswitch/list.h | 67 +++++++++++++++++++++----------------- ofproto/bond.c | 2 +- tests/test-list.c | 6 ++-- 3 files changed, 41 insertions(+), 34 deletions(-) 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)) { -- 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
