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

[Martin Kletzander]

On Wed, Apr 05, 2017 at 06:53:10AM -0400, John Ferlan wrote:
>
>
>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.
>

Yeah, but my points were: a) why only CI? b) we need not to enable
static_analysis flag, but rather not define ATTRIBUTE_NONNULL to nothing
if STATIC_ANALYSIS is set.  The STATIC_ANALYSIS macro is used to disable
some code that we want to be using in both CI and local builds.

>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
>>
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170405/d00e799c/attachment-0001.sig>