> From: "Ben Pfaff" <[email protected]> > To: "Lance Richardson" <[email protected]> > Cc: [email protected] > Sent: Tuesday, 13 June, 2017 11:21:16 AM > Subject: Re: [ovs-dev] [RFC] treewide: undefined behavior, passing null in > nonnull parameters > > On Tue, Jun 13, 2017 at 08:21:23AM -0400, Lance Richardson wrote: > > > From: "Ben Pfaff" <[email protected]> > > > To: "Lance Richardson" <[email protected]> > > > Cc: [email protected] > > > Sent: Tuesday, 13 June, 2017 1:06:12 AM > > > Subject: Re: [ovs-dev] [RFC] treewide: undefined behavior, passing null > > > in nonnull parameters > > > > > > On Mon, Jun 12, 2017 at 08:06:01PM -0400, Lance Richardson wrote: > > > > Eliminate a number of instances of undefined behavior related to > > > > passing NULL in parameters having "nonnull" annotations. > > > > > > > > Found with gcc's undefined behavior sanitizer. > > > > > > > > Signed-off-by: Lance Richardson <[email protected]> > > > > --- > > > > > > > > Posting this as RFC because there is no apparent risk of > > > > unwanted compiler optimizations related to undefined behavior > > > > in existing code. The main value in fixing these issues is > > > > in reducing noise to make it easier to find problematic > > > > cases in the future. > > > > > > > > Here is a small example of the type of unwanted optimization > > > > to be concerned about: > > > > > > > > test1a.c: > > > > > > > > #include <stdio.h> > > > > > > > > extern void foo(char*, size_t); > > > > > > > > int main(int argc, char **argv) > > > > { > > > > char x[128]; > > > > > > > > foo(x, sizeof x); > > > > foo(NULL, 0); > > > > > > > > return 0; > > > > } > > > > > > > > test1b.c: > > > > > > > > #include <stdio.h> > > > > #include <string.h> > > > > > > > > void foo(char *bar, size_t len) > > > > { > > > > memset(bar, 0, len); > > > > > > > > if (bar) > > > > printf("bar is non-null: %p\n", bar); > > > > } > > > > > > > > Compile and run: > > > > gcc -o test -O2 test1a.c test1b.c > > > > ./test > > > > > > > > Output (second line might be a bit of a surprise): > > > > bar is non-null: 0x7fff80f90d50 > > > > bar is non-null: (nil) > > > > > > Hmm. That is surprising. > > > > > > > diff --git a/lib/netlink.c b/lib/netlink.c > > > > index 3da22a1..fcad884 100644 > > > > --- a/lib/netlink.c > > > > +++ b/lib/netlink.c > > > > @@ -241,7 +241,12 @@ void > > > > nl_msg_put_unspec(struct ofpbuf *msg, uint16_t type, > > > > const void *data, size_t size) > > > > { > > > > - memcpy(nl_msg_put_unspec_uninit(msg, type, size), data, size); > > > > + void *ptr; > > > > + > > > > + ptr = nl_msg_put_unspec_uninit(msg, type, size); > > > > + if (size) { > > > > + memcpy(ptr, data, size); > > > > + } > > > > } > > > > > > I guess the above is above null 'data', since 'ptr' should always be > > > nonnull. In that case, it seems reasonable. > > > > > > > /* Appends a Netlink attribute of the given 'type' and no payload to > > > > 'msg'. > > > > diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c > > > > index 3019c4a..2e71fed 100644 > > > > --- a/lib/ofpbuf.c > > > > +++ b/lib/ofpbuf.c > > > > @@ -375,7 +375,9 @@ void * > > > > ofpbuf_put_zeros(struct ofpbuf *b, size_t size) > > > > { > > > > void *dst = ofpbuf_put_uninit(b, size); > > > > - memset(dst, 0, size); > > > > + if (size) { > > > > + memset(dst, 0, size); > > > > + } > > > > return dst; > > > > } > > > > > > In the above, when is dst null? It looks to me like ofpbuf_put_uninit() > > > always returns nonnull. > > > > > > > Looks like it could return NULL if called with b->data = NULL, b->size = 0, > > and > > size = 0. Seems odd to want to append no zero bytes to an empty buffer, but > > it > > apparently happens while running the unit tests. > > OK. Somewhat weird. > > > > > diff --git a/lib/svec.c b/lib/svec.c > > > > index aad04e3..297a60c 100644 > > > > --- a/lib/svec.c > > > > +++ b/lib/svec.c > > > > @@ -127,7 +127,9 @@ compare_strings(const void *a_, const void *b_) > > > > void > > > > svec_sort(struct svec *svec) > > > > { > > > > - qsort(svec->names, svec->n, sizeof *svec->names, compare_strings); > > > > + if (svec->n) { > > > > + qsort(svec->names, svec->n, sizeof *svec->names, > > > > compare_strings); > > > > + } > > > > } > > > > > > This one in svec_sort() looks good to me. > > > > > > > void > > > > diff --git a/lib/util.c b/lib/util.c > > > > index b2a1f8a..ddf8546 100644 > > > > --- a/lib/util.c > > > > +++ b/lib/util.c > > > > @@ -132,7 +132,9 @@ void * > > > > xmemdup(const void *p_, size_t size) > > > > { > > > > void *p = xmalloc(size); > > > > - memcpy(p, p_, size); > > > > + if (size) { > > > > + memcpy(p, p_, size); > > > > + } > > > > return p; > > > > } > > > > > > I guess that the above must be about a null 'p_' parameter? xmalloc() > > > never returns null. > > > > > > Maybe we should invent a nullable_memcpy() helper: > > > > > > /* The C standards say that neither the 'dst' nor 'src' argument to > > > * memcpy() may be null, even if 'n' is zero. This wrapper tolerates > > > * the null case. */ > > > static inline void > > > nullable_memcpy(void *dst, const void *src, size_t n) > > > { > > > if (n) { > > > memcpy(dst, src, n); > > > } > > > } > > > > > > > Makes sense, ditto for a nullable_memset(). > > OK, maybe that's the approach we should take. >
OK, I will respin and post a non-RFC version. Thanks, Lance _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
