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

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(a)laine.org&gt; 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.