[ovs-dev] [RFC] treewide: undefined behavior, passing null in nonnull parameters

[Ben Pfaff]

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 <lrichard at redhat.com>
> ---
> 
> 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.

> 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);
    }
}