[cfe-commits] [PATCH] strnlen checker

[Ted Kremenek]

Hi Lenny,

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).

Other than that, looks great!


On Feb 7, 2011, at 12:06 PM, Lenny Maiorani wrote:

> The attached patch is available for review/inclusion.
> 
> 
> This patch adds support for checking strnlen() in the Static Analyzer by extending and common-izing the strlen() checker. 
> 
> The checker first computes the length of the string, then compares that to the value of the maxlen arg. If the maxlen is less than the length of the string the maxlen value becomes the bind value instead of the length of the string.
> 
> -Lenny
> 
> <strnlen-checker.diff>
> 
> 
>       __o
>     _`\<,_
>    (*)/ (*)
> ~~~~~~~~~~~~~~~~~~~~
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits