Changes according to comments.
I'm pretty sure PVS-Studio does not have this checker, this bug has been seen
in production code.
//Anders
________________________________________
Från: Jordan Rose [jordan_r...@apple.com]
Skickat: den 16 juni 2014 18:39
Till: Anders Rönnholm
Cc: cfe-commits@cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] Division by zero
On Jun 5, 2014, at 5:16 , Anders Rönnholm
<anders.ronnh...@evidente.se<mailto:anders.ronnh...@evidente.se>> wrote:
Hi Jordan,
New patch and some comments.
+inline void inline_func3(int x) {
+ var = 77 / x;
+}
+void ok_inline(int x) {
+ var = 77 / x;
+ inline_func3(x);
+ if (x==0){}
+}
This is an example where we should still be warning. Why aren't we?
It doesn't warn here because blockid is in the value and not the key as it
should. I have made a set of symbol-block-frame as you suggested and that fixes
this problem.
+// RUN: %clang_cc1 -analyze -analyzer-checker=core.DivideZero2
-analyzer-output=text -verify %s
We should never be running path-sensitive checks without the rest of the core
checkers.
There is a small problem here. It looks like the DivideZero checker makes the
assumtion that because it can't see that a variable is zero then it's not zero
so all variables in our tests have the range reg_$0<x> : { [-2147483648, -1],
[1, 2147483647] }. I have removed the range check in my checker to make it work
otherwise i would have to change the DivideZero checker.
Hm. The important thing is that a variable is zero before the division; in the
absence of this checker, it's a reasonable assumption that it's zero if we made
it past the division. Would it make sense to step back through ExplodedNodes
instead until we found a ProgramPoint before the division statement itself, and
then look at the value of the variable in that state?
+ const ZeroState *ZS = C.getState()->get<DivZeroMap>(SR);
+ return (ZS && *ZS == ZeroState(C.getBlockID(), C.getStackFrame()));
...now I'm wondering about the wisdom of using a map for this. What if the same
symbol is constrained in two stack frames? Would it make more sense to keep
around a set of symbol-block-frame triples, rather than a map from symbols to
block/frame pairs?
Or can this not happen because a value can only be used once along a certain
path before it is constrained to be non-zero?
Changed to a set of symbol-block-frame triples to make the above example work.
+def DivZeroChecker2 : Checker<"DivideZero2">,
+ HelpText<"Check for division by variable that is later compared against 0.
Either the comparison is useless or there is division by zero.">,
+ DescFile<"DivZeroChecker2.cpp">;
+
I'm not sure this is really a core checker, since it's safe to run the analyzer
without it. But we don't have a category for "useful C checks that should be on
by default" yet. Anton was looking at reorganizing the categories, but the
problem is that everything outside of "alpha" is something people expect to be
able to rely on when using scan-build.
Any ideas for this particular checker, or for the general issue?
I ok with having it as an alpha, or something else. I just put it in core since
the other DivideZero checker is there.
Let's put it in alpha for now.
Code comments:
+class DivZeroChecker2
+ : public Checker<check::PreStmt<BinaryOperator>, check::BranchCondition,
+ check::EndFunction> {
Since we're not doing the normal divide-by-zero check in this checker, let's
name it something else. "TestAfterDivideChecker", maybe?
+ DivZeroMapTy DivZeroes = C.getState()->get<DivZeroMap>();
+ if (DivZeroes.isEmpty())
+ return false;
+
+ for (llvm::ImmutableSet<ZeroState>::iterator I = DivZeroes.begin(),
+ E = DivZeroes.end();
+ I != E; ++I) {
+ if (*I == ZeroState(SR, C.getBlockID(), C.getStackFrame()))
+ return true;
+ }
+ return false;
This whole thing can be simplified quite a bit:
ZeroState ZS(SR, C.getBlockID(), C.getStackFrame());
return C.getState()->contains<DivZeroMap>(ZS);
...and other than that it's looking pretty good! Has this checker found any
real bugs that you know of? (Does PVS-Studio have a similar checker?)
JordanIndex: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (revision 210244)
+++ 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.
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td (revision 210244)
+++ 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: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt (revision 210244)
+++ 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/TestAfterDivZeroChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
+++ lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp (revision 0)
@@ -0,0 +1,280 @@
+//== 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 {
+ 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());
+ }
+ 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"));
+
+ if (Optional<KnownSVal> V = Val.getAs<KnownSVal>()) {
+ 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) {
+ if (!B->getRHS()->getType()->isScalarType())
+ return;
+
+ 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: 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,187 @@
+// 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}}
+
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
