| Message ID | 20170613000601.13775-1-lrichard@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
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@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); } }
> From: "Ben Pfaff" <blp@ovn.org> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: dev@openvswitch.org > 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 <lrichard@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. > 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. > > 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(). Thanks, Lance
On Tue, Jun 13, 2017 at 08:21:23AM -0400, Lance Richardson wrote: > > From: "Ben Pfaff" <blp@ovn.org> > > To: "Lance Richardson" <lrichard@redhat.com> > > Cc: dev@openvswitch.org > > 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 <lrichard@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. > > > > 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.
> From: "Ben Pfaff" <blp@ovn.org> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: dev@openvswitch.org > 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" <blp@ovn.org> > > > To: "Lance Richardson" <lrichard@redhat.com> > > > Cc: dev@openvswitch.org > > > 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 <lrichard@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. > > > > > > > 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
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); + } } /* 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; } 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); + } } 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; } diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index bd0160a..3b2d3e9 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -920,7 +920,9 @@ nbctl_lsp_add(struct ctl_context *ctx) nbrec_logical_switch_verify_ports(ls); struct nbrec_logical_switch_port **new_ports = xmalloc(sizeof *new_ports * (ls->n_ports + 1)); - memcpy(new_ports, ls->ports, sizeof *new_ports * ls->n_ports); + if (ls->n_ports) { + memcpy(new_ports, ls->ports, sizeof *new_ports * ls->n_ports); + } new_ports[ls->n_ports] = CONST_CAST(struct nbrec_logical_switch_port *, lsp); nbrec_logical_switch_set_ports(ls, new_ports, ls->n_ports + 1); @@ -1379,7 +1381,9 @@ nbctl_acl_add(struct ctl_context *ctx) /* Insert the acl into the logical switch. */ nbrec_logical_switch_verify_acls(ls); struct nbrec_acl **new_acls = xmalloc(sizeof *new_acls * (ls->n_acls + 1)); - memcpy(new_acls, ls->acls, sizeof *new_acls * ls->n_acls); + if (ls->n_acls) { + memcpy(new_acls, ls->acls, sizeof *new_acls * ls->n_acls); + } new_acls[ls->n_acls] = acl; nbrec_logical_switch_set_acls(ls, new_acls, ls->n_acls + 1); free(new_acls); @@ -1697,7 +1701,10 @@ nbctl_lr_lb_add(struct ctl_context *ctx) struct nbrec_load_balancer **new_lbs = xmalloc(sizeof *new_lbs * (lr->n_load_balancer + 1)); - memcpy(new_lbs, lr->load_balancer, sizeof *new_lbs * lr->n_load_balancer); + if (lr->n_load_balancer) { + memcpy(new_lbs, lr->load_balancer, + sizeof *new_lbs * lr->n_load_balancer); + } new_lbs[lr->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer *, new_lb); nbrec_logical_router_set_load_balancer(lr, new_lbs, @@ -1793,7 +1800,10 @@ nbctl_ls_lb_add(struct ctl_context *ctx) struct nbrec_load_balancer **new_lbs = xmalloc(sizeof *new_lbs * (ls->n_load_balancer + 1)); - memcpy(new_lbs, ls->load_balancer, sizeof *new_lbs * ls->n_load_balancer); + if (ls->n_load_balancer) { + memcpy(new_lbs, ls->load_balancer, + sizeof *new_lbs * ls->n_load_balancer); + } new_lbs[ls->n_load_balancer] = CONST_CAST(struct nbrec_load_balancer *, new_lb); nbrec_logical_switch_set_load_balancer(ls, new_lbs, @@ -2200,8 +2210,10 @@ nbctl_lr_route_add(struct ctl_context *ctx) nbrec_logical_router_verify_static_routes(lr); struct nbrec_logical_router_static_route **new_routes = xmalloc(sizeof *new_routes * (lr->n_static_routes + 1)); - memcpy(new_routes, lr->static_routes, - sizeof *new_routes * lr->n_static_routes); + if (lr->n_static_routes) { + memcpy(new_routes, lr->static_routes, + sizeof *new_routes * lr->n_static_routes); + } new_routes[lr->n_static_routes] = route; nbrec_logical_router_set_static_routes(lr, new_routes, lr->n_static_routes + 1); @@ -2364,7 +2376,9 @@ nbctl_lr_nat_add(struct ctl_context *ctx) /* Insert the NAT into the logical router. */ nbrec_logical_router_verify_nat(lr); struct nbrec_nat **new_nats = xmalloc(sizeof *new_nats * (lr->n_nat + 1)); - memcpy(new_nats, lr->nat, sizeof *new_nats * lr->n_nat); + if (lr->n_nat) { + memcpy(new_nats, lr->nat, sizeof *new_nats * lr->n_nat); + } new_nats[lr->n_nat] = nat; nbrec_logical_router_set_nat(lr, new_nats, lr->n_nat + 1); free(new_nats); @@ -2642,7 +2656,9 @@ nbctl_lrp_add(struct ctl_context *ctx) nbrec_logical_router_verify_ports(lr); struct nbrec_logical_router_port **new_ports = xmalloc(sizeof *new_ports * (lr->n_ports + 1)); - memcpy(new_ports, lr->ports, sizeof *new_ports * lr->n_ports); + if (lr->n_ports) { + memcpy(new_ports, lr->ports, sizeof *new_ports * lr->n_ports); + } new_ports[lr->n_ports] = CONST_CAST(struct nbrec_logical_router_port *, lrp); nbrec_logical_router_set_ports(lr, new_ports, lr->n_ports + 1);
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@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) lib/netlink.c | 7 ++++++- lib/ofpbuf.c | 4 +++- lib/svec.c | 4 +++- lib/util.c | 4 +++- ovn/utilities/ovn-nbctl.c | 32 ++++++++++++++++++++++++-------- 5 files changed, 39 insertions(+), 12 deletions(-)