From: Jordan Rose Date: Thu, 10 Jul 2014 16:10:52 +0000 (+0000) Subject: [analyzer] Check for code testing a variable for 0 after using it as a denominator. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=00a1ef8023158119fb897948b384a6bc8c96d801;p=clang [analyzer] Check for code testing a variable for 0 after using it as a denominator. This new checker, alpha.core.TestAfterDivZero, catches issues like this: int sum = ... int avg = sum / count; // potential division by zero... if (count == 0) { ... } // ...caught here Because the analyzer does not necessarily explore /all/ paths through a program, this check is restricted to only work on zero checks that immediately follow a division operation (/ % /= %=). This could later be expanded to handle checks dominated by a division operation but not necessarily in the same CFG block. Patch by Anders Rönnholm! (with very minor modifications by me) git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@212731 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 143eb95aaa..5a33bdf01b 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -174,8 +174,13 @@ public: return Pred->getLocationContext()->getAnalysisDeclContext(); } - /// \brief If the given node corresponds to a PostStore program point, retrieve - /// the location region as it was uttered in the code. + /// \brief Get the blockID. + unsigned getBlockID() const { + return NB.getContext().getBlock()->getBlockID(); + } + + /// \brief If the given node corresponds to a PostStore program point, + /// retrieve the location region as it was uttered in the code. /// /// This utility can be useful for generating extensive diagnostics, for /// example, for finding variables that the given symbol was assigned to. diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 8e7a839133..9fb22ecc85 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -64,6 +64,7 @@ add_clang_library(clangStaticAnalyzerCheckers StackAddrEscapeChecker.cpp StreamChecker.cpp TaintTesterChecker.cpp + TestAfterDivZeroChecker.cpp TraversalChecker.cpp UndefBranchChecker.cpp UndefCapturedBlockVarChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/Checkers.td b/lib/StaticAnalyzer/Checkers/Checkers.td index a457b44cbe..44eb64187b 100644 --- a/lib/StaticAnalyzer/Checkers/Checkers.td +++ b/lib/StaticAnalyzer/Checkers/Checkers.td @@ -124,6 +124,10 @@ def CallAndMessageUnInitRefArg : Checker<"CallAndMessageUnInitRefArg">, HelpText<"Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers, and pointer to undefined variables)">, DescFile<"CallAndMessageChecker.cpp">; +def TestAfterDivZeroChecker : Checker<"TestAfterDivZero">, + HelpText<"Check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero.">, + DescFile<"TestAfterDivZeroChecker.cpp">; + } // end "alpha.core" //===----------------------------------------------------------------------===// diff --git a/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp b/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp new file mode 100644 index 0000000000..a740b7c233 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp @@ -0,0 +1,264 @@ +//== TestAfterDivZeroChecker.cpp - Test after division by zero checker --*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines TestAfterDivZeroChecker, a builtin check that performs checks +// for division by zero where the division occurs before comparison with zero. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/FoldingSet.h" + +using namespace clang; +using namespace ento; + +namespace { + +class ZeroState { +private: + SymbolRef ZeroSymbol; + unsigned BlockID; + const StackFrameContext *SFC; + +public: + ZeroState(SymbolRef S, unsigned B, const StackFrameContext *SFC) + : ZeroSymbol(S), BlockID(B), SFC(SFC) {} + + const StackFrameContext *getStackFrameContext() const { return SFC; } + + bool operator==(const ZeroState &X) const { + return BlockID == X.BlockID && SFC == X.SFC && ZeroSymbol == X.ZeroSymbol; + } + + bool operator<(const ZeroState &X) const { + if (BlockID != X.BlockID) + return BlockID < X.BlockID; + if (SFC != X.SFC) + return SFC < X.SFC; + return ZeroSymbol < X.ZeroSymbol; + } + + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddInteger(BlockID); + ID.AddPointer(SFC); + ID.AddPointer(ZeroSymbol); + } +}; + +class DivisionBRVisitor : public BugReporterVisitorImpl { +private: + SymbolRef ZeroSymbol; + const StackFrameContext *SFC; + bool Satisfied = false; + +public: + DivisionBRVisitor(SymbolRef ZeroSymbol, const StackFrameContext *SFC) + : ZeroSymbol(ZeroSymbol), SFC(SFC) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + ID.Add(ZeroSymbol); + ID.Add(SFC); + } + + PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ, + const ExplodedNode *Pred, + BugReporterContext &BRC, + BugReport &BR) override; +}; + +class TestAfterDivZeroChecker + : public Checker, check::BranchCondition, + check::EndFunction> { + mutable std::unique_ptr DivZeroBug; + void reportBug(SVal Val, CheckerContext &C) const; + +public: + void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const; + void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const; + void checkEndFunction(CheckerContext &C) const; + void setDivZeroMap(SVal Var, CheckerContext &C) const; + bool hasDivZeroMap(SVal Var, const CheckerContext &C) const; + bool isZero(SVal S, CheckerContext &C) const; +}; +} // end anonymous namespace + +REGISTER_SET_WITH_PROGRAMSTATE(DivZeroMap, ZeroState) + +PathDiagnosticPiece *DivisionBRVisitor::VisitNode(const ExplodedNode *Succ, + const ExplodedNode *Pred, + BugReporterContext &BRC, + BugReport &BR) { + if (Satisfied) + return nullptr; + + const Expr *E = nullptr; + + if (Optional P = Succ->getLocationAs()) + if (const BinaryOperator *BO = P->getStmtAs()) { + BinaryOperator::Opcode Op = BO->getOpcode(); + if (Op == BO_Div || Op == BO_Rem || Op == BO_DivAssign || + Op == BO_RemAssign) { + E = BO->getRHS(); + } + } + + if (!E) + return nullptr; + + ProgramStateRef State = Succ->getState(); + SVal S = State->getSVal(E, Succ->getLocationContext()); + if (ZeroSymbol == S.getAsSymbol() && SFC == Succ->getStackFrame()) { + Satisfied = true; + + // Construct a new PathDiagnosticPiece. + ProgramPoint P = Succ->getLocation(); + PathDiagnosticLocation L = + PathDiagnosticLocation::create(P, BRC.getSourceManager()); + + if (!L.isValid() || !L.asLocation().isValid()) + return nullptr; + + return new PathDiagnosticEventPiece( + L, "Division with compared value made here"); + } + + return nullptr; +} + +bool TestAfterDivZeroChecker::isZero(SVal S, CheckerContext &C) const { + Optional DSV = S.getAs(); + + if (!DSV) + return false; + + ConstraintManager &CM = C.getConstraintManager(); + return !CM.assume(C.getState(), *DSV, true); +} + +void TestAfterDivZeroChecker::setDivZeroMap(SVal Var, CheckerContext &C) const { + SymbolRef SR = Var.getAsSymbol(); + if (!SR) + return; + + ProgramStateRef State = C.getState(); + State = + State->add(ZeroState(SR, C.getBlockID(), C.getStackFrame())); + C.addTransition(State); +} + +bool TestAfterDivZeroChecker::hasDivZeroMap(SVal Var, + const CheckerContext &C) const { + SymbolRef SR = Var.getAsSymbol(); + if (!SR) + return false; + + ZeroState ZS(SR, C.getBlockID(), C.getStackFrame()); + return C.getState()->contains(ZS); +} + +void TestAfterDivZeroChecker::reportBug(SVal Val, CheckerContext &C) const { + if (ExplodedNode *N = C.generateSink(C.getState())) { + if (!DivZeroBug) + DivZeroBug.reset(new BuiltinBug(this, "Division by zero")); + + BugReport *R = + new BugReport(*DivZeroBug, "Value being compared against zero has " + "already been used for division", + N); + + R->addVisitor(new DivisionBRVisitor(Val.getAsSymbol(), C.getStackFrame())); + C.emitReport(R); + } +} + +void TestAfterDivZeroChecker::checkEndFunction(CheckerContext &C) const { + ProgramStateRef State = C.getState(); + + DivZeroMapTy DivZeroes = State->get(); + if (DivZeroes.isEmpty()) + return; + + DivZeroMapTy::Factory &F = State->get_context(); + for (llvm::ImmutableSet::iterator I = DivZeroes.begin(), + E = DivZeroes.end(); + I != E; ++I) { + ZeroState ZS = *I; + if (ZS.getStackFrameContext() == C.getStackFrame()) + DivZeroes = F.remove(DivZeroes, ZS); + } + C.addTransition(State->set(DivZeroes)); +} + +void TestAfterDivZeroChecker::checkPreStmt(const BinaryOperator *B, + CheckerContext &C) const { + BinaryOperator::Opcode Op = B->getOpcode(); + if (Op == BO_Div || Op == BO_Rem || Op == BO_DivAssign || + Op == BO_RemAssign) { + SVal S = C.getSVal(B->getRHS()); + + if (!isZero(S, C)) + setDivZeroMap(S, C); + } +} + +void TestAfterDivZeroChecker::checkBranchCondition(const Stmt *Condition, + CheckerContext &C) const { + if (const BinaryOperator *B = dyn_cast(Condition)) { + if (B->isComparisonOp()) { + const IntegerLiteral *IntLiteral = dyn_cast(B->getRHS()); + bool LRHS = true; + if (!IntLiteral) { + IntLiteral = dyn_cast(B->getLHS()); + LRHS = false; + } + + if (!IntLiteral || IntLiteral->getValue() != 0) + return; + + SVal Val = C.getSVal(LRHS ? B->getLHS() : B->getRHS()); + if (hasDivZeroMap(Val, C)) + reportBug(Val, C); + } + } else if (const UnaryOperator *U = dyn_cast(Condition)) { + if (U->getOpcode() == UO_LNot) { + SVal Val; + if (const ImplicitCastExpr *I = + dyn_cast(U->getSubExpr())) + Val = C.getSVal(I->getSubExpr()); + + if (hasDivZeroMap(Val, C)) + reportBug(Val, C); + else { + Val = C.getSVal(U->getSubExpr()); + if (hasDivZeroMap(Val, C)) + reportBug(Val, C); + } + } + } else if (const ImplicitCastExpr *IE = + dyn_cast(Condition)) { + SVal Val = C.getSVal(IE->getSubExpr()); + + if (hasDivZeroMap(Val, C)) + reportBug(Val, C); + else { + SVal Val = C.getSVal(Condition); + + if (hasDivZeroMap(Val, C)) + reportBug(Val, C); + } + } +} + +void ento::registerTestAfterDivZeroChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} diff --git a/test/Analysis/test-after-div-zero.c b/test/Analysis/test-after-div-zero.c new file mode 100644 index 0000000000..f34c4f7a36 --- /dev/null +++ b/test/Analysis/test-after-div-zero.c @@ -0,0 +1,204 @@ +// RUN: %clang_cc1 -std=c99 -Dbool=_Bool -analyze -analyzer-checker=core,alpha.core.TestAfterDivZero -analyzer-output=text -verify %s +// RUN: %clang_cc1 -x c++ -analyze -analyzer-checker=core,alpha.core.TestAfterDivZero -analyzer-output=text -verify %s + +int var; + +void err_eq(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x == 0) { } // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_eq2(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (0 == x) { } // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_ne(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x != 0) { } // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_ge(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x >= 0) { } // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_le(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x <= 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_yes(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} +void err_not(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_pnot(int x) { + int *y = &x; + var = 77 / *y; // expected-note {{Division with compared value made here}} + if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_pnot2(int x) { + int *y = &x; + var = 77 / x; // expected-note {{Division with compared value made here}} + if (!*y) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_ppnot(int x) { + int *y = &x; + int **z = &y; + var = 77 / **z; // expected-note {{Division with compared value made here}} + if (!x) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_orig_checker(int x) { + if (x != 0) // expected-note {{Assuming 'x' is equal to 0}} expected-note {{Taking false branch}} + return; + var = 77 / x; // expected-warning {{Division by zero}} expected-note {{Division by zero}} + if (!x) {} // no-warning +} + +void ok_other(int x, int y) { + var = 77 / y; + if (x == 0) { + } +} + +void ok_assign(int x) { + var = 77 / x; + x = var / 77; // <- assignment => don't warn + if (x == 0) { + } +} + +void ok_assign2(int x) { + var = 77 / x; + x = var / 77; // <- assignment => don't warn + if (0 == x) { + } +} + +void ok_dec(int x) { + var = 77 / x; + x--; // <- assignment => don't warn + if (x == 0) { + } +} + +void ok_inc(int x) { + var = 77 / x; + x++; // <- assignment => don't warn + if (x == 0) { + } +} + +void do_something_ptr(int *x); +void ok_callfunc_ptr(int x) { + var = 77 / x; + do_something_ptr(&x); // <- pass address of x to function => don't warn + if (x == 0) { + } +} + +void do_something(int x); +void nok_callfunc(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + do_something(x); + if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void ok_if(int x) { + if (x > 3) + var = 77 / x; + if (x == 0) { + } +} + +void ok_if2(int x) { + if (x < 3) + var = 77 / x; + if (x == 0) { + } // TODO warn here +} + +void ok_pif(int x) { + int *y = &x; + if (x < 3) + var = 77 / *y; + if (x == 0) { + } // TODO warn here +} + +int getValue(bool *isPositive); +void use(int a); +void foo() { + bool isPositive; + int x = getValue(&isPositive); + if (isPositive) { + use(5 / x); + } + + if (x == 0) { + } +} + +int getValue2(); +void foo2() { + int x = getValue2(); + int y = x; + + use(5 / x); // expected-note {{Division with compared value made here}} + if (y == 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void ok_while(int x) { + int n = 100 / x; + while (x != 0) { // <- do not warn + x--; + } +} + +void err_not2(int x, int y) { + int v; + var = 77 / x; + + if (y) + v = 0; + + if (!x) { + } // TODO warn here +} + +inline void inline_func(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +void err_inline(int x) { + var = 77 / x; + inline_func(x); // expected-note {{Calling 'inline_func'}} + if (x == 0) { + } +} + +inline void inline_func2(int x) {} + +void err_inline2(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + inline_func2(x); + if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}} + +inline void inline_func3(int x) { + var = 77 / x; +} +void ok_inline(int x) { + var = 77 / x; // expected-note {{Division with compared value made here}} + inline_func3(x); + if (x == 0) {} // expected-warning {{Value being compared against zero has already been used for division}} +} // expected-note@-1 {{Value being compared against zero has already been used for division}}