Message ID | 20211221110139.14345.96317.stgit@dceara.remote.csb |
---|---|
State | Changes Requested |
Headers | show |
Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Dumitru Ceara <dceara@redhat.com> writes: > UB Sanitizer was flagging OBJECT_CONTAINING() as undefined behavior > due to the way it's used when iterating through collections, e.g., > HMAP_FOR_EACH_INIT(). However, in such cases we don't actually > dereference the pointer, we just use its value. Avoid the undefined > behavior by casting to 'void *' first. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 12/21/21 12:01, Dumitru Ceara wrote: > UB Sanitizer was flagging OBJECT_CONTAINING() as undefined behavior > due to the way it's used when iterating through collections, e.g., > HMAP_FOR_EACH_INIT(). However, in such cases we don't actually > dereference the pointer, we just use its value. Avoid the undefined > behavior by casting to 'void *' first. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > include/openvswitch/util.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h > index 228b185c3a5f..cfbd5f1a3375 100644 > --- a/include/openvswitch/util.h > +++ b/include/openvswitch/util.h > @@ -128,7 +128,7 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *); > * from the type of '*OBJECT'. */ > #define OBJECT_CONTAINING(POINTER, OBJECT, MEMBER) \ > ((OVS_TYPEOF(OBJECT)) (void *) \ > - ((char *) (POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) > + ((uintptr_t)(void *)(POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) > > /* Given POINTER, the address of the given MEMBER within an object of the type > * that that OBJECT points to, assigns the address of the outer object to > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > I've been looking into UB recently and I agree: moving from pointer to integer arithmetics _can_ relax some restrictions that the compiler enforces on pointers, so the patch looks good to me. There are more pointer arithmetics that can be replaced with "uintptr_t" in util.h. Do you plan to replace all of them? If not, I can do it in a series I'm preparing that also addresses UB on this macros.
On 1/20/22 11:11, Adrian Moreno wrote: > > > On 12/21/21 12:01, Dumitru Ceara wrote: >> UB Sanitizer was flagging OBJECT_CONTAINING() as undefined behavior >> due to the way it's used when iterating through collections, e.g., >> HMAP_FOR_EACH_INIT(). However, in such cases we don't actually >> dereference the pointer, we just use its value. Avoid the undefined >> behavior by casting to 'void *' first. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> include/openvswitch/util.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >> index 228b185c3a5f..cfbd5f1a3375 100644 >> --- a/include/openvswitch/util.h >> +++ b/include/openvswitch/util.h >> @@ -128,7 +128,7 @@ OVS_NO_RETURN void ovs_assert_failure(const char >> *, const char *, const char *); >> * from the type of '*OBJECT'. */ >> #define OBJECT_CONTAINING(POINTER, OBJECT, >> MEMBER) \ >> ((OVS_TYPEOF(OBJECT)) (void >> *) \ >> - ((char *) (POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) >> + ((uintptr_t)(void *)(POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) >> /* Given POINTER, the address of the given MEMBER within an object >> of the type >> * that that OBJECT points to, assigns the address of the outer >> object to >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > I've been looking into UB recently and I agree: moving from pointer to > integer arithmetics _can_ relax some restrictions that the compiler > enforces on pointers, so the patch looks good to me. > Thanks for reviewing this! > There are more pointer arithmetics that can be replaced with "uintptr_t" > in util.h. Do you plan to replace all of them? If not, I can do it in a > series I'm preparing that also addresses UB on this macros. > I'm ok to drop this patch if you have a more comprehensive one. As long as the outcome is the same. For example, if applying the rest of this series on top of your patches keeps UBSan happy, then I guess we're good. Regards, Dumitru
diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h index 228b185c3a5f..cfbd5f1a3375 100644 --- a/include/openvswitch/util.h +++ b/include/openvswitch/util.h @@ -128,7 +128,7 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char *); * from the type of '*OBJECT'. */ #define OBJECT_CONTAINING(POINTER, OBJECT, MEMBER) \ ((OVS_TYPEOF(OBJECT)) (void *) \ - ((char *) (POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) + ((uintptr_t)(void *)(POINTER) - OBJECT_OFFSETOF(OBJECT, MEMBER))) /* Given POINTER, the address of the given MEMBER within an object of the type * that that OBJECT points to, assigns the address of the outer object to
UB Sanitizer was flagging OBJECT_CONTAINING() as undefined behavior due to the way it's used when iterating through collections, e.g., HMAP_FOR_EACH_INIT(). However, in such cases we don't actually dereference the pointer, we just use its value. Avoid the undefined behavior by casting to 'void *' first. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- include/openvswitch/util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)