From: Adam Balogh Date: Mon, 10 Sep 2018 09:03:22 +0000 (+0000) Subject: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6475259f54c232ecc64e6e351c1518af7afef87c;p=clang [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters New check added to the checker which checks whether iterator parameters of template functions typed by the same template parameter refer to the same container. Differential Revision: https://reviews.llvm.org/D32845 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@341790 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td index 95f11f32ec..435bf6023a 100644 --- a/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -309,12 +309,16 @@ def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">, "destructor in their base class">, DescFile<"DeleteWithNonVirtualDtorChecker.cpp">; +def InvalidatedIteratorChecker : Checker<"InvalidatedIterator">, + HelpText<"Check for use of invalidated iterators">, + DescFile<"IteratorChecker.cpp">; + def IteratorRangeChecker : Checker<"IteratorRange">, HelpText<"Check for iterators used outside their valid ranges">, DescFile<"IteratorChecker.cpp">; -def InvalidatedIteratorChecker : Checker<"InvalidatedIterator">, - HelpText<"Check for use of invalidated iterators">, +def MismatchedIteratorChecker : Checker<"MismatchedIterator">, + HelpText<"Check for use of iterators of different containers where iterators of the same container are expected">, DescFile<"IteratorChecker.cpp">; def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">, diff --git a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp index 64bac736bf..80ff661fc7 100644 --- a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -198,6 +198,7 @@ class IteratorChecker eval::Assume> { std::unique_ptr OutOfRangeBugType; + std::unique_ptr MismatchedBugType; std::unique_ptr InvalidatedBugType; void handleComparison(CheckerContext &C, const SVal &RetVal, const SVal &LVal, @@ -221,8 +222,14 @@ class IteratorChecker void verifyRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op, const SVal &RetVal, const SVal &LHS, const SVal &RHS) const; + void verifyMatch(CheckerContext &C, const SVal &Iter1, + const SVal &Iter2) const; + void reportOutOfRangeBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const; + void reportMismatchedBug(const StringRef &Message, const SVal &Val1, + const SVal &Val2, CheckerContext &C, + ExplodedNode *ErrNode) const; void reportInvalidatedBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const; @@ -231,6 +238,7 @@ public: enum CheckKind { CK_IteratorRangeChecker, + CK_MismatchedIteratorChecker, CK_InvalidatedIteratorChecker, CK_NumCheckKinds }; @@ -312,6 +320,8 @@ const ContainerData *getContainerData(ProgramStateRef State, ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont, const ContainerData &CData); bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont); +bool isBoundThroughLazyCompoundVal(const Environment &Env, + const MemRegion *Reg); bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos); bool isZero(ProgramStateRef State, const NonLoc &Val); } // namespace @@ -320,6 +330,9 @@ IteratorChecker::IteratorChecker() { OutOfRangeBugType.reset( new BugType(this, "Iterator out of range", "Misuse of STL APIs")); OutOfRangeBugType->setSuppressOnSink(true); + MismatchedBugType.reset( + new BugType(this, "Iterator(s) mismatched", "Misuse of STL APIs")); + MismatchedBugType->setSuppressOnSink(true); InvalidatedBugType.reset( new BugType(this, "Iterator invalidated", "Misuse of STL APIs")); InvalidatedBugType->setSuppressOnSink(true); @@ -368,6 +381,65 @@ void IteratorChecker::checkPreCall(const CallEvent &Call, verifyDereference(C, Call.getArgSVal(0)); } } + } else if (!isa(&Call)) { + // The main purpose of iterators is to abstract away from different + // containers and provide a (maybe limited) uniform access to them. + // This implies that any correctly written template function that + // works on multiple containers using iterators takes different + // template parameters for different containers. So we can safely + // assume that passing iterators of different containers as arguments + // whose type replaces the same template parameter is a bug. + // + // Example: + // template + // void f(I1 first1, I1 last1, I2 first2, I2 last2); + // + // In this case the first two arguments to f() must be iterators must belong + // to the same container and the last to also to the same container but + // not neccessarily to the same as the first two. + + if (!ChecksEnabled[CK_MismatchedIteratorChecker]) + return; + + const auto *Templ = Func->getPrimaryTemplate(); + if (!Templ) + return; + + const auto *TParams = Templ->getTemplateParameters(); + const auto *TArgs = Func->getTemplateSpecializationArgs(); + + // Iterate over all the template parameters + for (size_t I = 0; I < TParams->size(); ++I) { + const auto *TPDecl = dyn_cast(TParams->getParam(I)); + if (!TPDecl) + continue; + + if (TPDecl->isParameterPack()) + continue; + + const auto TAType = TArgs->get(I).getAsType(); + if (!isIteratorType(TAType)) + continue; + + SVal LHS = UndefinedVal(); + + // For every template parameter which is an iterator type in the + // instantiation look for all functions' parameters' type by it and + // check whether they belong to the same container + for (auto J = 0U; J < Func->getNumParams(); ++J) { + const auto *Param = Func->getParamDecl(J); + const auto *ParamType = + Param->getType()->getAs(); + if (!ParamType || + ParamType->getReplacedParameter()->getDecl() != TPDecl) + continue; + if (LHS.isUndef()) { + LHS = Call.getArgSVal(J); + } else { + verifyMatch(C, LHS, Call.getArgSVal(J)); + } + } + } } } @@ -529,7 +601,12 @@ void IteratorChecker::checkDeadSymbols(SymbolReaper &SR, auto RegionMap = State->get(); for (const auto Reg : RegionMap) { if (!SR.isLiveRegion(Reg.first)) { - State = State->remove(Reg.first); + // The region behind the `LazyCompoundVal` is often cleaned up before + // the `LazyCompoundVal` itself. If there are iterator positions keyed + // by these regions their cleanup must be deferred. + if (!isBoundThroughLazyCompoundVal(State->getEnvironment(), Reg.first)) { + State = State->remove(Reg.first); + } } } @@ -820,6 +897,21 @@ void IteratorChecker::verifyRandomIncrOrDecr(CheckerContext &C, } } +void IteratorChecker::verifyMatch(CheckerContext &C, const SVal &Iter1, + const SVal &Iter2) const { + // Verify match between the containers of two iterators + auto State = C.getState(); + const auto *Pos1 = getIteratorPosition(State, Iter1); + const auto *Pos2 = getIteratorPosition(State, Iter2); + if (Pos1 && Pos2 && Pos1->getContainer() != Pos2->getContainer()) { + auto *N = C.generateNonFatalErrorNode(State); + if (!N) + return; + reportMismatchedBug("Iterators of different containers used where the " + "same container is expected.", Iter1, Iter2, C, N); + } +} + void IteratorChecker::handleBegin(CheckerContext &C, const Expr *CE, const SVal &RetVal, const SVal &Cont) const { const auto *ContReg = Cont.getAsRegion(); @@ -917,6 +1009,16 @@ void IteratorChecker::reportOutOfRangeBug(const StringRef &Message, C.emitReport(std::move(R)); } +void IteratorChecker::reportMismatchedBug(const StringRef &Message, + const SVal &Val1, const SVal &Val2, + CheckerContext &C, + ExplodedNode *ErrNode) const { + auto R = llvm::make_unique(*MismatchedBugType, Message, ErrNode); + R->markInteresting(Val1); + R->markInteresting(Val2); + C.emitReport(std::move(R)); +} + void IteratorChecker::reportInvalidatedBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const { @@ -1261,6 +1363,18 @@ bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont) { return false; } +bool isBoundThroughLazyCompoundVal(const Environment &Env, + const MemRegion *Reg) { + for (const auto Binding: Env) { + if (const auto LCVal = Binding.second.getAs()) { + if (LCVal->getRegion() == Reg) + return true; + } + } + + return false; +} + template ProgramStateRef processIteratorPositions(ProgramStateRef State, Condition Cond, Process Proc) { @@ -1374,4 +1488,5 @@ bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2, } REGISTER_CHECKER(IteratorRangeChecker) +REGISTER_CHECKER(MismatchedIteratorChecker) REGISTER_CHECKER(InvalidatedIteratorChecker) diff --git a/test/Analysis/Inputs/system-header-simulator-cxx.h b/test/Analysis/Inputs/system-header-simulator-cxx.h index 468de9211a..050456740f 100644 --- a/test/Analysis/Inputs/system-header-simulator-cxx.h +++ b/test/Analysis/Inputs/system-header-simulator-cxx.h @@ -642,6 +642,12 @@ namespace std { template InputIterator find(InputIterator first, InputIterator last, const T &val); + template + ForwardIterator1 find_first_of(ForwardIterator1 first1, + ForwardIterator1 last1, + ForwardIterator2 first2, + ForwardIterator2 last2); + template OutputIterator copy(InputIterator first, InputIterator last, OutputIterator result); diff --git a/test/Analysis/invalidated-iterator.cpp b/test/Analysis/invalidated-iterator.cpp index 56f35c337b..2520072440 100644 --- a/test/Analysis/invalidated-iterator.cpp +++ b/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-relational-comparison-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-relational-comparison-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 %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 #include "Inputs/system-header-simulator-cxx.h"