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.