[libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]

[Daniel P. Berrangé]

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 at laine.org>
  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 :|