[PATCH] D83295: [Analyzer] Hotfix for various crashes in iterator checkers

[Balogh , Ádám via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa59d4ae4313c: [Analyzer] Hotfix for various crashes in 
iterator checkers (authored by baloghadamsoftware).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83295/new/

https://reviews.llvm.org/D83295

Files:
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/test/Analysis/iterator-modeling.cpp
  clang/test/Analysis/iterator-range.cpp


Index: clang/test/Analysis/iterator-range.cpp
===================================================================
--- clang/test/Analysis/iterator-range.cpp
+++ clang/test/Analysis/iterator-range.cpp
@@ -935,3 +935,7 @@
           // expected-note@-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();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
Index: clang/test/Analysis/iterator-modeling.cpp
===================================================================
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1948,6 +1948,13 @@
   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 @@
   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();
+  ptrdiff_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) {
Index: clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
@@ -169,6 +169,8 @@
     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);
   }
Index: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -272,6 +272,8 @@
     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 @@
     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 @@
                                            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 @@
     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 @@
 
   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();
 }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits