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 <[email protected]>
Some nits below, rest looks good:
Acked-by: Eelco Chaudron <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
