On Tue, Dec 18, 2018 at 11:10:34AM -0500, John Ferlan wrote:
>
>
> On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
> > On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
> >> Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair
> >> to perform NULL argument checking; however, no corresponding change
> >> to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the
> >> Coverity build failed. Adjust the prototypes accordingly.
> >
> > This sounds wrong or at least different from the past.
> >
>
> I didn't push yet...
>
> The last time this happened is/was commit 425aac3abf.
>
> Still, before this patch if you build with STATIC_ANALYSIS set you'd see
> the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1
> instead of 0, I get:
>
> util/vircommand.c: In function 'virCommandAddArg':
> util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL
> [-Werror=nonnull-compare]
> if (val == NULL) {
> ^
> util/vircommand.c: In function 'virCommandAddArgPair':
> util/vircommand.c:1604:14: error: nonnull argument 'name' compared to
> NULL [-Werror=nonnull-compare]
> if (name == NULL || val == NULL) {
> ^
> util/vircommand.c:1604:29: error: nonnull argument 'val' compared to
> NULL [-Werror=nonnull-compare]
> if (name == NULL || val == NULL) {
> ^
> Is there a different way you'd prefer this resolved? I could leave it as
> a my coverity environment only, but I would assume clang would be
> impacted too, although I don't use clang.
>
>
> > We have historically still added nonnull annotations even when
> > having checks for NULL in the impl. We had to explicitly disabble
> > the nonnull annotation when building under GCC to prevent it from
> > optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS
> > so that coverity would still report if any callers passed a NULL into
> > the methods.
> >
>
> That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less
> important" because we found that it really only helps if someone calls
> some API with NULL and it does not help if the argument itself is NULL.
> My recollection is always towards removing it once the attribute itself
> was checked in the API. I also have a faint recollection of a discussion
> we've had before on this with the thought towards removing all the
> ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but
> that got to be too many patches and checks in too many places.
This goes way back to this commit:
commit eefb881d4683d50882b43e5b28b0e94657cd0c9c
Author: Laine Stump <laine@xxxxxxxxx>
Date: Wed Apr 25 16:10:01 2012 -0400
build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on
The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin
__attribute__((__nonnull__(m))). The effect of this in gcc is
unfortunately only to make gcc believe that "m" can never possibly be
NULL, *not* to add in any checks to guarantee that it isn't ever NULL
(i.e. it is an optimization aid, *not* something to verify code
correctness.) - see the following gcc bug report for more details:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
Static source analyzers such as clang and coverity apparently can use
ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the
arg really is guaranteed non-NULL), as well as situations where an
obviously NULL arg is given to the function.
https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example
of a bug caused by erroneous application of ATTRIBUTE_NONNULL().
Several people spent a long time staring at this code and not finding
the problem, because the problem wasn't in the function itself, but in
the prototype that specified ATTRIBUTE_NONNULL() for an arg that
actually *wasn't* always non-NULL, and caused a segv when dereferenced
(even though the code that dereferenced the pointer was inside an if()
that checked for a NULL pointer, that code was optimized out by gcc).
There may be some very small gain to be had from the optimizations
that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to
err on the side of generating code that behaves as expected, while
turning on the attribute for static analyzers.
We had methods doing "if (foo == NULL) return 0" and had code that
was (mistakenly) passing in NULL which would have been safe, except
gcc had deleted the "if (foo == NULL)" check. Rather than removing
all attribute(nonnull) checks, we merely disabling it under gcc. We
left it under STATIC_ANALYSIS so that coverity could still report on
places which passed an explicit NULL into a method annotated nonnull.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
