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