[libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"

[Daniel P. Berrange]

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