Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"
On Wed, Sep 02, 2009 at 11:48:21AM -0400, Jim Paris wrote:
>
> Yeah, for functions where it is expected that the passed in param
> be non-NULL, then annotations are definitely the way togo. This
> lets the compiler/checkers validate the callers instead, avoiding
> the need to clutter the methods with irrelevant NULL checks.
Does __attribute__((__nonnull__())) really cover the case we're
concerned about here? As I understand it, it tells the compiler
1) if the caller obviously passes NULL, emit a warning
2) assume that the arguments are non-NULL and optimize away
In other words it will do nothing to prevent a null dereference and
will even make it more likely since non-NULL checks will be skipped.
We should only add the nonnull attribute annotation on methods
where we are not intending to do any checks for non-NULL. They
would already be segfaulting if passed a NULL pointer, so adding
the annotation gives us the static validation of the existing
non-NULL assumption.
For example:
test.c:
--------
#include <stdio.h>
#include <stdlib.h>
void foo(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
__attribute__((__nonnull__ (1)))
void bar(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
int main(void)
{
foo(malloc(1000000000000ULL));
bar(malloc(1000000000000ULL));
return 0;
}
--------
$ gcc -O2 -Wall -Werror -o test test.c
$ ./test
(null)
Segmentation fault
THis is not a case where we would add a non-null annotation - the method
is clearly expecting a NULL parameter as valid since it has a check &
branch to handle that. So using the annotation is not correct.
The prime example of usefulness in libvirt is in the main API glue
- In include/libvirt.h most of the parameters are documented to
be non-NULL. We should *not*, however, annotate libvirt.h since
the implmentation of these APIs is including checks for NULL
and returns a runtime error to the caller in this case. We
don't want the compiler to optimize away these checks
- In the src/driver.h, and the implementations of these driver
APIs eg qemu_driver.c, xen_unified.c, we *should* include
the non-NULL annotation. These methods are relying on the
fact that libvirt.c has already weeded out any NULL pointers
and so don't do any NULL checks themselves.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|