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 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 :|