Skip to content

Commit

Permalink
[Analyzer] Iterator Checker - Forbid decrements past the begin() and …
Browse files Browse the repository at this point in the history 
…increments past the end() of containers

Previously, the iterator range checker only warned upon dereferencing of
iterators outside their valid range as well as increments and decrements of
out-of-range iterators where the result remains out-of-range. However, the C++
standard is more strict than this: decrementing begin() or incrementing end()
results in undefined behaviour even if the iterator is not dereferenced
afterwards. Coming back to the range once out-of-range is also undefined.

This patch corrects the behaviour of the iterator range checker: warnings are
given for any operation whose result is ahead of begin() or past the end()
(which is the past-end iterator itself, thus now we are speaking of past
past-the-end).

Differential Revision: https://reviews.llvm.org/D53812

llvm-svn: 348245
  • Loading branch information
Adam Balogh committed Dec 4, 2018
1 parent 42d241f commit d5bd3f6
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 124 deletions.
244 changes: 154 additions & 90 deletions clang/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
View file
Expand Up @@ -238,14 +238,17 @@ class IteratorChecker
void handleEraseAfter(CheckerContext &C, const SVal &Iter) const;
void handleEraseAfter(CheckerContext &C, const SVal &Iter1,
const SVal &Iter2) const;
void verifyIncrement(CheckerContext &C, const SVal &Iter) const;
void verifyDecrement(CheckerContext &C, const SVal &Iter) const;
void verifyRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op,
const SVal &RetVal, const SVal &LHS,
const SVal &RHS) const;
const SVal &LHS, const SVal &RHS) const;
void verifyMatch(CheckerContext &C, const SVal &Iter,
const MemRegion *Cont) const;
void verifyMatch(CheckerContext &C, const SVal &Iter1,
const SVal &Iter2) const;

IteratorPosition advancePosition(CheckerContext &C, OverloadedOperatorKind Op,
const IteratorPosition &Pos,
const SVal &Distance) const;
void reportOutOfRangeBug(const StringRef &Message, const SVal &Val,
CheckerContext &C, ExplodedNode *ErrNode) const;
void reportMismatchedBug(const StringRef &Message, const SVal &Val1,
Expand Down Expand Up @@ -388,7 +391,9 @@ ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont,
bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont);
bool isBoundThroughLazyCompoundVal(const Environment &Env,
const MemRegion *Reg);
bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos);
bool isPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos);
bool isAheadOfRange(ProgramStateRef State, const IteratorPosition &Pos);
bool isBehindPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos);
bool isZero(ProgramStateRef State, const NonLoc &Val);
} // namespace

Expand Down Expand Up @@ -422,29 +427,46 @@ void IteratorChecker::checkPreCall(const CallEvent &Call,
verifyAccess(C, Call.getArgSVal(0));
}
}
if (ChecksEnabled[CK_IteratorRangeChecker] &&
isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) {
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
// Check for out-of-range incrementions and decrementions
if (Call.getNumArgs() >= 1) {
verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
Call.getReturnValue(),
InstCall->getCXXThisVal(), Call.getArgSVal(0));
if (ChecksEnabled[CK_IteratorRangeChecker]) {
if (isIncrementOperator(Func->getOverloadedOperator())) {
// Check for out-of-range incrementions
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
verifyIncrement(C, InstCall->getCXXThisVal());
} else {
if (Call.getNumArgs() >= 1) {
verifyIncrement(C, Call.getArgSVal(0));
}
}
} else {
if (Call.getNumArgs() >= 2) {
verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
Call.getReturnValue(), Call.getArgSVal(0),
Call.getArgSVal(1));
} else if (isDecrementOperator(Func->getOverloadedOperator())) {
// Check for out-of-range decrementions
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
verifyDecrement(C, InstCall->getCXXThisVal());
} else {
if (Call.getNumArgs() >= 1) {
verifyDecrement(C, Call.getArgSVal(0));
}
}
} else if (isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) {
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
// Check for out-of-range incrementions and decrementions
if (Call.getNumArgs() >= 1) {
verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
InstCall->getCXXThisVal(),
Call.getArgSVal(0));
}
} else {
if (Call.getNumArgs() >= 2) {
verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
Call.getArgSVal(0), Call.getArgSVal(1));
}
}
} else if (isDereferenceOperator(Func->getOverloadedOperator())) {
// Check for dereference of out-of-range iterators
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
verifyDereference(C, InstCall->getCXXThisVal());
} else {
verifyDereference(C, Call.getArgSVal(0));
}
}
} else if (ChecksEnabled[CK_IteratorRangeChecker] &&
isDereferenceOperator(Func->getOverloadedOperator())) {
// Check for dereference of out-of-range iterators
if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
verifyDereference(C, InstCall->getCXXThisVal());
} else {
verifyDereference(C, Call.getArgSVal(0));
}
} else if (ChecksEnabled[CK_MismatchedIteratorChecker] &&
isComparisonOperator(Func->getOverloadedOperator())) {
Expand Down Expand Up @@ -895,11 +917,12 @@ void IteratorChecker::verifyDereference(CheckerContext &C,
const SVal &Val) const {
auto State = C.getState();
const auto *Pos = getIteratorPosition(State, Val);
if (Pos && isOutOfRange(State, *Pos)) {
if (Pos && isPastTheEnd(State, *Pos)) {
auto *N = C.generateNonFatalErrorNode(State);
if (!N)
return;
reportOutOfRangeBug("Iterator accessed outside of its range.", Val, C, N);
reportOutOfRangeBug("Past-the-end iterator dereferenced.", Val, C, N);
return;
}
}

Expand All @@ -924,14 +947,9 @@ void IteratorChecker::handleIncrement(CheckerContext &C, const SVal &RetVal,
if (Pos) {
auto &SymMgr = C.getSymbolManager();
auto &BVF = SymMgr.getBasicVals();
auto &SVB = C.getSValBuilder();
const auto OldOffset = Pos->getOffset();
auto NewOffset =
SVB.evalBinOp(State, BO_Add,
nonloc::SymbolVal(OldOffset),
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))),
SymMgr.getType(OldOffset)).getAsSymbol();
auto NewPos = Pos->setTo(NewOffset);
const auto NewPos =
advancePosition(C, OO_Plus, *Pos,
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))));
State = setIteratorPosition(State, Iter, NewPos);
State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos);
C.addTransition(State);
Expand All @@ -947,14 +965,9 @@ void IteratorChecker::handleDecrement(CheckerContext &C, const SVal &RetVal,
if (Pos) {
auto &SymMgr = C.getSymbolManager();
auto &BVF = SymMgr.getBasicVals();
auto &SVB = C.getSValBuilder();
const auto OldOffset = Pos->getOffset();
auto NewOffset =
SVB.evalBinOp(State, BO_Sub,
nonloc::SymbolVal(OldOffset),
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))),
SymMgr.getType(OldOffset)).getAsSymbol();
auto NewPos = Pos->setTo(NewOffset);
const auto NewPos =
advancePosition(C, OO_Minus, *Pos,
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))));
State = setIteratorPosition(State, Iter, NewPos);
State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos);
C.addTransition(State);
Expand Down Expand Up @@ -1020,69 +1033,64 @@ void IteratorChecker::handleRandomIncrOrDecr(CheckerContext &C,
value = &val;
}

auto &SymMgr = C.getSymbolManager();
auto &SVB = C.getSValBuilder();
auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
const auto OldOffset = Pos->getOffset();
SymbolRef NewOffset;
if (const auto intValue = value->getAs<nonloc::ConcreteInt>()) {
// For concrete integers we can calculate the new position
NewOffset = SVB.evalBinOp(State, BinOp, nonloc::SymbolVal(OldOffset),
*intValue,
SymMgr.getType(OldOffset)).getAsSymbol();
} else {
// For other symbols create a new symbol to keep expressions simple
const auto &LCtx = C.getLocationContext();
NewOffset = SymMgr.conjureSymbol(nullptr, LCtx, SymMgr.getType(OldOffset),
C.blockCount());
State = assumeNoOverflow(State, NewOffset, 4);
}
auto NewPos = Pos->setTo(NewOffset);
auto &TgtVal = (Op == OO_PlusEqual || Op == OO_MinusEqual) ? LHS : RetVal;
State = setIteratorPosition(State, TgtVal, NewPos);
State =
setIteratorPosition(State, TgtVal, advancePosition(C, Op, *Pos, *value));
C.addTransition(State);
}

void IteratorChecker::verifyIncrement(CheckerContext &C,
const SVal &Iter) const {
auto &BVF = C.getSValBuilder().getBasicValueFactory();
verifyRandomIncrOrDecr(C, OO_Plus, Iter,
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))));
}

void IteratorChecker::verifyDecrement(CheckerContext &C,
const SVal &Iter) const {
auto &BVF = C.getSValBuilder().getBasicValueFactory();
verifyRandomIncrOrDecr(C, OO_Minus, Iter,
nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))));
}

void IteratorChecker::verifyRandomIncrOrDecr(CheckerContext &C,
OverloadedOperatorKind Op,
const SVal &RetVal,
const SVal &LHS,
const SVal &RHS) const {
auto State = C.getState();

// If the iterator is initially inside its range, then the operation is valid
const auto *Pos = getIteratorPosition(State, LHS);
if (!Pos || !isOutOfRange(State, *Pos))
if (!Pos)
return;

auto value = RHS;
if (auto loc = RHS.getAs<Loc>()) {
value = State->getRawSVal(*loc);
auto Value = RHS;
if (auto ValAsLoc = RHS.getAs<Loc>()) {
Value = State->getRawSVal(*ValAsLoc);
}

// Incremention or decremention by 0 is never bug
if (isZero(State, value.castAs<NonLoc>()))
if (Value.isUnknown())
return;

auto &SymMgr = C.getSymbolManager();
auto &SVB = C.getSValBuilder();
auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
const auto OldOffset = Pos->getOffset();
const auto intValue = value.getAs<nonloc::ConcreteInt>();
if (!intValue)
// Incremention or decremention by 0 is never a bug.
if (isZero(State, Value.castAs<NonLoc>()))
return;

auto NewOffset = SVB.evalBinOp(State, BinOp, nonloc::SymbolVal(OldOffset),
*intValue,
SymMgr.getType(OldOffset)).getAsSymbol();
auto NewPos = Pos->setTo(NewOffset);

// If out of range, the only valid operation is to step into the range
if (isOutOfRange(State, NewPos)) {
// The result may be the past-end iterator of the container, but any other
// out of range position is undefined behaviour
if (isAheadOfRange(State, advancePosition(C, Op, *Pos, Value))) {
auto *N = C.generateNonFatalErrorNode(State);
if (!N)
return;
reportOutOfRangeBug("Iterator accessed past its end.", LHS, C, N);
reportOutOfRangeBug("Iterator decremented ahead of its valid range.", LHS,
C, N);
}
if (isBehindPastTheEnd(State, advancePosition(C, Op, *Pos, Value))) {
auto *N = C.generateNonFatalErrorNode(State);
if (!N)
return;
reportOutOfRangeBug("Iterator incremented behind the past-the-end "
"iterator.", LHS, C, N);
}
}

Expand Down Expand Up @@ -1544,6 +1552,35 @@ void IteratorChecker::handleEraseAfter(CheckerContext &C, const SVal &Iter1,
C.addTransition(State);
}

IteratorPosition IteratorChecker::advancePosition(CheckerContext &C,
OverloadedOperatorKind Op,
const IteratorPosition &Pos,
const SVal &Distance) const {
auto State = C.getState();
auto &SymMgr = C.getSymbolManager();
auto &SVB = C.getSValBuilder();

assert ((Op == OO_Plus || Op == OO_PlusEqual ||
Op == OO_Minus || Op == OO_MinusEqual) &&
"Advance operator must be one of +, -, += and -=.");
auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
if (const auto IntDist = Distance.getAs<nonloc::ConcreteInt>()) {
// For concrete integers we can calculate the new position
return Pos.setTo(SVB.evalBinOp(State, BinOp,
nonloc::SymbolVal(Pos.getOffset()), *IntDist,
SymMgr.getType(Pos.getOffset()))
.getAsSymbol());
} else {
// For other symbols create a new symbol to keep expressions simple
const auto &LCtx = C.getLocationContext();
const auto NewPosSym = SymMgr.conjureSymbol(nullptr, LCtx,
SymMgr.getType(Pos.getOffset()),
C.blockCount());
State = assumeNoOverflow(State, NewPosSym, 4);
return Pos.setTo(NewPosSym);
}
}

void IteratorChecker::reportOutOfRangeBug(const StringRef &Message,
const SVal &Val, CheckerContext &C,
ExplodedNode *ErrNode) const {
Expand Down Expand Up @@ -1583,7 +1620,8 @@ void IteratorChecker::reportInvalidatedBug(const StringRef &Message,
namespace {

bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
bool isGreater(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
bool isEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
BinaryOperator::Opcode Opc);
bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2,
Expand Down Expand Up @@ -2276,14 +2314,27 @@ bool isZero(ProgramStateRef State, const NonLoc &Val) {
BO_EQ);
}

bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) {
bool isPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos) {
const auto *Cont = Pos.getContainer();
const auto *CData = getContainerData(State, Cont);
if (!CData)
return false;

// Out of range means less than the begin symbol or greater or equal to the
// end symbol.
const auto End = CData->getEnd();
if (End) {
if (isEqual(State, Pos.getOffset(), End)) {
return true;
}
}

return false;
}

bool isAheadOfRange(ProgramStateRef State, const IteratorPosition &Pos) {
const auto *Cont = Pos.getContainer();
const auto *CData = getContainerData(State, Cont);
if (!CData)
return false;

const auto Beg = CData->getBegin();
if (Beg) {
Expand All @@ -2292,9 +2343,18 @@ bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) {
}
}

return false;
}

bool isBehindPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos) {
const auto *Cont = Pos.getContainer();
const auto *CData = getContainerData(State, Cont);
if (!CData)
return false;

const auto End = CData->getEnd();
if (End) {
if (isGreaterOrEqual(State, Pos.getOffset(), End)) {
if (isGreater(State, Pos.getOffset(), End)) {
return true;
}
}
Expand All @@ -2306,8 +2366,12 @@ bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) {
return compare(State, Sym1, Sym2, BO_LT);
}

bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) {
return compare(State, Sym1, Sym2, BO_GE);
bool isGreater(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) {
return compare(State, Sym1, Sym2, BO_GT);
}

bool isEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) {
return compare(State, Sym1, Sym2, BO_EQ);
}

bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
Expand Down

0 comments on commit d5bd3f6

Please sign in to comment.