[clang] a59d4ae - [Analyzer] Hotfix for various crashes in iterator checkers
Adam Balogh via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 16 11:50:48 PDT 2020
Author: Adam Balogh
Date: 2020-07-16T20:49:33+02:00
New Revision: a59d4ae4313c0a961c50d14c0616b49220c5a469
URL: https://github.com/llvm/llvm-project/commit/a59d4ae4313c0a961c50d14c0616b49220c5a469
DIFF: https://github.com/llvm/llvm-project/commit/a59d4ae4313c0a961c50d14c0616b49220c5a469.diff
LOG: [Analyzer] Hotfix for various crashes in iterator checkers
The patch that introduces handling iterators implemented as pointers may
cause crash in some projects because pointer difference is mistakenly
handled as pointer decrement. (Similair case for iterators implemented
as class instances are already handled correctly.) This patch fixes this
issue.
The second case that causes crash is comparison of an iterator
implemented as pointer and a null-pointer. This patch contains a fix for
this issue as well.
The third case which causes crash is that the checker mistakenly
considers all integers as nonloc::ConcreteInt when handling an increment
or decrement of an iterator implemented as pointers. This patch adds a
fix for this too.
The last case where crashes were detected is when checking for success
of an std::advance() operation. Since the modeling of iterators
implemented as pointers is still incomplete this may result in an
assertion. This patch replaces the assertion with an early exit and
adds a FIXME there.
Differential Revision: https://reviews.llvm.org/D83295
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
clang/test/Analysis/iterator-modeling.cpp
clang/test/Analysis/iterator-range.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
index fd8cbd694b24..632de9e5dc83 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -272,6 +272,8 @@ void IteratorModeling::checkPostStmt(const BinaryOperator *BO,
handleComparison(C, BO, Result, LVal, RVal,
BinaryOperator::getOverloadedOperator(OK));
} else if (isRandomIncrOrDecrOperator(OK)) {
+ if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+ return;
handlePtrIncrOrDecr(C, BO->getLHS(),
BinaryOperator::getOverloadedOperator(OK), RVal);
}
@@ -461,6 +463,12 @@ void IteratorModeling::handleComparison(CheckerContext &C, const Expr *CE,
RPos = getIteratorPosition(State, RVal);
}
+ // If the value for which we just tried to set a new iterator position is
+ // an `SVal`for which no iterator position can be set then the setting was
+ // unsuccessful. We cannot handle the comparison in this case.
+ if (!LPos || !RPos)
+ return;
+
// We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
// instead.
if (RetVal.isUnknown()) {
@@ -599,6 +607,9 @@ void IteratorModeling::handlePtrIncrOrDecr(CheckerContext &C,
const Expr *Iterator,
OverloadedOperatorKind OK,
SVal Offset) const {
+ if (!Offset.getAs<DefinedSVal>())
+ return;
+
QualType PtrType = Iterator->getType();
if (!PtrType->isPointerType())
return;
@@ -612,13 +623,11 @@ void IteratorModeling::handlePtrIncrOrDecr(CheckerContext &C,
return;
SVal NewVal;
- if (OK == OO_Plus || OK == OO_PlusEqual)
+ if (OK == OO_Plus || OK == OO_PlusEqual) {
NewVal = State->getLValue(ElementType, Offset, OldVal);
- else {
- const llvm::APSInt &OffsetInt =
- Offset.castAs<nonloc::ConcreteInt>().getValue();
- auto &BVF = C.getSymbolManager().getBasicVals();
- SVal NegatedOffset = nonloc::ConcreteInt(BVF.getValue(-OffsetInt));
+ } else {
+ auto &SVB = C.getSValBuilder();
+ SVal NegatedOffset = SVB.evalMinus(Offset.castAs<NonLoc>());
NewVal = State->getLValue(ElementType, NegatedOffset, OldVal);
}
@@ -684,9 +693,14 @@ bool IteratorModeling::noChangeInAdvance(CheckerContext &C, SVal Iter,
const auto StateBefore = N->getState();
const auto *PosBefore = getIteratorPosition(StateBefore, Iter);
-
- assert(PosBefore && "`std::advance() should not create new iterator "
- "position but change existing ones");
+ // FIXME: `std::advance()` should not create a new iterator position but
+ // change existing ones. However, in case of iterators implemented as
+ // pointers the handling of parameters in `std::advance()`-like
+ // functions is still incomplete which may result in cases where
+ // the new position is assigned to the wrong pointer. This causes
+ // crash if we use an assertion here.
+ if (!PosBefore)
+ return false;
return PosBefore->getOffset() == PosAfter->getOffset();
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
index df8e379d1f20..dd014648eb6f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
@@ -169,6 +169,8 @@ void IteratorRangeChecker::checkPreStmt(const BinaryOperator *BO,
verifyDereference(C, LVal);
} else if (isRandomIncrOrDecrOperator(OK)) {
SVal RVal = State->getSVal(BO->getRHS(), C.getLocationContext());
+ if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+ return;
verifyRandomIncrOrDecr(C, BinaryOperator::getOverloadedOperator(OK), LVal,
RVal);
}
diff --git a/clang/test/Analysis/iterator-modeling.cpp b/clang/test/Analysis/iterator-modeling.cpp
index f19848b8dc93..0b76b0bfa723 100644
--- a/clang/test/Analysis/iterator-modeling.cpp
+++ b/clang/test/Analysis/iterator-modeling.cpp
@@ -1948,6 +1948,13 @@ void minus_equal_ptr_iterator(const cont_with_ptr_iterator<int> &c) {
clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning{{$c.end() - 2}}
}
+void minus_equal_ptr_iterator_variable(const cont_with_ptr_iterator<int> &c,
+ int n) {
+ auto i = c.end();
+
+ i -= n; // no-crash
+}
+
void plus_ptr_iterator(const cont_with_ptr_iterator<int> &c) {
auto i1 = c.begin();
@@ -1972,6 +1979,17 @@ void minus_ptr_iterator(const cont_with_ptr_iterator<int> &c) {
clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$c.end() - 2}}
}
+void ptr_iter_
diff (cont_with_ptr_iterator<int> &c) {
+ auto i0 = c.begin(), i1 = c.end();
+ ptr
diff _t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator<int> &c) {
+ auto i0 = c.begin();
+ if (i0 != nullptr) // no-crash
+ ++i0;
+}
+
void clang_analyzer_printState();
void print_state(std::vector<int> &V) {
diff --git a/clang/test/Analysis/iterator-range.cpp b/clang/test/Analysis/iterator-range.cpp
index 657ae89998e8..8d7103929047 100644
--- a/clang/test/Analysis/iterator-range.cpp
+++ b/clang/test/Analysis/iterator-range.cpp
@@ -935,3 +935,7 @@ void postfix_minus_assign_2_begin_ptr_iterator(
// expected-note at -1{{Iterator decremented ahead of its valid range}}
}
+void ptr_iter_
diff (cont_with_ptr_iterator<S> &c) {
+ auto i0 = c.begin(), i1 = c.end();
+ ptr
diff _t len = i1 - i0; // no-crash
+}
More information about the cfe-commits
mailing list