Thanks for your comments. Here's a new patch.
//Anders
________________________________________
Från: Jordan Rose [jordan_r...@apple.com]
Skickat: den 19 juni 2014 18:17
Till: Anders Rönnholm
Cc: cfe-commits@cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] Division by zero
On Jun 18, 2014, at 7:30 , Anders Rönnholm <anders.ronnh...@evidente.se> wrote:
> Changes according to comments.
>
> I'm pretty sure PVS-Studio does not have this checker, this bug has been seen
> in production code.
Oops, noticing more things:
+ const ExplodedNode *N = C.getPredecessor();
+ while (N) {
+ ProgramStateRef State = N->getState();
+
+ // Find the most recent expression bound to the symbol in the current
+ // context.
+ if (const MemRegion *MR = C.getLocationRegionIfPostStore(N)) {
+ SVal Val = State->getSVal(MR);
+ if (Val == S) {
+ Optional<DefinedSVal> DSV = Val.getAs<DefinedSVal>();
+ if (!C.getConstraintManager().assume(State, *DSV, false))
+ return true;
+ }
+ }
+ N = N->pred_empty() ? nullptr : *(N->pred_begin());
+ }
Why are you looking for the last time a value was stored rather than just
asking for the value in the current state?
if (Optional<Loc> L = S.getAs<Loc>()) {
SVal V = State->getSVal(L) // or getRawSVal if you don't want a symbol
constrained to 0 to be simplified to 0.
// ...
+ if (Optional<KnownSVal> V = Val.getAs<KnownSVal>()) {
Because you needed a symbol already to get this far, you can skip this step. Or
assert it.
+void TestAfterDivZeroChecker::checkPreStmt(const BinaryOperator *B,
+ CheckerContext &C) const {
Strange alignment here. If the second line doesn't fit, just indent it twice
from the start of the function...don't try to right-align it.
+ if (!B->getRHS()->getType()->isScalarType())
+ return;
Dividing by a floating-point 0 is well-defined, so this should be a more
specific check.
Jordan
Index: lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
+++ lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
@@ -0,0 +1,276 @@
+//== 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<DivisionBRVisitor> {
+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::PreStmt<BinaryOperator>, check::BranchCondition,
+ check::EndFunction> {
+ mutable std::unique_ptr<BuiltinBug> 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<PostStmt> P = Succ->getLocationAs<PostStmt>())
+ if (const BinaryOperator *BO = P->getStmtAs<BinaryOperator>()) {
+ 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<Loc> L;
+ if (!(L = S.getAs<Loc>())) return false;
+
+ const ExplodedNode *N = C.getPredecessor();
+ while (N) {
+ ProgramStateRef State = N->getState();
+
+ // Find the most recent expression bound to the symbol in the current
+ // context.
+ SVal Val = State->getRawSVal(L.getValue());
+ if (Val == S) {
+ Optional<DefinedSVal> DSV = Val.getAs<DefinedSVal>();
+ return !C.getConstraintManager().assume(State, *DSV, false);
+ }
+ N = N->pred_empty() ? nullptr : *(N->pred_begin());
+ }
+ return false;
+}
+
+void TestAfterDivZeroChecker::setDivZeroMap(SVal Var, CheckerContext &C) const {
+ SymbolRef SR = Var.getAsSymbol();
+ if (!SR)
+ return;
+
+ ProgramStateRef State = C.getState();
+ State =
+ State->add<DivZeroMap>(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<DivZeroMap>(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<DivZeroMap>();
+ if (DivZeroes.isEmpty())
+ return;
+
+ for (llvm::ImmutableSet<ZeroState>::iterator I = DivZeroes.begin(),
+ E = DivZeroes.end();
+ I != E; ++I) {
+ ZeroState ZS = *I;
+ if (ZS.getStackFrameContext() == C.getStackFrame())
+ State = State->remove<DivZeroMap>(ZS);
+ }
+ C.addTransition(State);
+}
+
+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) {
+
+ ProgramStateRef State = C.getState();
+ SVal S = C.getSVal(B->getRHS());
+ if (isZero(S, C))
+ return;
+
+ setDivZeroMap(S, C);
+ }
+}
+
+void TestAfterDivZeroChecker::checkBranchCondition(const Stmt *Condition,
+ CheckerContext &C) const {
+ if (const BinaryOperator *B = dyn_cast<BinaryOperator>(Condition)) {
+ if (B->isComparisonOp()) {
+ const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(B->getRHS());
+ bool LRHS = true;
+ if (!IntLiteral) {
+ IntLiteral = dyn_cast<IntegerLiteral>(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<UnaryOperator>(Condition)) {
+ if (U->getOpcode() == UO_LNot) {
+ SVal Val;
+ if (const ImplicitCastExpr *I =
+ dyn_cast<ImplicitCastExpr>(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<ImplicitCastExpr>(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<TestAfterDivZeroChecker>();
+}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt (revision 211588)
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt (working copy)
@@ -64,6 +64,7 @@
StackAddrEscapeChecker.cpp
StreamChecker.cpp
TaintTesterChecker.cpp
+ TestAfterDivZeroChecker.cpp
TraversalChecker.cpp
UndefBranchChecker.cpp
UndefCapturedBlockVarChecker.cpp
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td (revision 211588)
+++ lib/StaticAnalyzer/Checkers/Checkers.td (working copy)
@@ -124,6 +124,10 @@
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"
//===----------------------------------------------------------------------===//
Index: test/Analysis/test-after-div-zero.c
===================================================================
--- test/Analysis/test-after-div-zero.c (revision 0)
+++ test/Analysis/test-after-div-zero.c (revision 0)
@@ -0,0 +1,197 @@
+// 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 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}}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (revision 211588)
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (working copy)
@@ -174,8 +172,13 @@
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.
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
