Re: [libvirt] [PATCH (RFC?)] Remove usage of __attribute__((nonnull))

On 04/05/2017 05:06 AM, Daniel P. Berrange wrote: > On Tue, Apr 04, 2017 at 09:51:47PM +0200, Martin Kletzander wrote: >> On Tue, Apr 04, 2017 at 04:10:59PM +0100, Daniel P. Berrange wrote: >>> On Tue, Mar 28, 2017 at 01:46:31PM +0200, Martin Kletzander wrote: >>>> The attribute (defined as ATTRIBUTE_NONNULL) was added long time >>>> ago (2009), but in 2012 (commit eefb881d4683) it was disabled for >>>> normal build, making it used only when coverity was building libvirt >>>> or on special request. It was disabled because it was being misused >>>> and misunderstood -- the attribute is there as an optimization hint >>>> for the compiler, not to enhance static analysis. >>> >>> Actually the attribute does both and the primary intention of the attribute >>> *is* build time warnings and/or static analysis warnings: >>> >>> [quote] >>> 'nonnull (ARG-INDEX, ...)' >>> The 'nonnull' attribute specifies that some function parameters >>> should be non-null pointers. For instance, the declaration: >>> >>> extern void * >>> my_memcpy (void *dest, const void *src, size_t len) >>> __attribute__((nonnull (1, 2))); >>> >>> causes the compiler to check that, in calls to 'my_memcpy', >>> arguments DEST and SRC are non-null. If the compiler determines >> >> This, however, happens only if we pass NULL directly. There's not much >> of a deeper analysis involved IIRC. > > Yep, coverity is needed to do most deeper analysis of NULL handling > >>> The use of nonnull attribute would help the compiler report >>> mistakes, but the compiler only catches some easy cases at build >>> time. >>> >> >> It would. But for now it is only enabled if STATIC_ANALYSIS=1 and that >> is only set if you are running in coverity (or explicitly specify that >> during configure, which almost nobody does). > > Easily solvable by enabling it in CI. > >>> So the key issue is whether we have enough confidence in fact that >>> our calling methods really will be passing a non-NULL value or not. >>> >> >> Well, I definitely don't. But from what I see on the list, people only >> add these attributes to function declarations if they are added (read: >> copy-pasted) near other functions that have this attribute. That is >> what sparked this attribute removal idea. So there are *lot of* >> functions that don't fail gracefully on NULL parameters because nobody >> added (or fixed) the attributes when modifying the code. >> >>> If we're not confident in our callers for a method, then we should >>> remove nonnull, and have an explicit 'if (arg == NULL)' check in >>> the code instead. >>> >> >> We already behave like that. Except John (and me for a while), everyone >> has ATTRIBUTE_NONNULL defined as nothing. And I believe no distribution >> is defining STATIC_ANALYSIS when building their packages. However that >> leads to unnecessary late removals of ATTRIBUTE_NONNULL specifications > g> for some functions later on, which in turn might cause problems with >> backports and from my point of view it just causes mess and unnecessary >> time spent on it. What's worse, if you decide to compile with the >> static analysis turned on, you don't get a check for the fact that all >> devices are handled in virDomainDefCheckABIStabilityFlags() and >> virDomainDeviceInfoIterateInternal(). > > I don't see the virDomainDefCheckABIStabilityFlags thing as a real problem. That code is > there to check for some silly mistakes at build time, so it is not something that needs to > be enable by everyone. It really just needs to be run once by someone/something - IOW as > long as our CI triggers that codepath the sanity check is serving its purpose. > >> >>> Some projects might use asserts() against args being non-NULL, but >>> libvirt tends to try to return soft errors. Given this, it could be >>> better use to 'if (arg == NULL)' checks in general, over nonnull, >>> unless we have high confidence in callers. >>> >>>> However, it is used until today in many places and since it is >>>> disabled by default, it just screws up builds with STATIC_ANALYSIS >>>> enabled. >>> >>> Screws them up in what way ? >>> >> >> As said before, the warnings are found out only by some, so it needs to >> be fixed in a separate patch and so on. > > I don't see that as a real problem. It is no different situation to fact that someone writes > and patch on Linux and doesn't test Windows or BSD, or a different older version of Linux, > etc. We never expect developers to test all situations. The key is to ensure that we have > CI coverage of the option. I thought that John still had a nightly coverity job running that > would trigger the -DSTATIC_ANALYSIS codepaths. If that's not the case, then we'd wnat to look > at enabling that in one of the centos CI jobs. > I still have a run of Coverity every night although I have been less diligent about checking errors lately. Mainly because generally things that are considered a false positive were being rejected when I posted patches. I keep about 20 patches in a local branch. There was a point quite a few months ago where my nightly build started failing because either I changed the Coverity version or the compiler version on my laptop - cannot recall exactly. That resulted in a bunch more local patches until I finally had too many and posted that pile late last month. Enabling static analysis in CI builds was something I suggested in my cover - although now I've done it for my every day work environment so I will see the problems much sooner.

John >> >>>> Remove that attribute and all its usages. >>> >>> Do we not use '-DSTATIC_ANLYSIS' when running through coverity, so >>> that coverity sees the nonnull annotations and thus is able to do >>> more advanced checks for NULL args ? If so, removing it would make >>> coverity miss bugs. >>> >> >> STATIC_ANALYSIS is automatically set if configure realizes it's running >> under coverity. However we don't run coverity that often. More often >> we run some build with STATIC_ANALYSIS turned on explicitly. >> >>>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >>> >>> >>>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c >>>> index 891238bcbe0d..5cd3fa4ebccb 100644 >>>> --- a/daemon/libvirtd.c >>>> +++ b/daemon/libvirtd.c >>>> @@ -410,7 +410,7 @@ static void daemonInitialize(void) >>>> #undef VIR_DAEMON_LOAD_MODULE >>>> >>>> >>>> -static int ATTRIBUTE_NONNULL(3) >>>> +static int >>>> daemonSetupNetworking(virNetServerPtr srv, >>>> virNetServerPtr srvAdm, >>>> struct daemonConfig *config, >>> >>> If we do decide to remove ATTRIBUTE_NONNULL annotations, then we need to >>> make sure the method impls have suitable 'if (args == NULL) ....' checks >>> that handle this safely. >>> >> >> As said above, I believe there are many functions that do not use >> ATTRIBUTE_NONNULL and would segfault on a NULL argument. Let's see how >> long it takes me to find one. >> >> ... (10 seconds later) >> >> Look, the one I mentioned (virDomainDefCheckABIStabilityFlags) is one of >> them. It dereferences both pointers immediately. I bet there are lot >> of those. It's good that most of them would be caught in tests if it >> was obvious. > > Sure, and adding / removing ATTRIBUTE_NONNULL doesn't really make any > difference to whether that is a bug or not. ie if we do have some code > somewhere that could cause NULL to be passed, then we have a bug regardless > of whether we add ATTRIBUTE_NONNULL annotation to that method or not. If we > did add the annotation though, we might get a notification of the bug from > coverity. So from this POV, ATTRIBUTE_NONNULL feels beneficial to me and > rather than removing it, we should instead make an effort to look for any > methods which defererence pointer args and add more ATTRIBUTE_NONNULL > annotations to them. > > In fact we could go as far to say that every single method which takes a > pointer arg should have ATTRIBUTE_NONNULL unless there's a clear valid > use case for it to accept NULL. > >> So the other thing to do would be to ditch commit eefb881d4683 and the >> whole STATIC_ANALYSIS and just add a comment for making coverity happy >> on those two aforementioned occasions. > > > > Regards, > Daniel > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list