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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
