| Message ID | 11dfd6b80a350ceafbd25b2b68cf283b523d1f12.1704991344.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v2] util: Annotate function that will never return NULL. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1/11/24 17:42, Eelco Chaudron wrote: > The make clang-analyze target reports an 'Dereference of null > pointer' and an 'Uninitialized argument value' issue due to > it assumes some function can return NULL. > > This patch annotates these functions, so the static analyzer > is aware of this. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- Hi Eelco, Thanks for the patch! These were also causing static analysis warnings in OVN code and getting them annotated will make those go away too. > > v2: Accidentally added nullable_xstrdup(), removed it. > > include/openvswitch/compiler.h | 6 ++++++ > lib/util.h | 30 +++++++++++++++--------------- > 2 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h > index 52614a5ac..878c5c6a7 100644 > --- a/include/openvswitch/compiler.h > +++ b/include/openvswitch/compiler.h > @@ -37,6 +37,12 @@ > #define OVS_NO_RETURN > #endif > > +#if __GNUC__ && !__CHECKER__ > +#define OVS_RETURNS_NONNULL __attribute__((returns_nonnull)) > +#else > +#define OVS_RETURNS_NONNULL > +#endif > + > #ifndef typeof > #define typeof __typeof__ > #endif > diff --git a/lib/util.h b/lib/util.h > index 62801e85f..33e195878 100644 > --- a/lib/util.h > +++ b/lib/util.h > @@ -162,13 +162,13 @@ bool memory_locked(void); > OVS_NO_RETURN void out_of_memory(void); > > /* Allocation wrappers that abort if memory is exhausted. */ > -void *xmalloc(size_t) MALLOC_LIKE; > -void *xcalloc(size_t, size_t) MALLOC_LIKE; > -void *xzalloc(size_t) MALLOC_LIKE; > -void *xrealloc(void *, size_t); > -void *xmemdup(const void *, size_t) MALLOC_LIKE; > -char *xmemdup0(const char *, size_t) MALLOC_LIKE; > -char *xstrdup(const char *) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xmalloc(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xcalloc(size_t, size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xzalloc(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xrealloc(void *, size_t); > +OVS_RETURNS_NONNULL void *xmemdup(const void *, size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL char *xmemdup0(const char *, size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL char *xstrdup(const char *) MALLOC_LIKE; > char *nullable_xstrdup(const char *) MALLOC_LIKE; > bool nullable_string_is_equal(const char *a, const char *b); > char *xasprintf(const char *format, ...) OVS_PRINTF_FORMAT(1, 2) MALLOC_LIKE; xasprintf also never returns NULL. I guess you didn't annotate it because internally it calls xvasprintf() -> xmalloc(). However, xstrdup() and x2nrealloc() also indirectly/directly call xmalloc() and those were annotated. Would it be nicer to just annotate all top level functions that never return NULL in util.h? Regards, Dumitru > @@ -177,13 +177,13 @@ void *x2nrealloc(void *p, size_t *n, size_t s); > > /* Allocation wrappers for specialized situations where coverage counters > * cannot be used. */ > -void *xmalloc__(size_t) MALLOC_LIKE; > -void *xcalloc__(size_t, size_t) MALLOC_LIKE; > -void *xzalloc__(size_t) MALLOC_LIKE; > -void *xrealloc__(void *, size_t); > +OVS_RETURNS_NONNULL void *xmalloc__(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xcalloc__(size_t, size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xzalloc__(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xrealloc__(void *, size_t); > > -void *xmalloc_cacheline(size_t) MALLOC_LIKE; > -void *xzalloc_cacheline(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xmalloc_cacheline(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xzalloc_cacheline(size_t) MALLOC_LIKE; > void free_cacheline(void *); > > void ovs_strlcpy(char *dst, const char *src, size_t size); > @@ -191,9 +191,9 @@ void ovs_strzcpy(char *dst, const char *src, size_t size); > > int string_ends_with(const char *str, const char *suffix); > > -void *xmalloc_pagealign(size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xmalloc_pagealign(size_t) MALLOC_LIKE; > void free_pagealign(void *); > -void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; > +OVS_RETURNS_NONNULL void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; > void free_size_align(void *); > > /* The C standards say that neither the 'dst' nor 'src' argument to
On 12 Jan 2024, at 10:54, Dumitru Ceara wrote: > On 1/11/24 17:42, Eelco Chaudron wrote: >> The make clang-analyze target reports an 'Dereference of null >> pointer' and an 'Uninitialized argument value' issue due to >> it assumes some function can return NULL. >> >> This patch annotates these functions, so the static analyzer >> is aware of this. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- > > Hi Eelco, > > Thanks for the patch! These were also causing static analysis warnings > in OVN code and getting them annotated will make those go away too. > >> >> v2: Accidentally added nullable_xstrdup(), removed it. >> >> include/openvswitch/compiler.h | 6 ++++++ >> lib/util.h | 30 +++++++++++++++--------------- >> 2 files changed, 21 insertions(+), 15 deletions(-) >> >> diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h >> index 52614a5ac..878c5c6a7 100644 >> --- a/include/openvswitch/compiler.h >> +++ b/include/openvswitch/compiler.h >> @@ -37,6 +37,12 @@ >> #define OVS_NO_RETURN >> #endif >> >> +#if __GNUC__ && !__CHECKER__ >> +#define OVS_RETURNS_NONNULL __attribute__((returns_nonnull)) >> +#else >> +#define OVS_RETURNS_NONNULL >> +#endif >> + >> #ifndef typeof >> #define typeof __typeof__ >> #endif >> diff --git a/lib/util.h b/lib/util.h >> index 62801e85f..33e195878 100644 >> --- a/lib/util.h >> +++ b/lib/util.h >> @@ -162,13 +162,13 @@ bool memory_locked(void); >> OVS_NO_RETURN void out_of_memory(void); >> >> /* Allocation wrappers that abort if memory is exhausted. */ >> -void *xmalloc(size_t) MALLOC_LIKE; >> -void *xcalloc(size_t, size_t) MALLOC_LIKE; >> -void *xzalloc(size_t) MALLOC_LIKE; >> -void *xrealloc(void *, size_t); >> -void *xmemdup(const void *, size_t) MALLOC_LIKE; >> -char *xmemdup0(const char *, size_t) MALLOC_LIKE; >> -char *xstrdup(const char *) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xmalloc(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xcalloc(size_t, size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xzalloc(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xrealloc(void *, size_t); >> +OVS_RETURNS_NONNULL void *xmemdup(const void *, size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL char *xmemdup0(const char *, size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL char *xstrdup(const char *) MALLOC_LIKE; >> char *nullable_xstrdup(const char *) MALLOC_LIKE; >> bool nullable_string_is_equal(const char *a, const char *b); >> char *xasprintf(const char *format, ...) OVS_PRINTF_FORMAT(1, 2) MALLOC_LIKE; > > xasprintf also never returns NULL. I guess you didn't annotate it > because internally it calls xvasprintf() -> xmalloc(). > > However, xstrdup() and x2nrealloc() also indirectly/directly call > xmalloc() and those were annotated. > > Would it be nicer to just annotate all top level functions that never > return NULL in util.h? You are right I missed xasprintf and xvasprintf, will take another look to make sure I did not miss anything else. Thanks, Eelco > Regards, > Dumitru > >> @@ -177,13 +177,13 @@ void *x2nrealloc(void *p, size_t *n, size_t s); >> >> /* Allocation wrappers for specialized situations where coverage counters >> * cannot be used. */ >> -void *xmalloc__(size_t) MALLOC_LIKE; >> -void *xcalloc__(size_t, size_t) MALLOC_LIKE; >> -void *xzalloc__(size_t) MALLOC_LIKE; >> -void *xrealloc__(void *, size_t); >> +OVS_RETURNS_NONNULL void *xmalloc__(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xcalloc__(size_t, size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xzalloc__(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xrealloc__(void *, size_t); >> >> -void *xmalloc_cacheline(size_t) MALLOC_LIKE; >> -void *xzalloc_cacheline(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xmalloc_cacheline(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xzalloc_cacheline(size_t) MALLOC_LIKE; >> void free_cacheline(void *); >> >> void ovs_strlcpy(char *dst, const char *src, size_t size); >> @@ -191,9 +191,9 @@ void ovs_strzcpy(char *dst, const char *src, size_t size); >> >> int string_ends_with(const char *str, const char *suffix); >> >> -void *xmalloc_pagealign(size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xmalloc_pagealign(size_t) MALLOC_LIKE; >> void free_pagealign(void *); >> -void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; >> +OVS_RETURNS_NONNULL void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; >> void free_size_align(void *); >> >> /* The C standards say that neither the 'dst' nor 'src' argument to
On Thu, Jan 11, 2024 at 05:42:24PM +0100, Eelco Chaudron wrote: > The make clang-analyze target reports an 'Dereference of null > pointer' and an 'Uninitialized argument value' issue due to > it assumes some function can return NULL. > > This patch annotates these functions, so the static analyzer > is aware of this. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > > v2: Accidentally added nullable_xstrdup(), removed it. Thanks Eelco, I ran this with clang-16 and do see a reduction in warnings flagged. Acked-by: Simon Horman <horms@ovn.org>
diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h index 52614a5ac..878c5c6a7 100644 --- a/include/openvswitch/compiler.h +++ b/include/openvswitch/compiler.h @@ -37,6 +37,12 @@ #define OVS_NO_RETURN #endif +#if __GNUC__ && !__CHECKER__ +#define OVS_RETURNS_NONNULL __attribute__((returns_nonnull)) +#else +#define OVS_RETURNS_NONNULL +#endif + #ifndef typeof #define typeof __typeof__ #endif diff --git a/lib/util.h b/lib/util.h index 62801e85f..33e195878 100644 --- a/lib/util.h +++ b/lib/util.h @@ -162,13 +162,13 @@ bool memory_locked(void); OVS_NO_RETURN void out_of_memory(void); /* Allocation wrappers that abort if memory is exhausted. */ -void *xmalloc(size_t) MALLOC_LIKE; -void *xcalloc(size_t, size_t) MALLOC_LIKE; -void *xzalloc(size_t) MALLOC_LIKE; -void *xrealloc(void *, size_t); -void *xmemdup(const void *, size_t) MALLOC_LIKE; -char *xmemdup0(const char *, size_t) MALLOC_LIKE; -char *xstrdup(const char *) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xmalloc(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xcalloc(size_t, size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xzalloc(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xrealloc(void *, size_t); +OVS_RETURNS_NONNULL void *xmemdup(const void *, size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL char *xmemdup0(const char *, size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL char *xstrdup(const char *) MALLOC_LIKE; char *nullable_xstrdup(const char *) MALLOC_LIKE; bool nullable_string_is_equal(const char *a, const char *b); char *xasprintf(const char *format, ...) OVS_PRINTF_FORMAT(1, 2) MALLOC_LIKE; @@ -177,13 +177,13 @@ void *x2nrealloc(void *p, size_t *n, size_t s); /* Allocation wrappers for specialized situations where coverage counters * cannot be used. */ -void *xmalloc__(size_t) MALLOC_LIKE; -void *xcalloc__(size_t, size_t) MALLOC_LIKE; -void *xzalloc__(size_t) MALLOC_LIKE; -void *xrealloc__(void *, size_t); +OVS_RETURNS_NONNULL void *xmalloc__(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xcalloc__(size_t, size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xzalloc__(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xrealloc__(void *, size_t); -void *xmalloc_cacheline(size_t) MALLOC_LIKE; -void *xzalloc_cacheline(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xmalloc_cacheline(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xzalloc_cacheline(size_t) MALLOC_LIKE; void free_cacheline(void *); void ovs_strlcpy(char *dst, const char *src, size_t size); @@ -191,9 +191,9 @@ void ovs_strzcpy(char *dst, const char *src, size_t size); int string_ends_with(const char *str, const char *suffix); -void *xmalloc_pagealign(size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xmalloc_pagealign(size_t) MALLOC_LIKE; void free_pagealign(void *); -void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; +OVS_RETURNS_NONNULL void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; void free_size_align(void *); /* The C standards say that neither the 'dst' nor 'src' argument to
The make clang-analyze target reports an 'Dereference of null pointer' and an 'Uninitialized argument value' issue due to it assumes some function can return NULL. This patch annotates these functions, so the static analyzer is aware of this. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- v2: Accidentally added nullable_xstrdup(), removed it. include/openvswitch/compiler.h | 6 ++++++ lib/util.h | 30 +++++++++++++++--------------- 2 files changed, 21 insertions(+), 15 deletions(-)