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

2020-07-16 Thread 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 ) {
+  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 n) {
+  auto i = c.end();
+
+  i -= n; // no-crash
+}
+
 void plus_ptr_iterator(const cont_with_ptr_iterator ) {
   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 ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator ) {
+  auto i0 = c.begin();
+  if (i0 != nullptr) // no-crash
+++i0;
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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())
+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  =
-  Offset.castAs().getValue();
-auto  = C.getSymbolManager().getBasicVals();
-SVal NegatedOffset = nonloc::ConcreteInt(BVF.getValue(-OffsetInt));
+  } else {
+auto  = C.getSValBuilder();
+SVal NegatedOffset = SVB.evalMinus(Offset.castAs());
 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

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

2020-07-16 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 278394.
baloghadamsoftware added a comment.

Protection against `Unknown` added for pointer increments and decrements. No 
more crashes on `LLVM/Clang`.


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 ) {
+  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 n) {
+  auto i = c.end();
+
+  i -= n; // no-crash
+}
+
 void plus_ptr_iterator(const cont_with_ptr_iterator ) {
   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 ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator ) {
+  auto i0 = c.begin();
+  if (i0 != nullptr) // no-crash
+++i0;
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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())
+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  =
-  Offset.castAs().getValue();
-auto  = C.getSymbolManager().getBasicVals();
-SVal NegatedOffset = nonloc::ConcreteInt(BVF.getValue(-OffsetInt));
+  } else {
+auto  = C.getSValBuilder();
+SVal NegatedOffset = SVB.evalMinus(Offset.castAs());
 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

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

2020-07-13 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 277340.
baloghadamsoftware edited the summary of this revision.
baloghadamsoftware added a comment.

Two more crashes detected, fixes for them added.


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 ) {
+  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 n) {
+  auto i = c.end();
+
+  i -= n; // no-crash
+}
+
 void plus_ptr_iterator(const cont_with_ptr_iterator ) {
   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 ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator ) {
+  auto i0 = c.begin();
+  if (i0 != nullptr) // no-crash
+++i0;
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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()) {
@@ -615,10 +623,8 @@
   if (OK == OO_Plus || OK == OO_PlusEqual)
 NewVal = State->getLValue(ElementType, Offset, OldVal);
   else {
-const llvm::APSInt  =
-  Offset.castAs().getValue();
-auto  = C.getSymbolManager().getBasicVals();
-SVal NegatedOffset = nonloc::ConcreteInt(BVF.getValue(-OffsetInt));
+auto  = C.getSValBuilder();
+SVal NegatedOffset = SVB.evalMinus(Offset.castAs());
 NewVal = State->getLValue(ElementType, NegatedOffset, OldVal);
   }
 
@@ -684,9 +690,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();
 }

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

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus accepted this revision.
Szelethus added a comment.

LGTM, but if we knowingly a subpar solution, we should make that clear in the 
surrounding code. I know the followup patch is around the corner, but just to 
be sure.




Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276
   } else if (isRandomIncrOrDecrOperator(OK)) {
+if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+  return;
 handlePtrIncrOrDecr(C, BO->getLHS(),

baloghadamsoftware wrote:
> gamesh411 wrote:
> > Szelethus wrote:
> > > This doesn't look symmetrical. How does this patch interact with D83190?
> > I see that patch D83190 will need not only to be rebased, but modified 
> > slightly to take this early checking into account. I will refer to this 
> > review over there.
> This //is// not symmetrical. Since this patch is a hotfix for three bugs, all 
> of them cause crashes it is more urgent to land. The other patch should be 
> rebased on it.
Please add a FIXME before commiting, if we knowingly add a hack.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:466-470
+  // 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;

Is this always okay? Shouldn't we check what the reason was behind the failure 
to retrieve the position? Is a `TODO: ` at the very least appropriate?



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp:172-173
 SVal RVal = State->getSVal(BO->getRHS(), C.getLocationContext());
+if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+  return;
 verifyRandomIncrOrDecr(C, BinaryOperator::getOverloadedOperator(OK), LVal,

FIXME:


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

https://reviews.llvm.org/D83295



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

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

2020-07-09 Thread Endre Fülöp via Phabricator via cfe-commits 
gamesh411 added a comment.

@Szelethus thanks for being watchful, appreciated c:


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

https://reviews.llvm.org/D83295



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

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

2020-07-09 Thread Endre Fülöp via Phabricator via cfe-commits 
gamesh411 accepted this revision.
gamesh411 added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276
   } else if (isRandomIncrOrDecrOperator(OK)) {
+if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+  return;
 handlePtrIncrOrDecr(C, BO->getLHS(),

Szelethus wrote:
> This doesn't look symmetrical. How does this patch interact with D83190?
I see that patch D83190 will need not only to be rebased, but modified slightly 
to take this early checking into account. I will refer to this review over 
there.


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

https://reviews.llvm.org/D83295



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

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

2020-07-09 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276
   } else if (isRandomIncrOrDecrOperator(OK)) {
+if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+  return;
 handlePtrIncrOrDecr(C, BO->getLHS(),

Szelethus wrote:
> This doesn't look symmetrical. How does this patch interact with D83190?
This //is// not symmetrical. Since this patch is a hotfix for three bugs, all 
of them cause crashes it is more urgent to land. The other patch should be 
rebased on it.


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

https://reviews.llvm.org/D83295



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

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

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276
   } else if (isRandomIncrOrDecrOperator(OK)) {
+if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+  return;
 handlePtrIncrOrDecr(C, BO->getLHS(),

This doesn't look symmetrical. How does this patch interact with D83190?


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

https://reviews.llvm.org/D83295



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

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

2020-07-09 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 276684.
baloghadamsoftware edited the summary of this revision.
baloghadamsoftware added a comment.

Test added for the third fix in this patch.


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 ) {
+  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
@@ -1972,6 +1972,17 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // 
expected-warning{{$c.end() - 2}}
 }
 
+void ptr_iter_diff(cont_with_ptr_iterator ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator ) {
+  auto i0 = c.begin();
+  if (i0 != nullptr) // no-crash
+++i0;
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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()) {


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 ) {
+  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
@@ -1972,6 +1972,17 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$c.end() - 2}}
 }
 
+void ptr_iter_diff(cont_with_ptr_iterator ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
+void ptr_iter_cmp_nullptr(cont_with_ptr_iterator ) {
+  auto i0 = c.begin();
+  if (i0 != nullptr) // no-crash
+++i0;
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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,

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

2020-07-07 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, gamesh411, martong, balazske.
baloghadamsoftware added a project: clang.
Herald added subscribers: ASDenysPetrov, steakhal, Charusso, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, 
whisperity.
Herald added a reviewer: Szelethus.

The patch that introduces handling pointers implemented as iterators 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.


Repository:
  rG LLVM Github Monorepo

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 ) {
+  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
@@ -1972,6 +1972,11 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // 
expected-warning{{$c.end() - 2}}
 }
 
+void ptr_iter_diff(cont_with_ptr_iterator ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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,9 @@
 RPos = getIteratorPosition(State, RVal);
   }
 
+  if (!LPos || !RPos)
+return;
+
   // We cannot make assumptions on `UnknownVal`. Let us conjure a symbol
   // instead.
   if (RetVal.isUnknown()) {


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 ) {
+  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
@@ -1972,6 +1972,11 @@
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$c.end() - 2}}
 }
 
+void ptr_iter_diff(cont_with_ptr_iterator ) {
+  auto i0 = c.begin(), i1 = c.end();
+  ptrdiff_t len = i1 - i0; // no-crash
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
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,