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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list