[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

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

baloghadamsoftware added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423
+
+void IteratorPastEndChecker::handleComparison(CheckerContext &C,
+                                              const SVal &LVal,
----------------
NoQ wrote:
> a.sidorin wrote:
> > What will happen if we write something like this:
> > ```
> > bool Eq1 = it1 == it2;
> > bool Eq2 = it3 == it4;
> > if (Eq1) {...}?
> > ```
> > 
> > As I understand, we'll assume the second condition instead of first.
> Had a look. So the problem is, we obtain the result of the comparison as a symbol, from which it is too hard to recover the operands in order to move iterator position data from one value to another.
> 
> Normally we obtain a simple SymbolConjured for the return value of the `operator==()` call (or, similarly, `operator!=()`). For plain-value iterators (eg. `typedef T *iterator`) we might be obtaining an actual binary symbolic expression, but even then it's not quite clear how to obtain operands (the structure of the expression might have changed due to algebraic simplifications). Additionally, LHS and RHS aren't necessarily symbols (they might be semi-concrete), so composing symbolic expressions from them in general is problematic with our symbol hierarchy, which is rarely a problem for numbers but for structural symbols it'd be a mess.
> 
> For now i suggest, instead of storing only the last LHS and RHS, to save a map from symbols (which are results of comparisons) to (LHS value, RHS value) pairs. This map should, apart from the obvious, be cleaned up whenever one of the iterators in the pair gets mutated (incremented or decremented). This should take care of the problem Alexey points out, and should work with semi-concrete stuff.
> 
> For the future i suggest to let users construct their own symbols and symbolic expressions more easily. In fact, if only we had all iterators as regions, we should have probably used SymbolMetadata for this purpose: it's easy to both recover the parent region from it and use it in symbolic expressions. We could also deprecate the confusing structural symbols (provide default-bound lazy compound values for conjured structures instead), and then it'd be possible to transition to SymbolMetadata entirely.
Thank you for the suggestion. I am not sure if I fully understand you. If I create a map where the key is the resulting symbol of the comparison, it will not work because evalAssume is called for the innermost comparison. So if the body of operator== (or operator!=) is inlined, then I get a binary symbolic expression in evalAssume, not the SymbolConjured. This binary Symbolic expression is a comparison of the internals of the iterators, e.g. the internal pointer. So the key will not match any LHS and RHS value pair in the map. I also thought on such solution earlier but I dismissed it because of this.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:580
+  C.addTransition(stateFound);
+  C.addTransition(stateNotFound);
+}
----------------
NoQ wrote:
> NoQ wrote:
> > Ouch, i have one more concern, which can be expressed with the following false-positive test which currently fails:
> > 
> > ```
> > void foo() {
> >   std::vector<int> vec;
> >   vec.push_back(2016);
> >   auto i = vec.find(vec.begin(), vec.end(), 2016);
> >   *i; // no-warning
> > }
> > ```
> > 
> > Not instantly sure what to do with this. You can avoid state splits until you are actually sure if both branches are possible, but that'd suppress a lot of useful positives. Such positives could be suppressed with assertions, of course, but i'd still hope there aren't too many of those.
> I mean, `std::find(...` ><
False positives can occur whenever we are sure that we will find the element so we do not check for the result to be equal with end().


https://reviews.llvm.org/D25660