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

Wed Apr 5 10:38:23 UTC 2017

On Wed, Apr 05, 2017 at 10:06:22AM +0100, 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.
>

How about we enable it by default for build with new enough GCC (so that
it reports these errors without silently dropping the non-NULL checks)?

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

I should've picked a better example.  My point was that if someone wants
to pass NULL to any of our functions, they have to look at its
implementation IMHO.  I don't believe the attributes due to the reason
mentioned before (with people only adding the attribute to functions
only if other nearby functions have it).

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

Sure, but isn't it better to have those in a same patch?  We should
avoid breaking as much as possible.  Every little thing helps.  At least
I'm trying to break as less as possible with every patch, similar to
being able to build and pass all tests between individual patches.

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

Yeah, we can, but that requires a) enabling the attributes for normal
builds and b) educating all libvirt devs what actually the attribute
does (that could come over time side by side with when (a)).

>> 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
>--
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
-------------- 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/ed731322/attachment-0001.sig>

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

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