[cfe-commits] [PATCH] strnlen checker

[Ted Kremenek]

Committed: r126187

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
>     _`\<,_
>    (*)/ (*)
> ~~~~~~~~~~~~~~~~~~~~
>