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

[John Ferlan]

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 at redhat.com>
>>>
>>>
>>>> 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
>