2:51 p.m.
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.
that a null pointer is passed in an argument slot marked as
non-null, and the '-Wnonnull' option is enabled, a warning is
issued. The compiler may also choose to make optimizations based
on the knowledge that certain function arguments will never be
null.
[/quote]
IIRC, the problem we had was that we marked some methods as non-null,
but still passed NULL values into them. The compiler had optimized
away the code paths that if 'if (arg == NULL) { return 0 }', meaning
the later '*arg' de-reference would fail.
Yes, sure, this corresponds to the last sentence of the documentation
for the attribute.
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).
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
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().
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.
> 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>
> 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.
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/ :|