[cfe-dev] [clang-tidy] a simple strncmp checker

[Daniel Marjamäki via cfe-dev]

Hello Bob!

Thanks for sharing this idea. It's interesting.

as far as I understand there is no big danger here is there? It's just that using strcmp instead would be cleaner? Or did you spot any case with unwanted behaviour at work with this checker?

If you would prefer to check for dangerous code then I believe strncpy, strncat etc are more interesting. strncpy doesn't always 0-terminate the string. strncat(x,y,10) can generate a string that is longer than 10 bytes.


> I saw a presentation from someone using another static analysis tool (
> http://www.slideshare.net/Andrey_Karpov/200-open-sourceprojectslater) and
> in that presentation they mentioned finding a relatively simple error with
> strncmp.  They searched for calls to strncmp where one or both of the
> string parameters were shorter than the limit provided.  That is, we have
> something like:
> 
>     bar = strncmp(foo, "asdf", 6);

You don't say how you check that foo is shorter than 6 bytes.

Imho, if foo is a buffer with size <6 you can warn.

> 1)  Is this type of checking sound?  If so, should I put it up for review?

It is not strictly sound. But no checking is strictly sound. For instance, as far as I know you won't warn about such code:

    #ifndef __clang__
    strncmp(foo, "asdf", 6);
    #endif

It is not an issue that it's not sound. I don't care. Please put it up for review.

> 2)  In the above example, would strncmp(foo, "asdf", 5) be an error?  That
> is, should we include the null byte at the end of the string?

It depends on exactly what the warning message says. If it still applies then yes.

For instance if the warning message says "you can safely replace strncmp with strcmp" then I'd still write the warning.

> 3) One possible valid use case of strncmp is to do prefix checking.  In
> that case the limit n would be less than one or both of the strings.
> However, in my completely unscientific sample at work I saw what appear to
> be copy-paste errors where the string was changed but not the size with the
> resulting strncmp only checking a prefix of the given string.  Should we
> warn in these cases as well?

As I understand, I would not. It might be foo that contains the prefix.


> 4)  My clang-tidy checker is implemented with ASTMatchers for very
> simplistic checking.  Would it be better to implement this for
> context-sensitive checking?  If so, can someone point me to an existing
> checker to study?

I personally recommend that you keep it simple. And use ASTMatchers.

I think it would be interesting to see what false positives there will be with the simple checking to start with.

Since I believe this is a stylistic message, context sensitive analysis might just create lots of noise. For instance:

    const char *foo = "foo";
#ifndef __clang__
    foo = "asdfghjkl";
#endif
     bar = strncmp(foo, "asdf", 6);  // <- Warning here is FP

We only want to warn if foo is always <6 bytes.

Best regards,
Daniel Marjamäki


..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se