[cfe-commits] [PATCH] strnlen checker

[Ted Kremenek]

Looks great.  I think this can go in as is.

I was looking at the test cases, and noticed the following diagnostics:

  +  return strnlen(0, 3); // expected-warning{{Null pointer argument in call to byte string function}}

and

  +  return strnlen((char*)&&label, 3); // expected-warning{{Argument to byte string function is the address of the label 'label', which is not a null-terminated string}}

I know you didn't write these diagnostics, but the term "byte string function" sounds very clinical, and not all that helpful.    It's certainly not a term I'm familiar with (even though I can decipher its meaning).  Do you think we should just say the name of the function instead of an opaque term like "byte string function"?

Do you have commit access?  If not, I'm happy to go ahead and commit this.

On Feb 21, 2011, at 6:49 PM, Lenny Maiorani wrote:

> 
> On Feb 11, 2011, at 9:55 PM, Ted Kremenek wrote:
> 
>> 
>> I think this looks very good, but I have a couple comments:
>> 
>> +    // If the check is for strnlen() then bind the return value to no more than
>> +    // the maxlen value.
>> +    SVal maxlenVal;
>> +    if (IsStrnlen) {
>> +      const Expr *maxlenExpr = CE->getArg(1);
>> +      maxlenVal = state->getSVal(maxlenExpr);
>> +    
>> +      NonLoc * strLengthNL = dyn_cast<NonLoc>(&strLength);
>> +      NonLoc * maxlenValNL = dyn_cast<NonLoc>(&maxlenVal);
>> +
>> +      QualType cmpTy = C.getSValBuilder().getContext().IntTy;
>> +      const GRState *stateTrue, *stateFalse;
>> +    
>> +      // Check if the strLength is greater than or equal to the maxlen
>> +      llvm::tie(stateTrue, stateFalse) =
>> +        state->assume(cast<DefinedOrUnknownSVal>
>> +                      (C.getSValBuilder().evalBinOpNN(state, BO_GE, 
>> +                                                      *strLengthNL, *maxlenValNL,
>> +                                                      cmpTy)));
>> +
>> +      // If the strLength is greater than or equal to the maxlen, set strLength
>> +      // to maxlen
>> +      if (stateTrue && !stateFalse) {
>> +        strLength = maxlenVal;
>> +      }
>> +    }
>> +
>> 
>> Is there a reason 'maxlen' is declared outside if the 'if' block?  It's not used elsewhere.  Why not just write:
>> 
>> +    if (IsStrnlen) {
>> +      const Expr *maxlenExpr = CE->getArg(1);
>> +      SVal maxlenVal = state->getSVal(maxlenExpr);
>> +    
>> 
>> I strongly prefer this style because it keeps the lifetime of variables associated with the logic they are intended to be paired with, and no more.  By glancing at the code, I couldn't tell if maxlenVal was use elsewhere unless I searched for it.
>> 
>> Other minor nits:
>> 
>> +      NonLoc * strLengthNL = dyn_cast<NonLoc>(&strLength);
>> +      NonLoc * maxlenValNL = dyn_cast<NonLoc>(&maxlenVal);
>> 
>> please write:
>> 
>> NonLoc *strLengthNL
>> 
>> instead of:
>> 
>> NonLoc * strLengthNL
>> 
>> That matches the coding style of the majority of the codebase (which is gradually moving in this direction).
> 
> Attached is an updated version of the strnlen() checker. With the two changes you suggested, Ted. Please review.
> 
> I think the SVal declaration was leftover due to some other changes I was making and somehow didn't get rid of it. As for where the '*' goes... noted. Thanks for the input.
> 
> -Lenny
> <strnlen-checker.diff>
> 
> 
> --
>       __o
>     _`\<,_
>    (*)/ (*)
> ~~~~~~~~~~~~~~~~~~~~
>