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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list