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

[Jim Paris]

Daniel P. Berrange wrote:
> On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
> > Daniel Veillard wrote:
> > > On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
> > >> Here's the test just before the else-if in the patch below:
> > >>
> > >>     if (conn &&
> > >>         conn->driver &&
> > >>         STREQ (conn->driver->name, "remote")) {
> > >>
> > >> So, in the else-branch, "conn" is guaranteed to be NULL.
> > >> And dereferenced.
> > >>
> > >> This may be only a theoretical risk, but if so,
> > >> the test of "conn" above should be changed to an assertion,
> > >> and/or the parameter should get the nonnull attribute.
> > >
> > >   I'm fine with your patch, so that even if code around changes
> > > having a gard is IMHO a good thing
> > 
> > Thanks for the quick review.
> > Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
> > so I've just adjusted (caveat, still not tested or even compiled)
> 
> 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.
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
$

-jim