Add outOfBounds check for iterators to containers (#2752)

lib/checkstl.cpp

@@ -31,7 +31,9 @@
 #include "pathanalysis.h"
 #include "valueflow.h"
 
+#include <algorithm>
 #include <cstddef>
+#include <iterator>
 #include <list>
 #include <map>
 #include <set>
@@ -118,6 +120,15 @@ void CheckStl::outOfBounds()
     }
 }
 
+static std::string indexValueString(const ValueFlow::Value& indexValue)
+{
+    if (indexValue.isIteratorStartValue())
+        return "at position " + MathLib::toString(indexValue.intvalue) + " from the beginning";
+    if (indexValue.isIteratorEndValue())
+        return "at position " + MathLib::toString(-indexValue.intvalue) + " from the end";
+    return MathLib::toString(indexValue.intvalue);
+}
+
 void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerName, const ValueFlow::Value *containerSize, const std::string &index, const ValueFlow::Value *indexValue)
 {
     // Do not warn if both the container size and index value are possible
@@ -140,9 +151,9 @@ void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerNa
         if (containerSize->condition)
             errmsg = ValueFlow::eitherTheConditionIsRedundant(containerSize->condition) + " or $symbol size can be " + MathLib::toString(containerSize->intvalue) + ". Expression '" + expression + "' cause access out of bounds.";
         else if (indexValue->condition)
-            errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index + "' can have the value " + MathLib::toString(indexValue->intvalue) + ". Expression '" + expression + "' cause access out of bounds.";
+            errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index + "' can have the value " + indexValueString(*indexValue) + ". Expression '" + expression + "' cause access out of bounds.";
         else
-            errmsg = "Out of bounds access in '" + expression + "', if '$symbol' size is " + MathLib::toString(containerSize->intvalue) + " and '" + index + "' is " + MathLib::toString(indexValue->intvalue);
+            errmsg = "Out of bounds access in '" + expression + "', if '$symbol' size is " + MathLib::toString(containerSize->intvalue) + " and '" + index + "' is " + indexValueString(*indexValue);
     } else {
         // should not happen
         return;
@@ -2013,6 +2024,7 @@ void CheckStl::checkDereferenceInvalidIterator()
     }
 }
 
+
 void CheckStl::checkDereferenceInvalidIterator2()
 {
     const bool printInconclusive = (mSettings->inconclusive);
@@ -2023,6 +2035,16 @@ void CheckStl::checkDereferenceInvalidIterator2()
             continue;
         }
 
+        std::vector<ValueFlow::Value> contValues;
+        std::copy_if(tok->values().begin(), tok->values().end(), std::back_inserter(contValues), [&](const ValueFlow::Value& value) {
+            if (value.isImpossible())
+                return false;
+            if (!printInconclusive && value.isInconclusive())
+                return false;
+            return value.isContainerSizeValue();
+        });
+
+
         // Can iterator point to END or before START?
         for (const ValueFlow::Value& value:tok->values()) {
             if (value.isImpossible())
@@ -2031,17 +2053,33 @@ void CheckStl::checkDereferenceInvalidIterator2()
                 continue;
             if (!value.isIteratorValue())
                 continue;
-            if (value.isIteratorEndValue() && value.intvalue < 0)
+            const bool isInvalidIterator = (value.isIteratorEndValue() && value.intvalue >= 0) || (value.isIteratorStartValue() && value.intvalue < 0);
-                continue;
+            const ValueFlow::Value* cValue = nullptr;
-            if (value.isIteratorStartValue() && value.intvalue >= 0)
+            if (!isInvalidIterator) {
-                continue;
+                auto it = std::find_if(contValues.begin(), contValues.end(), [&](const ValueFlow::Value& c) {
+                    if (value.isIteratorStartValue() && value.intvalue >= c.intvalue)
+                        return true;
+                    if (value.isIteratorEndValue() && -value.intvalue > c.intvalue)
+                        return true;
+                    return false;
+                });
+                if (it == contValues.end())
+                    continue;
+                cValue = &*it;
+            }
+            bool inconclusive = false;
             bool unknown = false;
             if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings)) {
-                if (unknown)
+                if (!unknown)
-                    dereferenceInvalidIteratorError(tok, &value, true);
+                    continue;
-                continue;
+                inconclusive = true;
+            }
+            if (cValue) {
+                const ValueFlow::Value& lValue = getLifetimeObjValue(tok, true);
+                outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value);
+            } else {
+                dereferenceInvalidIteratorError(tok, &value, inconclusive);
             }
-            dereferenceInvalidIteratorError(tok, &value, false);
         }
     }
 }

lib/valueflow.cpp

@@ -2947,13 +2947,13 @@ std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, Error
     return msg;
 }
 
-ValueFlow::Value getLifetimeObjValue(const Token *tok)
+ValueFlow::Value getLifetimeObjValue(const Token *tok, bool inconclusive)
 {
     ValueFlow::Value result;
-    auto pred = [](const ValueFlow::Value &v) {
+    auto pred = [&](const ValueFlow::Value &v) {
         if (!v.isLocalLifetimeValue())
             return false;
-        if (v.isInconclusive())
+        if (!inconclusive && v.isInconclusive())
             return false;
         if (!v.tokvalue->variable())
             return false;
@@ -4104,7 +4104,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
                 continue;
 
             // Lhs should be a variable
-            if (!tok->astOperand1() || !tok->astOperand1()->varId() || tok->astOperand1()->hasKnownValue())
+            if (!tok->astOperand1() || !tok->astOperand1()->varId())
                 continue;
             const int varid = tok->astOperand1()->varId();
             if (aliased.find(varid) != aliased.end())
@@ -4118,6 +4118,20 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
                 continue;
 
             std::list<ValueFlow::Value> values = truncateValues(tok->astOperand2()->values(), tok->astOperand1()->valueType(), settings);
+            // Remove known values
+            std::set<ValueFlow::Value::ValueType> types;
+            if (tok->astOperand1()->hasKnownValue()) {
+                for(const ValueFlow::Value& value:tok->astOperand1()->values()) {
+                    if (value.isKnown())
+                        types.insert(value.valueType);
+                }
+            }
+            values.remove_if([&](const ValueFlow::Value& value) { return types.count(value.valueType) > 0;});
+            // Remove container size if its not a container
+            if (!astIsContainer(tok->astOperand2()))
+                values.remove_if([&](const ValueFlow::Value& value) { return value.valueType == ValueFlow::Value::CONTAINER_SIZE;});
+            if (values.empty())
+                continue;
             const bool constValue = isLiteralNumber(tok->astOperand2(), tokenlist->isCPP());
             const bool init = var->nameToken() == tok->astOperand1();
             valueFlowForwardAssign(tok->astOperand2(), var, values, constValue, init, tokenlist, errorLogger, settings);
@@ -5686,7 +5700,13 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer {
     ContainerVariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector<const Variable*> paliases, const TokenList* t)
         : VariableForwardAnalyzer(v, val, std::move(paliases), t) {}
 
+    virtual bool match(const Token* tok) const OVERRIDE {
+        return tok->varId() == var->declarationId() || (astIsIterator(tok) && isAliasOf(tok, var->declarationId()));
+    }
+
     virtual Action isWritable(const Token* tok) const OVERRIDE {
+        if (astIsIterator(tok))
+            return Action::None;
         const ValueFlow::Value* value = getValue(tok);
         if (!value)
             return Action::None;
@@ -5743,6 +5763,9 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer {
 
     virtual Action isModified(const Token* tok) const OVERRIDE {
         Action read = Action::Read;
+        // An iterator wont change the container size
+        if (astIsIterator(tok))
+            return read;
         if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok))
             return Action::Invalid;
         if (isLikelyStreamRead(isCPP(), tok->astParent()))
@@ -5900,6 +5923,8 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold
             continue;
         if (!Token::Match(var->nameToken(), "%name% ;"))
             continue;
+        if (!astIsContainer(var->nameToken()))
+            continue;
         if (var->nameToken()->hasKnownValue())
             continue;
         ValueFlow::Value value(0);

lib/valueflow.h

@@ -374,6 +374,6 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val);
 
 std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ValueFlow::Value::ErrorPath &errorPath);
 
-ValueFlow::Value getLifetimeObjValue(const Token *tok);
+ValueFlow::Value getLifetimeObjValue(const Token *tok, bool inconclusive = false);
 
 #endif // valueflowH

test/teststl.cpp

@@ -42,6 +42,7 @@ private:
 
         TEST_CASE(outOfBounds);
         TEST_CASE(outOfBoundsIndexExpression);
+        TEST_CASE(outOfBoundsIterator);
 
         TEST_CASE(iterator1);
         TEST_CASE(iterator2);
@@ -371,6 +372,50 @@ private:
                     "}");
         ASSERT_EQUALS("test.cpp:2:error:Out of bounds access of s, index 'x*s.size()' is out of bounds.\n", errout.str());
     }
+    void outOfBoundsIterator() {
+        check("int f() {\n"
+              "    std::vector<int> v;\n"
+              "    auto it = v.begin();\n"
+              "    return *it;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Out of bounds access in expression 'it' because 'v' is empty.\n",
+                      errout.str());
+
+        check("int f() {\n"
+              "    std::vector<int> v;\n"
+              "    v.push_back(0);\n"
+              "    auto it = v.begin() + 1;\n"
+              "    return *it;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Out of bounds access in 'it', if 'v' size is 1 and 'it' is at position 1 from the beginning\n",
+                      errout.str());
+
+        check("int f() {\n"
+              "    std::vector<int> v;\n"
+              "    v.push_back(0);\n"
+              "    auto it = v.end() - 1;\n"
+              "    return *it;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("int f() {\n"
+              "    std::vector<int> v;\n"
+              "    v.push_back(0);\n"
+              "    auto it = v.end() - 2;\n"
+              "    return *it;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Out of bounds access in 'it', if 'v' size is 1 and 'it' is at position 2 from the end\n",
+                      errout.str());
+
+        check("void g(int);\n"
+              "void f(std::vector<int> x) {\n"
+              "    std::map<int, int> m;\n"
+              "    if (!m.empty()) {\n"
+              "        g(m.begin()->second);\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 
     void iterator1() {
         check("void f()\n"
@@ -1243,9 +1288,7 @@ private:
               "}\n");
         ASSERT_EQUALS("", errout.str());
 
-        check("void f() {\n"
+        check("void f(std::list<int*> a, std::list<int*> b) {\n"
-              "  std::list<int*> a;\n"
-              "  std::list<int*> b;\n"
               "  if (*a.begin() == *b.begin()) {}\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
@@ -1932,9 +1975,8 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (error) Iterator 'it' used after element has been erased.\n", errout.str());
 
-        check("void f()\n"
+        check("void f(std::set<int> foo)\n"
               "{\n"
-              "    std::set<int> foo;\n"
               "    std::set<int>::iterator it = foo.begin();\n"
               "    foo.erase(*it);\n"
               "}");