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

[Jim Meyering]

Jim Paris wrote:
> 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

That's true, in a way.

> will even make it more likely since non-NULL checks will be skipped.

Once a parameter is marked nonnull, tools can do a better job
of spotting *callers* that might mistakenly pass a corresponding NULL
argument.  Without nonnull, the compiler could only detect a problem
*inside* the function, in the case that we lack adequate protection
on a dereferencing expression.

For external functions, it's a policy decision.  If you say the
function is defined only for non-NULL pointers, then you may as well
add an assert(ptr != NULL) and __attribute__((nonnull...)).  Otherwise,
we must perform the test at run-time.  Note that memcpy, strcpy, unlink,
stat, etc. have parameters that can be marked with the nonnull attribute.
That's a plus, because compiler/analyzer tools can then warn callers
about misuse, and they needn't be burdened with detecting and diagnosing
NULL pointers.

Using attribute-nonnull is not an excuse for skipping a "ptr == NULL"
test.  Rather, it is a way to tell the compiler that we require
every caller to ensure that a "ptr" parameter is non-NULL.

This has been true of most internal "conn" parameters for some time.
There were even quite a few places in the code where "conn" would be
dereferenced without first ensuring it is non-NULL, but there was no
way to trigger the NULL-deref in those seemingly-erroneous bits of code,
because upstream tests ensured non-NULL conn.

I spent time adding guards, as I would find those disturbing bits
of code.  I now think that I should have been adding assertions and/or
attribute-nonnull directives, instead.

Adding an "assert (ptr != NULL);" is tempting, because then we'd get
a pretty diagnostic with filename:lineno rather than just a segfault,
whenever this assumption is violated.  However, adding a raw "assert"
use in library grade code is frowned upon, to say the least.