On Mon, Oct 26, 2009 at 12:13 PM, Daniel Stenberg <daniel_at_haxx.se> wrote:
> On Mon, 26 Oct 2009, Jakub Hrozek wrote:
>
> I'm aware that with srv_out == NULL you get a crash -- I'm still not sure
>> which way of coding is preferred within c-ares, be it the more defensive one
>> of ares_parse_{a,aaaa}_reply or the one of ptr/srv. If a very defensive way
>> of checking is preferred, I think the parsing function should just return
>> something like ARES_EINVAL or similar..
>>
>
> Personally I tend to prefer a "strict usage" where the arguments are not
> optional (ie they must not be NULL) and the function implementations can
> assume that they are set correctly as per the documentation. assert()s etc
> could be used to verify for debug-builds.
>
> But I'm open for what others think. If we have documented these functions
> to allow NULL for not storing any info, then we should of course check them.
> We should be consistent among all the functions that are so similar
> methinks.
I discovered this problem when running a clang (http://clang.llvm.org/)
static analysis pass. There were a few other warnings, but they were either
false positives or inconsequential (ie, value assigned to a variable is not
used after the assignment). The error I originally reported was the only
"NULL dereference" warning detected. I don't know c-ares that well so it's
hard to say why ares_parse_srv_reply.c was the only 'hit'. For example,
assuming that assert() wasn't #defined away, clang won't flag potential NULL
dereferences if an assert() catches NULL a pointer because clang knows that
the assertion will result in a call to abort(), terminating execution.
My $0.02: I don't mind "strict usage" API's, but it should definitely be
consistent throughout the API and documented that an argument must be a
valid, non-NULL pointer, and anything else will result in "undefined
behavior".
Sometimes, however, I do like API's that accept NULL and take it to mean
that that particular piece of information "isn't needed.". An example that
comes to mind is modf(), which is defined as:
double modf(double value, double *iptr);
modf() requires that iptr be non-NULL, but the last time I needed modf()
(which is why it comes to mind), I didn't need the extra information
returned via iptr, I just needed the value that was returned by the
function. It would have been much more convenient if it had accepted a
NULL.
I haven't looked at or used ares_parse_srv_reply() to know if it "makes
sense" to have the arguments in question be NULL. If it contextually makes
sense such that a function is still 'useful', and the pointer argument could
be set to NULL to signal that that particular piece of info isn't needed,
then I think it's worth the "effort" to robustly support NULL pointers. If,
on the other hand, it simply doesn't make any sense to have the arguments in
question be NULL, then I think any checks should be done via an assert()
macro. But that's just my $0.02 :).
Also worth considering is gcc's __attribute__((nonnull)) attribute. I don't
know if c-ares already has such machinery in place, but here's an example of
what I've used in another project:
// __attribute__ is supported by gcc versions before 4, I just happened to
need it to be >= 4 for my code...
#if defined (__GNUC__) && (__GNUC__ >= 4)
#define RKL_ATTRIBUTES(attr, ...) __attribute__((attr,
##__VA_ARGS__))
#else // defined (__GNUC__) && (__GNUC__ >= 4)
#define RKL_ATTRIBUTES(attr, ...)
#endif // defined (__GNUC__) && (__GNUC__ >= 4)
#define RKL_STATIC_INLINE static __inline__
RKL_ATTRIBUTES(always_inline)
#define RKL_UNUSED_ARG
RKL_ATTRIBUTES(unused)
#define RKL_NONNULL_ARGS(arg, ...)
RKL_ATTRIBUTES(nonnull(arg, ##__VA_ARGS__))
#define RKL_NONNULL_ARGS_WARN_UNUSED(arg, ...)
RKL_ATTRIBUTES(warn_unused_result, nonnull(arg, ##__VA_ARGS__))
#if defined (__GNUC__) && (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 3)
#define RKL_ALLOC_SIZE_NON_NULL_ARGS_WARN_UNUSED(as, nn, ...)
RKL_ATTRIBUTES(warn_unused_result, nonnull(nn, ##__VA_ARGS__),
alloc_size(as))
#else // defined (__GNUC__) && (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 3)
#define RKL_ALLOC_SIZE_NON_NULL_ARGS_WARN_UNUSED(as, nn, ...)
RKL_ATTRIBUTES(warn_unused_result, nonnull(nn, ##__VA_ARGS__))
#endif // defined (__GNUC__) && (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 3)
Examples of their usage:
static void rkl_handleDelayedAssert(id self, SEL _cmd, id exception)
RKL_NONNULL_ARGS(1,2,3);
static NSUInteger rkl_search(RKLCacheSlot *cacheSlot, NSRange *searchRange,
NSUInteger updateSearchRange, id *exception RKL_UNUSED_ARG, int32_t *status)
RKL_NONNULL_ARGS_WARN_UNUSED(1,2,4,5);
RKL_STATIC_INLINE void *rkl_realloc(void **ptr, size_t size, NSUInteger
flags) RKL_ALLOC_SIZE_NON_NULL_ARGS_WARN_UNUSED(2,1);
Another advantage to using nonnull is that -Wnonnull (which is included in
-Wall) will warn you if the compiler can determine if a NULL value will be
passed.
Received on 2009-10-26