[cfe-commits] [PATCH] strnlen checker

[Lenny Maiorani]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: strnlen-checker.diff
Type: application/octet-stream
Size: 7077 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110221/9b5f13a0/attachment.obj>
-------------- next part --------------



--
       __o
     _`\<,_
    (*)/ (*)
~~~~~~~~~~~~~~~~~~~~