baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus.
baloghadamsoftware added a project: clang.
Herald added subscribers: steakhal, Charusso, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, xazax.hun.
To understand the bug report issued by the `InvalidatedIterator` checker it is
essential to see the point where the iterator was invalidated and also to track
the assignemnts of the already invalidated iterator up to its errorneous
access. Since iterator invalidation is neither "done" by `InvalidatedIterator`
checker nor by `IteratorModeling` but "happens" upon container modifications
(modeled by `ContainerModeling`) the proper solution here seems to be a
`BugReporterVisitor` instead of `NoteTag`s.
Repository:
rC Clang
https://reviews.llvm.org/D74615
Files:
clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
clang/test/Analysis/invalidated-iterator.cpp
Index: clang/test/Analysis/invalidated-iterator.cpp
===================================================================
--- clang/test/Analysis/invalidated-iterator.cpp
+++ clang/test/Analysis/invalidated-iterator.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
#include "Inputs/system-header-simulator-cxx.h"
@@ -12,8 +12,9 @@
void invalidated_dereference(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
*i; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_prefix_increment(std::vector<int> &V) {
@@ -23,8 +24,9 @@
void invalidated_prefix_increment(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
++i; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_prefix_decrement(std::vector<int> &V) {
@@ -34,8 +36,9 @@
void invalidated_prefix_decrement(std::vector<int> &V) {
auto i = ++V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
--i; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_postfix_increment(std::vector<int> &V) {
@@ -45,8 +48,9 @@
void invalidated_postfix_increment(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
i++; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_postfix_decrement(std::vector<int> &V) {
@@ -56,8 +60,9 @@
void invalidated_postfix_decrement(std::vector<int> &V) {
auto i = ++V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
i--; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_increment_by_2(std::vector<int> &V) {
@@ -67,8 +72,9 @@
void invalidated_increment_by_2(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
i += 2; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_increment_by_2_copy(std::vector<int> &V) {
@@ -78,8 +84,9 @@
void invalidated_increment_by_2_copy(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
auto j = i + 2; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_decrement_by_2(std::vector<int> &V) {
@@ -89,8 +96,9 @@
void invalidated_decrement_by_2(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
i -= 2; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_decrement_by_2_copy(std::vector<int> &V) {
@@ -100,8 +108,9 @@
void invalidated_decrement_by_2_copy(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
auto j = i - 2; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void normal_subscript(std::vector<int> &V) {
@@ -111,8 +120,9 @@
void invalidated_subscript(std::vector<int> &V) {
auto i = V.cbegin();
- V.erase(i);
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
i[1]; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
}
void assignment(std::vector<int> &V) {
@@ -120,3 +130,11 @@
V.erase(i);
auto j = V.cbegin(); // no-warning
}
+
+void indirect(std::vector<int> &V) {
+ auto i = V.cbegin();
+ V.erase(i); // expected-note{{Iterator 'i' invalidated}}
+ auto j = i; // expected-note{{Iterator 'i' assigned to 'j'}}
+ *j; // expected-warning{{Invalidated iterator accessed}}
+ // expected-note@-1{{Invalidated iterator accessed}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
@@ -30,9 +30,7 @@
std::unique_ptr<BugType> InvalidatedBugType;
- void verifyAccess(CheckerContext &C, const SVal &Val) const;
- void reportBug(const StringRef &Message, const SVal &Val,
- CheckerContext &C, ExplodedNode *ErrNode) const;
+ void verifyAccess(CheckerContext &C, const SVal &Val, const Expr *Ex) const;
public:
InvalidatedIteratorChecker();
@@ -40,6 +38,29 @@
};
+class IteratorInvalidationBRVisitor final : public BugReporterVisitor {
+ SVal Iter;
+ const Expr *IterExpr;
+ bool Satisfied = false;
+
+public:
+ IteratorInvalidationBRVisitor(SVal It, const Expr *ItEx) : Iter(It),
+ IterExpr(ItEx) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.Add(Iter);
+ ID.Add(IterExpr);
+ }
+
+ std::shared_ptr<PathDiagnosticPiece>
+ VisitNode(const ExplodedNode *Succ, BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) override;
+
+private:
+ bool getPredecessor(const ExplodedNode *Node, const Stmt *S, BugReport &BR,
+ SVal &PredVal, const Expr *&PredExpr) const;
+};
+
} //namespace
InvalidatedIteratorChecker::InvalidatedIteratorChecker() {
@@ -58,14 +79,16 @@
isAccessOperator(Func->getOverloadedOperator())) {
// Check for any kind of access of invalidated iterator positions
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
- verifyAccess(C, InstCall->getCXXThisVal());
+ verifyAccess(C, InstCall->getCXXThisVal(), InstCall->getCXXThisExpr());
} else {
- verifyAccess(C, Call.getArgSVal(0));
+ verifyAccess(C, Call.getArgSVal(0), Call.getArgExpr(0));
}
}
}
-void InvalidatedIteratorChecker::verifyAccess(CheckerContext &C, const SVal &Val) const {
+void InvalidatedIteratorChecker::verifyAccess(CheckerContext &C,
+ const SVal &Val,
+ const Expr *Ex) const {
auto State = C.getState();
const auto *Pos = getIteratorPosition(State, Val);
if (Pos && !Pos->isValid()) {
@@ -73,17 +96,151 @@
if (!N) {
return;
}
- reportBug("Invalidated iterator accessed.", Val, C, N);
+ auto R =
+ std::make_unique<PathSensitiveBugReport>(*InvalidatedBugType,
+ "Invalidated iterator accessed.",
+ N);
+ R->markInteresting(Val);
+ R->addVisitor(std::make_unique<IteratorInvalidationBRVisitor>(Val, Ex));
+ C.emitReport(std::move(R));
}
}
-void InvalidatedIteratorChecker::reportBug(const StringRef &Message,
- const SVal &Val, CheckerContext &C,
- ExplodedNode *ErrNode) const {
- auto R = std::make_unique<PathSensitiveBugReport>(*InvalidatedBugType,
- Message, ErrNode);
- R->markInteresting(Val);
- C.emitReport(std::move(R));
+std::shared_ptr<PathDiagnosticPiece>
+IteratorInvalidationBRVisitor::VisitNode(const ExplodedNode *Succ,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) {
+ if (Satisfied)
+ return nullptr;
+
+ const ExplodedNode *Pred = Succ->getFirstPred();
+ const Stmt *S = nullptr;
+ const auto SP = Succ->getLocation().getAs<StmtPoint>();
+ if (SP.hasValue()) {
+ S = SP->getStmt();
+ } else {
+ const auto E = Succ->getLocation().getAs<BlockEdge>();
+ if (E.hasValue()) {
+ S = E->getSrc()->getTerminator().getStmt();
+ }
+ }
+
+ if (!S)
+ return nullptr;
+
+ const auto &StateBefore = Pred->getState();
+ const auto &StateAfter = Succ->getState();
+ const auto *PosBefore = getIteratorPosition(StateBefore, Iter);
+ const auto *PosAfter = getIteratorPosition(StateAfter, Iter);
+
+ if (!PosAfter)
+ return nullptr;
+
+ SmallString<256> Buf;
+ llvm::raw_svector_ostream Out(Buf);
+
+ StringRef Name;
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(IterExpr->IgnoreParenCasts())) {
+ Name = DRE->getDecl()->getName();
+ }
+
+ if ((PosBefore && PosBefore->isValid()) &&
+ (!PosAfter->isValid())) {
+
+ Out << "Iterator " << (!Name.empty() ? ("'" + Name.str() + "' ") : "" )
+ << "invalidated.";
+ Satisfied = true;
+ }
+
+ SVal PredIter;
+ const Expr *PredIterExpr;
+ if (getPredecessor(Succ, S, BR, PredIter, PredIterExpr)) {
+ StringRef PredName;
+ if (const auto *DRE =
+ dyn_cast<DeclRefExpr>(PredIterExpr->IgnoreParenCasts())) {
+ PredName = DRE->getDecl()->getName();
+ }
+
+ if (Name != PredName) {
+ Out << "Iterator "
+ << (!PredName.empty() ? ("'" + PredName.str() + "' ") : "" )
+ << "assigned"
+ << (!Name.empty() ? (" to '" + Name.str() + "'.") : "." );
+ }
+
+ BR.addVisitor(std::make_unique<IteratorInvalidationBRVisitor>(PredIter,
+ PredIterExpr));
+ Satisfied = true;
+ }
+
+ if (!Buf.empty()) {
+ auto L = PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(),
+ Succ->getLocationContext());
+
+ if (!L.isValid() || !L.asLocation().isValid())
+ return nullptr;
+
+ auto Piece = std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
+ Piece->addRange(S->getSourceRange());
+
+ return std::move(Piece);
+ }
+
+ return nullptr;
+}
+
+bool
+IteratorInvalidationBRVisitor::getPredecessor(const ExplodedNode *Node,
+ const Stmt *S,
+ BugReport &BR,
+ SVal &PredVal,
+ const Expr *&PredExpr) const {
+ auto State = Node->getState();
+ const auto *LCtx = Node->getLocationContext();
+
+ const auto IterSym = Iter.getAsSymbol();
+ const auto *IterReg = Iter.getAsRegion();
+ if (const auto IterLC = Iter.getAs<nonloc::LazyCompoundVal>()) {
+ IterReg = IterLC->getRegion();
+ }
+ const auto *IterPos = getIteratorPosition(State, Iter);
+ assert(IterPos &&
+ "This function is only for iterators with existing positions.");
+
+ const auto *E = dyn_cast<Expr>(S);
+ if (!E)
+ return false;
+
+ const auto &RetVal = State->getSVal(E, LCtx);
+ const auto RetSym = RetVal.getAsSymbol();
+ const auto *RetReg = RetVal.getAsRegion();
+ if (const auto RetLC = RetVal.getAs<nonloc::LazyCompoundVal>()) {
+ RetReg = RetLC->getRegion();
+ }
+
+ // If the current iterator is the return value of an operator and it is a an
+ // assignment operator then its parameter is the predecessor.
+ if (const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(E)) {
+ if (OpCall->getOperator() == OO_Equal) {
+ if (IterSym == RetSym || IterReg == RetReg) {
+ PredExpr = OpCall->getArg(0);
+ PredVal = State->getSVal(PredExpr, LCtx);
+ return true;
+ }
+ }
+ // If the current iterator is the result of a C++ construct expression
+ // and the constructor is a copy or move constructor then its parameter is
+ // the predecessor.
+ } else if (const auto *ConstrExpr = dyn_cast<CXXConstructExpr>(E)) {
+ if (ConstrExpr->getConstructor()->isCopyOrMoveConstructor()) {
+ if (IterSym == RetSym || IterReg == RetReg) {
+ PredExpr = ConstrExpr->getArg(0);
+ PredVal = State->getSVal(PredExpr, LCtx);
+ return true;
+ }
+ }
+ }
+ return false;
}
void ento::registerInvalidatedIteratorChecker(CheckerManager &mgr) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
