[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function |
Date: |
Tue, 7 Jul 2020 09:38:35 +0100 |
User-agent: |
Mutt/1.14.3 (2020-06-14) |
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 :|