Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"
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