[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Tue, Dec 18, 2018 at 02:31:27PM -0500, John Ferlan wrote:


On 12/18/18 11:16 AM, Daniel P. Berrangé wrote:
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 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


And Eric's comment from Apr 2012 eventually answered in Sept 2015 would
seem to indicate to me the exact thing that happened in this code and
why removing the ATTRIBUTE_NONNULL from the header file would be
necessary.

Maybe I'm reading it wrong, but it appears to say that if
ATTRIBUTE_NONNULL was provided in the prototype and the code did a
comparison of the variable to NULL, then some sort of warning would be
issued. And that's what I see in the code provided. I really only
skimmed and didn't spend too many cycles thinking about it though.


    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.



Um, OK. Well, I've been removing ATTRIBUTE_NONNULL when encountering
this same error in the past. It just seemed to be the path of least
resistance. After all now regardless of whether its "bar(NULL)" or
"bar(foo)" where foo==NULL, if the code in bar does "if (arg1 == NULL)
xxx;" then we're covered.

In any case, I'll just leave this change in my local Coverity tree and
drop this patch.


What if we also disable the nonnull-compare in case of STATIC_ANALYSIS?  That
would make perfect sense if what Daniel said is the desired thing to do.

However maybe Daniel misread what is written in the commit message.  The fact
that these functions now _do_ handle NULL, the argument _can_ be NULL, so the
attributes should _not_ be there.

The scenario only gets weird because the way the NULL is handled here is a call
to abort(), which should not be in our code given the way we are handling errors
usually.  Maybe instead the cmd->error should be set instead.  Maybe it should
not be there.  Maybe I'm completely off track.

John

--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]