On Tue, Jul 07, 2020 at 04:14:40AM +0200, Philippe Mathieu-Daudé wrote: > On 7/7/20 3:14 AM, Richard Henderson wrote: > > On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote: > >> Coverity noticed commit 950c4e6c94 introduced a dereference before > >> null check in get_opt_value (CID1391003): > >> > >> In get_opt_value: All paths that lead to this null pointer > >> comparison already dereference the pointer earlier (CWE-476) > >> > >> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit > >> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL > >> for the 'value' parameter". > >> > >> Since this function is publicly exposed, it risks new users to do > >> the same error again. Avoid that documenting the 'value' argument > >> must not be NULL. > > > > I think we should also add some use of __attribute__((nonnull(...))) to > > enforce > > this within the compiler. > > > > I recently did this without a qemu/compiler.h QEMU_FOO wrapper within > > target/arm. But the nonnull option has optional arguments, so it might be > > difficult to wrap in macros. > > I have this patch after your suggestion from last year: > > +#if __has_attribute(nonnull) > +# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST)))) > +#else > +# define QEMU_NONNULL(LIST) > +#endif
The if/else branch is not required, as both clang and gcc support this, and they are our only supported compilers. Beware that __attribute__((nonnul)) has side-effects, as it was originally implemented as a hint for the optimizer. It allows it to eliminate any code in the method that does a comparison to NULL. Historically it only generated warning messages in very few scenarios involving a literal NULL. Only more recently with -fanalyzer can it generate warnings about indirect passing of NULL via variables. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
