| Message ID | 20220216142800.3295236-2-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix undefined behavior in loop macros | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2/16/22 15:27, Adrian Moreno wrote: > Multi-variable loop iterators avoid potential undefined behavior by > using an internal iterator variable to perform the iteration and only > referencing the containing object (via OBJECT_CONTAINING) if the > iterator has been validated via the second expression of the for > statement. > > That way, the user can easily implement a loop that never tries to > obtain the object containing NULL or stack-allocated non-contained > nodes. > > When the loop ends normally (not via "break;") the user-provided > variable is set to NULL. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- Hi Adrian, It feels a bit weird that these new macros are not used anywhere yet (until we apply the next patches in the series). On the other hand I don't think it's easy to split the series otherwise so, with a small nit below: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru > include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index 228b185c3..9c09e8aea 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *); > #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ > ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) > > + Nit: We don't need this extra line. > +/* Multi-variable container iterators. > + * > + * The following macros facilitate safe iteration over data structures > + * contained in objects. It does so by using an internal iterator variable of > + * the type of the member object pointer (i.e: pointer to the data structure). > + */ > + > +/* Multi-variable iterator variable name. > + * Returns the name of the internal iterator variable. > + */ > +#define ITER_VAR(NAME) NAME ## __iterator__ > + > +/* Multi-variable initialization. Creates an internal iterator variable that > + * points to the provided pointer. The type of the iterator variable is > + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. > + * > + * The _EXP version evaluates the extra expressions once. > + */ > +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, ITER_TYPE) \ > + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) > + > +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...) \ > + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) > + > +/* Multi-variable condition. > + * Evaluates the condition expression (that must be based on the internal > + * iterator variable). Only if the result of expression is true, the OBJECT is > + * set to the object containing the current value of the iterator variable. > + * > + * It is up to the caller to make sure it is safe to run OBJECT_CONTAINING on > + * the pointers that verify the condition. > + */ > +#define CONDITION_MULTIVAR(VAR, MEMBER, EXPR) \ > + ((EXPR) ? \ > + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) : \ > + (((VAR) = NULL), 0)) > + > +/* Multi-variable update. > + * Evaluates the expresssion that is supposed to update the iterator variable. > + */ > +#define UPDATE_MULTIVAR(VAR, EXPR) \ > + ((EXPR), (VAR) = NULL) > + > /* Returns the number of elements in ARRAY. */ > #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY) >
Thanks for the review Dumitru, A comment below. On 2/25/22 13:06, Dumitru Ceara wrote: > On 2/16/22 15:27, Adrian Moreno wrote: >> Multi-variable loop iterators avoid potential undefined behavior by >> using an internal iterator variable to perform the iteration and only >> referencing the containing object (via OBJECT_CONTAINING) if the >> iterator has been validated via the second expression of the for >> statement. >> >> That way, the user can easily implement a loop that never tries to >> obtain the object containing NULL or stack-allocated non-contained >> nodes. >> >> When the loop ends normally (not via "break;") the user-provided >> variable is set to NULL. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- > > Hi Adrian, > > It feels a bit weird that these new macros are not used anywhere yet > (until we apply the next patches in the series). On the other hand I > don't think it's easy to split the series otherwise so, with a small nit > below: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Thanks, > Dumitru > >> include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >> index 228b185c3..9c09e8aea 100644 >> --- a/include/openvswitch/util.h >> +++ b/include/openvswitch/util.h >> @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *); >> #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ >> ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) >> >> + > > Nit: We don't need this extra line. > >> +/* Multi-variable container iterators. >> + * >> + * The following macros facilitate safe iteration over data structures >> + * contained in objects. It does so by using an internal iterator variable of >> + * the type of the member object pointer (i.e: pointer to the data structure). >> + */ >> + >> +/* Multi-variable iterator variable name. >> + * Returns the name of the internal iterator variable. >> + */ >> +#define ITER_VAR(NAME) NAME ## __iterator__ >> + >> +/* Multi-variable initialization. Creates an internal iterator variable that >> + * points to the provided pointer. The type of the iterator variable is >> + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. >> + * >> + * The _EXP version evaluates the extra expressions once. >> + */ >> +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, ITER_TYPE) \ >> + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) >> + >> +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...) \ >> + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) >> + >> +/* Multi-variable condition. >> + * Evaluates the condition expression (that must be based on the internal >> + * iterator variable). Only if the result of expression is true, the OBJECT is >> + * set to the object containing the current value of the iterator variable. >> + * >> + * It is up to the caller to make sure it is safe to run OBJECT_CONTAINING on >> + * the pointers that verify the condition. >> + */ >> +#define CONDITION_MULTIVAR(VAR, MEMBER, EXPR) \ >> + ((EXPR) ? \ >> + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) : \ >> + (((VAR) = NULL), 0)) >> + >> +/* Multi-variable update. >> + * Evaluates the expresssion that is supposed to update the iterator variable. >> + */ >> +#define UPDATE_MULTIVAR(VAR, EXPR) \ >> + ((EXPR), (VAR) = NULL) >> + Setting VAR = NULL here may not really be needed. The CONDITION is evaluated just after the UPDATE stage and it sets VAR appropriately. If you agree, I'll remove the entire UPDATE macro alongside the extra line and the extra "s" in "expression" in the next version. >> /* Returns the number of elements in ARRAY. */ >> #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY) >> >
On 3/3/22 07:43, Adrian Moreno wrote: > Thanks for the review Dumitru, > > A comment below. > > On 2/25/22 13:06, Dumitru Ceara wrote: >> On 2/16/22 15:27, Adrian Moreno wrote: >>> Multi-variable loop iterators avoid potential undefined behavior by >>> using an internal iterator variable to perform the iteration and only >>> referencing the containing object (via OBJECT_CONTAINING) if the >>> iterator has been validated via the second expression of the for >>> statement. >>> >>> That way, the user can easily implement a loop that never tries to >>> obtain the object containing NULL or stack-allocated non-contained >>> nodes. >>> >>> When the loop ends normally (not via "break;") the user-provided >>> variable is set to NULL. >>> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >> >> Hi Adrian, >> >> It feels a bit weird that these new macros are not used anywhere yet >> (until we apply the next patches in the series). On the other hand I >> don't think it's easy to split the series otherwise so, with a small nit >> below: >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> >> >> Thanks, >> Dumitru >> >>> include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >>> index 228b185c3..9c09e8aea 100644 >>> --- a/include/openvswitch/util.h >>> +++ b/include/openvswitch/util.h >>> @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char >>> *, const char *, const char *); >>> #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ >>> ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) >>> + >> >> Nit: We don't need this extra line. >> >>> +/* Multi-variable container iterators. >>> + * >>> + * The following macros facilitate safe iteration over data structures >>> + * contained in objects. It does so by using an internal iterator >>> variable of >>> + * the type of the member object pointer (i.e: pointer to the data >>> structure). >>> + */ >>> + >>> +/* Multi-variable iterator variable name. >>> + * Returns the name of the internal iterator variable. >>> + */ >>> +#define ITER_VAR(NAME) NAME ## __iterator__ >>> + >>> +/* Multi-variable initialization. Creates an internal iterator >>> variable that >>> + * points to the provided pointer. The type of the iterator variable is >>> + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. >>> + * >>> + * The _EXP version evaluates the extra expressions once. >>> + */ >>> +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, >>> ITER_TYPE) \ >>> + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) >>> + >>> +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, >>> ...) \ >>> + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) >>> + >>> +/* Multi-variable condition. >>> + * Evaluates the condition expression (that must be based on the >>> internal >>> + * iterator variable). Only if the result of expression is true, the >>> OBJECT is >>> + * set to the object containing the current value of the iterator >>> variable. >>> + * >>> + * It is up to the caller to make sure it is safe to run >>> OBJECT_CONTAINING on >>> + * the pointers that verify the condition. >>> + */ >>> +#define CONDITION_MULTIVAR(VAR, MEMBER, >>> EXPR) \ >>> + ((EXPR) >>> ? \ >>> + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) >>> : \ >>> + (((VAR) = NULL), 0)) >>> + >>> +/* Multi-variable update. >>> + * Evaluates the expresssion that is supposed to update the iterator >>> variable. >>> + */ >>> +#define UPDATE_MULTIVAR(VAR, >>> EXPR) \ >>> + ((EXPR), (VAR) = NULL) >>> + > > Setting VAR = NULL here may not really be needed. The CONDITION is > evaluated just after the UPDATE stage and it sets VAR appropriately. > Good point. The same is true for the _SAFE_* versions in the next patch too, right? > If you agree, I'll remove the entire UPDATE macro alongside the extra > line and the extra "s" in "expression" in the next version. > Sounds good to me, thanks!
On 3/3/22 10:39, Dumitru Ceara wrote: > On 3/3/22 07:43, Adrian Moreno wrote: >> Thanks for the review Dumitru, >> >> A comment below. >> >> On 2/25/22 13:06, Dumitru Ceara wrote: >>> On 2/16/22 15:27, Adrian Moreno wrote: >>>> Multi-variable loop iterators avoid potential undefined behavior by >>>> using an internal iterator variable to perform the iteration and only >>>> referencing the containing object (via OBJECT_CONTAINING) if the >>>> iterator has been validated via the second expression of the for >>>> statement. >>>> >>>> That way, the user can easily implement a loop that never tries to >>>> obtain the object containing NULL or stack-allocated non-contained >>>> nodes. >>>> >>>> When the loop ends normally (not via "break;") the user-provided >>>> variable is set to NULL. >>>> >>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>> --- >>> >>> Hi Adrian, >>> >>> It feels a bit weird that these new macros are not used anywhere yet >>> (until we apply the next patches in the series). On the other hand I >>> don't think it's easy to split the series otherwise so, with a small nit >>> below: >>> >>> Acked-by: Dumitru Ceara <dceara@redhat.com> >>> >>> Thanks, >>> Dumitru >>> >>>> include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 44 insertions(+) >>>> >>>> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >>>> index 228b185c3..9c09e8aea 100644 >>>> --- a/include/openvswitch/util.h >>>> +++ b/include/openvswitch/util.h >>>> @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char >>>> *, const char *, const char *); >>>> #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ >>>> ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) >>>> + >>> >>> Nit: We don't need this extra line. >>> >>>> +/* Multi-variable container iterators. >>>> + * >>>> + * The following macros facilitate safe iteration over data structures >>>> + * contained in objects. It does so by using an internal iterator >>>> variable of >>>> + * the type of the member object pointer (i.e: pointer to the data >>>> structure). >>>> + */ >>>> + >>>> +/* Multi-variable iterator variable name. >>>> + * Returns the name of the internal iterator variable. >>>> + */ >>>> +#define ITER_VAR(NAME) NAME ## __iterator__ >>>> + >>>> +/* Multi-variable initialization. Creates an internal iterator >>>> variable that >>>> + * points to the provided pointer. The type of the iterator variable is >>>> + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. >>>> + * >>>> + * The _EXP version evaluates the extra expressions once. >>>> + */ >>>> +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, >>>> ITER_TYPE) \ >>>> + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) >>>> + >>>> +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, >>>> ...) \ >>>> + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) >>>> + >>>> +/* Multi-variable condition. >>>> + * Evaluates the condition expression (that must be based on the >>>> internal >>>> + * iterator variable). Only if the result of expression is true, the >>>> OBJECT is >>>> + * set to the object containing the current value of the iterator >>>> variable. >>>> + * >>>> + * It is up to the caller to make sure it is safe to run >>>> OBJECT_CONTAINING on >>>> + * the pointers that verify the condition. >>>> + */ >>>> +#define CONDITION_MULTIVAR(VAR, MEMBER, >>>> EXPR) \ >>>> + ((EXPR) >>>> ? \ >>>> + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) >>>> : \ >>>> + (((VAR) = NULL), 0)) >>>> + >>>> +/* Multi-variable update. >>>> + * Evaluates the expresssion that is supposed to update the iterator >>>> variable. >>>> + */ >>>> +#define UPDATE_MULTIVAR(VAR, >>>> EXPR) \ >>>> + ((EXPR), (VAR) = NULL) >>>> + >> >> Setting VAR = NULL here may not really be needed. The CONDITION is >> evaluated just after the UPDATE stage and it sets VAR appropriately. >> > > Good point. The same is true for the _SAFE_* versions in the next patch > too, right? > Yes. >> If you agree, I'll remove the entire UPDATE macro alongside the extra >> line and the extra "s" in "expression" in the next version. >> > > Sounds good to me, thanks! > Ack, thanks.
diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h index 228b185c3..9c09e8aea 100644 --- a/include/openvswitch/util.h +++ b/include/openvswitch/util.h @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *); #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) + +/* Multi-variable container iterators. + * + * The following macros facilitate safe iteration over data structures + * contained in objects. It does so by using an internal iterator variable of + * the type of the member object pointer (i.e: pointer to the data structure). + */ + +/* Multi-variable iterator variable name. + * Returns the name of the internal iterator variable. + */ +#define ITER_VAR(NAME) NAME ## __iterator__ + +/* Multi-variable initialization. Creates an internal iterator variable that + * points to the provided pointer. The type of the iterator variable is + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. + * + * The _EXP version evaluates the extra expressions once. + */ +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, ITER_TYPE) \ + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) + +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...) \ + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) + +/* Multi-variable condition. + * Evaluates the condition expression (that must be based on the internal + * iterator variable). Only if the result of expression is true, the OBJECT is + * set to the object containing the current value of the iterator variable. + * + * It is up to the caller to make sure it is safe to run OBJECT_CONTAINING on + * the pointers that verify the condition. + */ +#define CONDITION_MULTIVAR(VAR, MEMBER, EXPR) \ + ((EXPR) ? \ + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) : \ + (((VAR) = NULL), 0)) + +/* Multi-variable update. + * Evaluates the expresssion that is supposed to update the iterator variable. + */ +#define UPDATE_MULTIVAR(VAR, EXPR) \ + ((EXPR), (VAR) = NULL) + /* Returns the number of elements in ARRAY. */ #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY)
Multi-variable loop iterators avoid potential undefined behavior by using an internal iterator variable to perform the iteration and only referencing the containing object (via OBJECT_CONTAINING) if the iterator has been validated via the second expression of the for statement. That way, the user can easily implement a loop that never tries to obtain the object containing NULL or stack-allocated non-contained nodes. When the loop ends normally (not via "break;") the user-provided variable is set to NULL. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)