| Message ID | 20220309161023.3347758-5-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix undefined behavior in loop macros | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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()
#134 FILE: tests/test-list.c:64:
assert(e == NULL);
ERROR: Use ovs_assert() in place of assert()
#143 FILE: tests/test-list.c:73:
assert(e == NULL);
ERROR: Use ovs_assert() in place of assert()
#153 FILE: tests/test-list.c:140:
assert(next == NULL);
ERROR: Use ovs_assert() in place of assert()
#155 FILE: tests/test-list.c:142:
assert(&next->node == e->node.next);
ERROR: Use ovs_assert() in place of assert()
#166 FILE: tests/test-list.c:158:
assert(e == NULL);
ERROR: Use ovs_assert() in place of assert()
#167 FILE: tests/test-list.c:159:
assert(next == NULL);
Lines checked: 174, Warnings: 0, Errors: 6
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > 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. > > For pop iterator, set the variable to NULL when the loop ends. > > 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, use the LONG version for backwards compatibility. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> Some nits below, rest looks good: Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > include/openvswitch/list.h | 73 ++++++++++++++++++++++---------------- > ofproto/bond.c | 2 +- > tests/test-list.c | 14 ++++++-- > 3 files changed, 54 insertions(+), 35 deletions(-) > > diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h > index 8ad5eeb32..bbd2edbd0 100644 > --- a/include/openvswitch/list.h > +++ b/include/openvswitch/list.h > @@ -72,37 +72,48 @@ 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) \ > - && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1)) > +#define LIST_FOR_EACH(VAR, MEMBER, LIST) \ > + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list); \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) > + > +#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST) \ > + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list); \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) > + > +#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST) \ > + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list); \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) > + > +#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST) \ > + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list); \ > + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ > + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) > + > +#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) \ > + for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, \ > + struct ovs_list); \ > + CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, \ > + ITER_VAR(VAR) != (LIST), \ > + ITER_VAR(PREV) = ITER_VAR(VAR)->prev, \ > + ITER_VAR(PREV) != (LIST)); \ > + UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV)) > + > +#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST) \ > + for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, \ > + struct ovs_list); \ > + CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, \ > + ITER_VAR(VAR) != (LIST), \ > + ITER_VAR(NEXT) = ITER_VAR(VAR)->next, \ > + ITER_VAR(NEXT) != (LIST)); \ > + UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT)) > + > +#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST) \ > + while (!ovs_list_is_empty(LIST) ? \ > + (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1) : \ > + (ITER = NULL, 0)) > > /* Inline implementations. */ > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index cdfdf0b9d..7e30fd8bd 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); Space before : > } > > /* 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..7c02ea40f 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); > @@ -135,6 +135,13 @@ test_list_for_each_safe(void) > values_idx = 0; > n_remaining = n; > LIST_FOR_EACH_SAFE (e, next, node, &list) { > + /*next is valid as long as it's not pointing to &list*/ Capital N for next, and ending dot. > + if (&e->node == list.prev) { > + assert(next == NULL); > + } else { > + assert(&next->node == e->node.next); > + } > + > assert(i < n); > if (pattern & (1ul << i)) { > ovs_list_remove(&e->node); > @@ -148,7 +155,8 @@ test_list_for_each_safe(void) > i++; > } > assert(i == n); > - assert(&e->node == &list); > + assert(e == NULL); > + assert(next == NULL); > > for (i = 0; i < n; i++) { > if (pattern & (1ul << i)) { > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 3/18/22 14:32, Eelco Chaudron wrote: > > > On 9 Mar 2022, at 17:10, Adrian Moreno wrote: > >> 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. >> >> For pop iterator, set the variable to NULL when the loop ends. >> >> 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, use the LONG version for backwards compatibility. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > > Some nits below, rest looks good: > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> --- >> include/openvswitch/list.h | 73 ++++++++++++++++++++++---------------- >> ofproto/bond.c | 2 +- >> tests/test-list.c | 14 ++++++-- >> 3 files changed, 54 insertions(+), 35 deletions(-) >> >> diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h >> index 8ad5eeb32..bbd2edbd0 100644 >> --- a/include/openvswitch/list.h >> +++ b/include/openvswitch/list.h >> @@ -72,37 +72,48 @@ 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) \ >> - && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1)) >> +#define LIST_FOR_EACH(VAR, MEMBER, LIST) \ >> + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list); \ >> + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ >> + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) >> + >> +#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST) \ >> + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list); \ >> + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ >> + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) >> + >> +#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST) \ >> + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list); \ >> + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ >> + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) >> + >> +#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST) \ >> + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list); \ >> + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ >> + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) >> + >> +#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) \ >> + for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, \ >> + struct ovs_list); \ >> + CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, \ >> + ITER_VAR(VAR) != (LIST), \ >> + ITER_VAR(PREV) = ITER_VAR(VAR)->prev, \ >> + ITER_VAR(PREV) != (LIST)); \ >> + UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV)) >> + >> +#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST) \ >> + for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, \ >> + struct ovs_list); \ >> + CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, \ >> + ITER_VAR(VAR) != (LIST), \ >> + ITER_VAR(NEXT) = ITER_VAR(VAR)->next, \ >> + ITER_VAR(NEXT) != (LIST)); \ >> + UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT)) >> + >> +#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST) \ >> + while (!ovs_list_is_empty(LIST) ? \ >> + (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1) : \ >> + (ITER = NULL, 0)) >> >> /* Inline implementations. */ >> >> diff --git a/ofproto/bond.c b/ofproto/bond.c >> index cdfdf0b9d..7e30fd8bd 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); > > Space before : > Ugh, I forgot this one. It was also pointed out by Dumitru I believe. >> } >> >> /* 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..7c02ea40f 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); >> @@ -135,6 +135,13 @@ test_list_for_each_safe(void) >> values_idx = 0; >> n_remaining = n; >> LIST_FOR_EACH_SAFE (e, next, node, &list) { >> + /*next is valid as long as it's not pointing to &list*/ > > Capital N for next, and ending dot. > I'm referring to the "next" variable so maybe just quote it? >> + if (&e->node == list.prev) { >> + assert(next == NULL); >> + } else { >> + assert(&next->node == e->node.next); >> + } >> + >> assert(i < n); >> if (pattern & (1ul << i)) { >> ovs_list_remove(&e->node); >> @@ -148,7 +155,8 @@ test_list_for_each_safe(void) >> i++; >> } >> assert(i == n); >> - assert(&e->node == &list); >> + assert(e == NULL); >> + assert(next == NULL); >> >> for (i = 0; i < n; i++) { >> if (pattern & (1ul << i)) { >> -- >> 2.34.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h index 8ad5eeb32..bbd2edbd0 100644 --- a/include/openvswitch/list.h +++ b/include/openvswitch/list.h @@ -72,37 +72,48 @@ 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) \ - && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1)) +#define LIST_FOR_EACH(VAR, MEMBER, LIST) \ + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list); \ + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) + +#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST) \ + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list); \ + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next)) + +#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST) \ + for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list); \ + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) + +#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST) \ + for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list); \ + CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); \ + UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) + +#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) \ + for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, \ + struct ovs_list); \ + CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, \ + ITER_VAR(VAR) != (LIST), \ + ITER_VAR(PREV) = ITER_VAR(VAR)->prev, \ + ITER_VAR(PREV) != (LIST)); \ + UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV)) + +#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST) \ + for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, \ + struct ovs_list); \ + CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, \ + ITER_VAR(VAR) != (LIST), \ + ITER_VAR(NEXT) = ITER_VAR(VAR)->next, \ + ITER_VAR(NEXT) != (LIST)); \ + UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT)) + +#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST) \ + while (!ovs_list_is_empty(LIST) ? \ + (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1) : \ + (ITER = NULL, 0)) /* Inline implementations. */ diff --git a/ofproto/bond.c b/ofproto/bond.c index cdfdf0b9d..7e30fd8bd 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..7c02ea40f 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); @@ -135,6 +135,13 @@ test_list_for_each_safe(void) values_idx = 0; n_remaining = n; LIST_FOR_EACH_SAFE (e, next, node, &list) { + /*next is valid as long as it's not pointing to &list*/ + if (&e->node == list.prev) { + assert(next == NULL); + } else { + assert(&next->node == e->node.next); + } + assert(i < n); if (pattern & (1ul << i)) { ovs_list_remove(&e->node); @@ -148,7 +155,8 @@ test_list_for_each_safe(void) i++; } assert(i == n); - assert(&e->node == &list); + assert(e == NULL); + assert(next == NULL); for (i = 0; i < n; i++) { if (pattern & (1ul << i)) {
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. For pop iterator, set the variable to NULL when the loop ends. 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, use the LONG version for backwards compatibility. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- include/openvswitch/list.h | 73 ++++++++++++++++++++++---------------- ofproto/bond.c | 2 +- tests/test-list.c | 14 ++++++-- 3 files changed, 54 insertions(+), 35 deletions(-)