Refine checking and diagnostics for use of floating point variable as a counter.

This implements <rdar://problem/6336718> and checks for CERT secure coding
advisory FLP30-C.

llvm-svn: 76900

clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp

@@ -15,6 +15,7 @@
 #include "clang/Analysis/LocalCheckers.h"
 #include "clang/AST/StmtVisitor.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 
@@ -25,15 +26,13 @@ public:
   WalkAST(BugReporter &br) : BR(br) {}
   
   // Statement visitor methods.
-  void VisitDoStmt(DoStmt *S);
-  void VisitWhileStmt(WhileStmt *S);
   void VisitForStmt(ForStmt *S);
-
-  void VisitChildren(Stmt *S);
   void VisitStmt(Stmt *S) { VisitChildren(S); }
 
+  void VisitChildren(Stmt *S);
+  
   // Checker-specific methods.
-  void CheckLoopConditionForFloat(Stmt *Loop, Expr *Condition); 
+  void CheckLoopConditionForFloat(const ForStmt *FS);
 };
 } // end anonymous namespace
 
@@ -47,69 +46,117 @@ void WalkAST::VisitChildren(Stmt *S) {
       Visit(child);
 }
 
-void WalkAST::VisitDoStmt(DoStmt *S) {
-  CheckLoopConditionForFloat(S, S->getCond());
-  VisitChildren(S);
-}
+void WalkAST::VisitForStmt(ForStmt *FS) {
+  CheckLoopConditionForFloat(FS);  
 
-void WalkAST::VisitForStmt(ForStmt *S) {
-  if (Expr *Cond = S->getCond())  
-    CheckLoopConditionForFloat(S, Cond);  
-
-  VisitChildren(S);
-}
-
-void WalkAST::VisitWhileStmt(WhileStmt *S) {
-  CheckLoopConditionForFloat(S, S->getCond());
-  VisitChildren(S);
+  // Recurse and check children.
+  VisitChildren(FS);
 }
 
 //===----------------------------------------------------------------------===//
-// Checking logic.
+// Check: floating poing variable used as loop counter.
 //===----------------------------------------------------------------------===//
 
-static Expr* IsFloatCondition(Expr *Condition) {  
-  while (Condition) {
-    Condition = Condition->IgnoreParenCasts();
+static const DeclRefExpr*
+GetIncrementedVar(const Expr *expr, const VarDecl *x, const VarDecl *y) {
+  expr = expr->IgnoreParenCasts();
   
-    if (Condition->getType()->isFloatingType())
-      return Condition;
+  if (const BinaryOperator *B = dyn_cast<BinaryOperator>(expr)) {      
+    if (!(B->isAssignmentOp() || B->isCompoundAssignmentOp() ||
+          B->getOpcode() == BinaryOperator::Comma))
+      return NULL;
       
-    switch (Condition->getStmtClass()) {
-      case Stmt::BinaryOperatorClass: {
-        BinaryOperator *B = cast<BinaryOperator>(Condition);
+    if (const DeclRefExpr *lhs = GetIncrementedVar(B->getLHS(), x, y))
+      return lhs;
       
-        Expr *LHS = B->getLHS();
-        if (LHS->getType()->isFloatingType())
-          return LHS;
-
-        Expr *RHS = B->getRHS();
-        if (RHS->getType()->isFloatingType())
-          return RHS;
+    if (const DeclRefExpr *rhs = GetIncrementedVar(B->getRHS(), x, y))
+      return rhs;
       
     return NULL;
   }
-      case Stmt::UnaryOperatorClass: {
-        UnaryOperator *U = cast<UnaryOperator>(Condition);
-        if (U->isArithmeticOp()) {
-          Condition = U->getSubExpr();
-          continue;
-        }
-        return NULL;
-      }
-      default:
-        break;
-    }
+    
+  if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(expr)) {
+    const NamedDecl *ND = DR->getDecl();
+    return ND == x || ND == y ? DR : NULL;
   }
+   
+  if (const UnaryOperator *U = dyn_cast<UnaryOperator>(expr))
+    return U->isIncrementDecrementOp()
+      ? GetIncrementedVar(U->getSubExpr(), x, y) : NULL;
+
   return NULL;
 }
 
-void WalkAST::CheckLoopConditionForFloat(Stmt *Loop, Expr *Condition) {
-  if ((Condition = IsFloatCondition(Condition))) {
-    const char *bugType = "Floating point value used in loop condition";
-    SourceRange R = Condition->getSourceRange();    
-    BR.EmitBasicReport(bugType, "Security", bugType, Loop->getLocStart(),&R, 1);
-  }
+/// CheckLoopConditionForFloat - This check looks for 'for' statements that
+///  use a floating point variable as a loop counter.
+///  CERT: FLP30-C, FLP30-CPP.
+///
+void WalkAST::CheckLoopConditionForFloat(const ForStmt *FS) {
+  // Does the loop have a condition?
+  const Expr *condition = FS->getCond();
+  
+  if (!condition)
+    return;
+
+  // Does the loop have an increment?
+  const Expr *increment = FS->getInc();
+  
+  if (!increment)
+    return;
+    
+  // Strip away '()' and casts.
+  condition = condition->IgnoreParenCasts();
+  increment = increment->IgnoreParenCasts();
+  
+  // Is the loop condition a comparison?
+  const BinaryOperator *B = dyn_cast<BinaryOperator>(condition);
+
+  if (!B)
+    return;
+  
+  // The actual error condition.
+  if (!((B->isRelationalOp() || B->isEqualityOp()) &&
+        ((B->getLHS()->getType()->isFloatingType() ||
+          B->getRHS()->getType()->isFloatingType()))))
+    return;
+  
+  // Are we comparing variables?
+  const DeclRefExpr *drLHS = dyn_cast<DeclRefExpr>(B->getLHS()->IgnoreParens());
+  const DeclRefExpr *drRHS = dyn_cast<DeclRefExpr>(B->getRHS()->IgnoreParens());
+  
+  if (!drLHS && !drRHS)
+    return;
+
+  const VarDecl *vdLHS = drLHS ? dyn_cast<VarDecl>(drLHS->getDecl()) : NULL;
+  const VarDecl *vdRHS = drRHS ? dyn_cast<VarDecl>(drRHS->getDecl()) : NULL;
+  
+  if (!vdLHS && !vdRHS)
+    return;  
+  
+  // Does either variable appear in increment?
+  const DeclRefExpr *drInc = GetIncrementedVar(increment, vdLHS, vdRHS);
+  
+  if (!drInc)
+    return;
+  
+  // Emit the error.  First figure out which DeclRefExpr in the condition
+  // referenced the compared variable.
+  const DeclRefExpr *drCond = vdLHS == drInc->getDecl() ? drLHS : drRHS;
+
+  llvm::SmallVector<SourceRange, 2> ranges;  
+  std::string sbuf;
+  llvm::raw_string_ostream os(sbuf);
+  
+  os << "Variable '" << drCond->getDecl()->getNameAsCString()
+     << "' with floating point type '" << drCond->getType().getAsString()
+     << "' should not be used as a loop counter";
+
+  ranges.push_back(drCond->getSourceRange());
+  ranges.push_back(drInc->getSourceRange());
+  
+  const char *bugType = "Floating point variable used as loop counter";
+  BR.EmitBasicReport(bugType, "Security", os.str().c_str(),
+                     FS->getLocStart(), ranges.data(), ranges.size());
 }
 
 //===----------------------------------------------------------------------===//

clang/test/Analysis/security-syntax-checks.m

@@ -0,0 +1,23 @@
+// RUN: clang-cc -analyze -warn-security-syntactic %s -verify
+
+// <rdar://problem/6336718> rule request: floating point used as loop 
+//  condition (FLP30-C, FLP-30-CPP)
+//
+// For reference: https://www.securecoding.cert.org/confluence/display/seccode/FLP30-C.+Do+not+use+floating+point+variables+as+loop+counters
+//
+void test_float_condition() {
+  for (float x = 0.1f; x <= 1.0f; x += 0.1f) {} // expected-warning{{Variable 'x' with floating point type 'float'}}
+  for (float x = 100000001.0f; x <= 100000010.0f; x += 1.0f) {} // expected-warning{{Variable 'x' with floating point type 'float'}}
+  for (float x = 100000001.0f; x <= 100000010.0f; x++ ) {} // expected-warning{{Variable 'x' with floating point type 'float'}}
+  for (double x = 100000001.0; x <= 100000010.0; x++ ) {} // expected-warning{{Variable 'x' with floating point type 'double'}}
+  for (double x = 100000001.0; ((x)) <= 100000010.0; ((x))++ ) {} // expected-warning{{Variable 'x' with floating point type 'double'}}
+  
+  for (double x = 100000001.0; 100000010.0 >= x; x = x + 1.0 ) {} // expected-warning{{Variable 'x' with floating point type 'double'}}
+  
+  int i = 0;
+  for (double x = 100000001.0; ((x)) <= 100000010.0; ((x))++, ++i ) {} // expected-warning{{Variable 'x' with floating point type 'double'}}
+  
+  typedef float FooType;
+  for (FooType x = 100000001.0f; x <= 100000010.0f; x++ ) {} // expected-warning{{Variable 'x' with floating point type 'FooType'}}
+}
+