diff --git a/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp b/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp index f29a8cc3b19e..acd2373effe9 100644 --- a/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp +++ b/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 VisitStmt(Stmt *S) { VisitChildren(S); } void VisitChildren(Stmt *S); - void VisitStmt(Stmt *S) { VisitChildren(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(); - - if (Condition->getType()->isFloatingType()) - return Condition; - - switch (Condition->getStmtClass()) { - case Stmt::BinaryOperatorClass: { - BinaryOperator *B = cast(Condition); - - Expr *LHS = B->getLHS(); - if (LHS->getType()->isFloatingType()) - return LHS; - - Expr *RHS = B->getRHS(); - if (RHS->getType()->isFloatingType()) - return RHS; - - return NULL; - } - case Stmt::UnaryOperatorClass: { - UnaryOperator *U = cast(Condition); - if (U->isArithmeticOp()) { - Condition = U->getSubExpr(); - continue; - } - return NULL; - } - default: - break; - } +static const DeclRefExpr* +GetIncrementedVar(const Expr *expr, const VarDecl *x, const VarDecl *y) { + expr = expr->IgnoreParenCasts(); + + if (const BinaryOperator *B = dyn_cast(expr)) { + if (!(B->isAssignmentOp() || B->isCompoundAssignmentOp() || + B->getOpcode() == BinaryOperator::Comma)) + return NULL; + + if (const DeclRefExpr *lhs = GetIncrementedVar(B->getLHS(), x, y)) + return lhs; + + if (const DeclRefExpr *rhs = GetIncrementedVar(B->getRHS(), x, y)) + return rhs; + + return NULL; } + + if (const DeclRefExpr *DR = dyn_cast(expr)) { + const NamedDecl *ND = DR->getDecl(); + return ND == x || ND == y ? DR : NULL; + } + + if (const UnaryOperator *U = dyn_cast(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(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(B->getLHS()->IgnoreParens()); + const DeclRefExpr *drRHS = dyn_cast(B->getRHS()->IgnoreParens()); + + if (!drLHS && !drRHS) + return; + + const VarDecl *vdLHS = drLHS ? dyn_cast(drLHS->getDecl()) : NULL; + const VarDecl *vdRHS = drRHS ? dyn_cast(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 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()); } //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/security-syntax-checks.m b/clang/test/Analysis/security-syntax-checks.m new file mode 100644 index 000000000000..897c925e2794 --- /dev/null +++ b/clang/test/Analysis/security-syntax-checks.m @@ -0,0 +1,23 @@ +// RUN: clang-cc -analyze -warn-security-syntactic %s -verify + +// 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'}} +} +