Fix 10590: container access out of bounds not found (#3560)

* Refactor container bounds check

* Use symbolic values

* Add test case

* Format

lib/checkstl.cpp

@@ -63,7 +63,43 @@ static const struct CWE CWE834(834U);   // Excessive Iteration
 
 static bool isElementAccessYield(const Library::Container::Yield& yield)
 {
-    return yield == Library::Container::Yield::ITEM || yield == Library::Container::Yield::AT_INDEX;
+    return contains({Library::Container::Yield::ITEM, Library::Container::Yield::AT_INDEX}, yield);
+}
+
+static bool containerYieldsElement(const Library::Container* container, const Token* parent)
+{
+    if (Token::Match(parent, ". %name% (")) {
+        Library::Container::Yield yield = container->getYield(parent->strAt(1));
+        if (isElementAccessYield(yield))
+            return true;
+    }
+    return false;
+}
+
+static const Token* getContainerIndex(const Library::Container* container, const Token* parent)
+{
+    if (Token::Match(parent, ". %name% (")) {
+        Library::Container::Yield yield = container->getYield(parent->strAt(1));
+        if (isElementAccessYield(yield) && !Token::simpleMatch(parent->tokAt(2), "( )"))
+            return parent->tokAt(2)->astOperand2();
+    }
+    if (!container->arrayLike_indexOp && !container->stdStringLike)
+        return nullptr;
+    if (Token::simpleMatch(parent, "["))
+        return parent->astOperand2();
+    return nullptr;
+}
+
+static const Token* getContainerFromSize(const Library::Container* container, const Token* tok)
+{
+    if (!tok)
+        return nullptr;
+    if (Token::Match(tok->tokAt(-2), ". %name% (")) {
+        Library::Container::Yield yield = container->getYield(tok->strAt(-1));
+        if (yield == Library::Container::Yield::SIZE)
+            return tok->tokAt(-2)->astOperand1();
+    }
+    return nullptr;
 }
 
 void CheckStl::outOfBounds()
@@ -74,6 +110,14 @@ void CheckStl::outOfBounds()
             if (!container)
                 continue;
             const Token * parent = astParentSkipParens(tok);
+            const Token* accessTok = parent;
+            if (Token::simpleMatch(accessTok, ".") && Token::simpleMatch(accessTok->astParent(), "("))
+                accessTok = accessTok->astParent();
+            if (astIsIterator(accessTok) && Token::simpleMatch(accessTok->astParent(), "+"))
+                accessTok = accessTok->astParent();
+            const Token* indexTok = getContainerIndex(container, parent);
+            if (indexTok == tok)
+                continue;
             for (const ValueFlow::Value &value : tok->values()) {
                 if (!value.isContainerSizeValue())
                     continue;
@@ -83,70 +127,60 @@ void CheckStl::outOfBounds()
                     continue;
                 if (!value.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
                     continue;
-                if (Token::Match(parent, ". %name% (") && isElementAccessYield(container->getYield(parent->strAt(1)))) {
+                if (value.intvalue == 0 && (indexTok || containerYieldsElement(container, parent))) {
-                    if (value.intvalue == 0) {
+                    std::string indexExpr;
-                        outOfBoundsError(parent->tokAt(2), tok->expressionString(), &value, parent->strAt(1), nullptr);
+                    if (indexTok && !indexTok->hasKnownValue())
-                        continue;
+                        indexExpr = indexTok->expressionString();
-                    }
+                    outOfBoundsError(accessTok, tok->expressionString(), &value, indexExpr, nullptr);
-
+                    continue;
-                    const Token* indexTok = parent->tokAt(2)->astOperand2();
+                }
-                    if (!indexTok)
+                if (indexTok) {
-                        continue;
                     std::vector<ValueFlow::Value> indexValues =
                         ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
                     if (!indexValues.empty()) {
                         outOfBoundsError(
-                            parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
+                            accessTok, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
                         continue;
                     }
                 }
-                if (Token::Match(tok, "%name% . %name% (") && container->getYield(tok->strAt(2)) == Library::Container::Yield::START_ITERATOR) {
+            }
-                    const Token *fparent = tok->tokAt(3)->astParent();
+            if (indexTok && !indexTok->hasKnownIntValue()) {
-                    const Token *other = nullptr;
+                const ValueFlow::Value* value =
-                    if (Token::simpleMatch(fparent, "+") && fparent->astOperand1() == tok->tokAt(3))
+                    ValueFlow::findValue(indexTok->values(), mSettings, [&](const ValueFlow::Value& v) {
-                        other = fparent->astOperand2();
+                    if (!v.isSymbolicValue())
-                    else if (Token::simpleMatch(fparent, "+") && fparent->astOperand2() == tok->tokAt(3))
+                        return false;
-                        other = fparent->astOperand1();
+                    if (v.isImpossible())
-                    if (other && other->hasKnownIntValue() && other->getKnownIntValue() > value.intvalue) {
+                        return false;
-                        outOfBoundsError(fparent, tok->expressionString(), &value, other->expressionString(), &other->values().back());
+                    if (v.intvalue < 0)
-                        continue;
+                        return false;
-                    } else if (other && !other->hasKnownIntValue() && value.isKnown() && value.intvalue==0) {
+                    const Token* containerTok = getContainerFromSize(container, v.tokvalue);
-                        outOfBoundsError(fparent, tok->expressionString(), &value, other->expressionString(), nullptr);
+                    if (!containerTok)
-                        continue;
+                        return false;
-                    }
+                    return containerTok->exprId() == tok->exprId();
-                }
+                });
-                if (!container->arrayLike_indexOp && !container->stdStringLike)
+                if (!value)
                     continue;
-                if (value.intvalue == 0 && Token::Match(parent, "[") && tok == parent->astOperand1()) {
+                outOfBoundsError(accessTok, tok->expressionString(), nullptr, indexTok->expressionString(), value);
-                    outOfBoundsError(parent, tok->expressionString(), &value, "", nullptr);
-                    continue;
-                }
-                if (container->arrayLike_indexOp && Token::Match(parent, "[")) {
-                    const Token* indexTok = parent->astOperand2();
-                    if (!indexTok)
-                        continue;
-                    std::vector<ValueFlow::Value> indexValues =
-                        ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
-                    if (!indexValues.empty()) {
-                        outOfBoundsError(
-                            parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
-                        continue;
-                    }
-                }
             }
         }
     }
 }
 
-static std::string indexValueString(const ValueFlow::Value& indexValue)
+static std::string indexValueString(const ValueFlow::Value& indexValue, const std::string& containerName = "")
 {
     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";
+    std::string indexString = MathLib::toString(indexValue.intvalue);
+    if (indexValue.isSymbolicValue()) {
+        indexString = containerName + ".size()";
+        if (indexValue.intvalue != 0)
+            indexString += "+" + MathLib::toString(indexValue.intvalue);
+    }
     if (indexValue.bound == ValueFlow::Value::Bound::Lower)
-        return "greater or equal to " + MathLib::toString(indexValue.intvalue);
+        return "greater or equal to " + indexString;
-    return MathLib::toString(indexValue.intvalue);
+    return indexString;
 }
 
 void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerName, const ValueFlow::Value *containerSize, const std::string &index, const ValueFlow::Value *indexValue)
@@ -158,9 +192,14 @@ void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerNa
     const std::string expression = tok ? tok->expressionString() : (containerName+"[x]");
 
     std::string errmsg;
-    if (!containerSize)
+    if (!containerSize) {
-        errmsg = "Out of bounds access in expression '" + expression + "'";
+        if (indexValue && indexValue->condition)
-    else if (containerSize->intvalue == 0) {
+            errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index +
+                     "' can have the value " + indexValueString(*indexValue, containerName) + ". Expression '" +
+                     expression + "' cause access out of bounds.";
+        else
+            errmsg = "Out of bounds access in expression '" + expression + "'";
+    } else if (containerSize->intvalue == 0) {
         if (containerSize->condition)
             errmsg = ValueFlow::eitherTheConditionIsRedundant(containerSize->condition) + " or expression '" + expression + "' cause access out of bounds.";
         else if (indexValue == nullptr && !index.empty())
@@ -1175,6 +1214,9 @@ void CheckStl::stlOutOfBounds()
                 continue;
             }
 
+            if (containerToken->hasKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE))
+                continue;
+
             // Is it a array like container?
             const Library::Container* container = containerToken->valueType() ? containerToken->valueType()->container : nullptr;
             if (!container)
@@ -2183,6 +2225,9 @@ void CheckStl::checkDereferenceInvalidIterator2()
             continue;
         }
 
+        if (Token::Match(tok, "%assign%"))
+            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())
@@ -2201,9 +2246,13 @@ void CheckStl::checkDereferenceInvalidIterator2()
                 continue;
             if (!value.isIteratorValue())
                 continue;
-            const bool isInvalidIterator = (value.isIteratorEndValue() && value.intvalue >= 0) || (value.isIteratorStartValue() && value.intvalue < 0);
+            bool isInvalidIterator = false;
             const ValueFlow::Value* cValue = nullptr;
-            if (!isInvalidIterator) {
+            if (value.isIteratorEndValue() && value.intvalue >= 0) {
+                isInvalidIterator = value.intvalue > 0;
+            } else if (value.isIteratorStartValue() && value.intvalue < 0) {
+                isInvalidIterator = true;
+            } else {
                 auto it = std::find_if(contValues.begin(), contValues.end(), [&](const ValueFlow::Value& c) {
                     if (value.isIteratorStartValue() && value.intvalue >= c.intvalue)
                         return true;
@@ -2214,17 +2263,41 @@ void CheckStl::checkDereferenceInvalidIterator2()
                 if (it == contValues.end())
                     continue;
                 cValue = &*it;
+                if (value.isIteratorStartValue() && value.intvalue > cValue->intvalue)
+                    isInvalidIterator = true;
             }
             bool inconclusive = false;
             bool unknown = false;
-            if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings)) {
+            const Token* emptyAdvance = nullptr;
+            const Token* advanceIndex = nullptr;
+            if (cValue && cValue->intvalue == 0) {
+                if (Token::Match(tok->astParent(), "+|-") && astIsIntegral(tok->astSibling(), false)) {
+                    if (tok->astSibling() && tok->astSibling()->hasKnownIntValue()) {
+                        if (tok->astSibling()->values().front().intvalue == 0)
+                            continue;
+                    } else {
+                        advanceIndex = tok->astSibling();
+                    }
+                    emptyAdvance = tok->astParent();
+                } else if (Token::Match(tok->astParent(), "++|--")) {
+                    emptyAdvance = tok->astParent();
+                }
+            }
+            if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings) && !isInvalidIterator && !emptyAdvance) {
                 if (!unknown)
                     continue;
                 inconclusive = true;
             }
             if (cValue) {
                 const ValueFlow::Value& lValue = getLifetimeObjValue(tok, true);
-                outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value);
+                if (emptyAdvance)
+                    outOfBoundsError(emptyAdvance,
+                                     lValue.tokvalue->expressionString(),
+                                     cValue,
+                                     advanceIndex ? advanceIndex->expressionString() : "",
+                                     nullptr);
+                else
+                    outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value);
             } else {
                 dereferenceInvalidIteratorError(tok, &value, inconclusive);
             }

lib/valueflow.cpp

@@ -2955,7 +2955,7 @@ std::vector<ValueFlow::Value> getLifetimeObjValues(const Token *tok, bool inconc
             return false;
         if (!inconclusive && v.isInconclusive())
             return false;
-        if (!v.tokvalue->variable())
+        if (!v.tokvalue)
             return false;
         return true;
     };

test/teststl.cpp

@@ -40,6 +40,7 @@ private:
         LOAD_LIB_2(settings.library, "std.cfg");
 
         TEST_CASE(outOfBounds);
+        TEST_CASE(outOfBoundsSymbolic);
         TEST_CASE(outOfBoundsIndexExpression);
         TEST_CASE(outOfBoundsIterator);
 
@@ -355,7 +356,7 @@ private:
                     "    if(v.at(b?42:0)) {}\n"
                     "}\n");
         ASSERT_EQUALS(
-            "test.cpp:3:error:Out of bounds access in expression 'v.at(b?42:0)' because 'v' is empty and 'at' may be non-zero.\n",
+            "test.cpp:3:error:Out of bounds access in expression 'v.at(b?42:0)' because 'v' is empty and 'b?42:0' may be non-zero.\n",
             errout.str());
 
         checkNormal("void f(std::vector<int> v, bool b){\n"
@@ -363,7 +364,7 @@ private:
                     "        if(v.at(b?42:0)) {}\n"
                     "}\n");
         ASSERT_EQUALS(
-            "test.cpp:3:warning:Either the condition 'v.size()==1' is redundant or v size can be 1. Expression 'v.at' cause access out of bounds.\n"
+            "test.cpp:3:warning:Either the condition 'v.size()==1' is redundant or v size can be 1. Expression 'v.at(b?42:0)' cause access out of bounds.\n"
             "test.cpp:2:note:condition 'v.size()==1'\n"
             "test.cpp:3:note:Access out of bounds\n",
             errout.str());
@@ -528,7 +529,8 @@ private:
                     "    std::vector<int> * pv = &v;\n"
                     "    return (*pv).at(42);\n"
                     "}\n");
-        ASSERT_EQUALS("test.cpp:4:error:Out of bounds access in expression '(*pv).at(42)' because '*pv' is empty and 'at' may be non-zero.\n", errout.str());
+        ASSERT_EQUALS("test.cpp:4:error:Out of bounds access in expression '(*pv).at(42)' because '*pv' is empty.\n",
+                      errout.str());
 
         checkNormal("std::string f(const char* DirName) {\n"
                     "  if (DirName == nullptr)\n"
@@ -645,6 +647,18 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void outOfBoundsSymbolic()
+    {
+        check("void foo(std::string textline, int col) {\n"
+              "    if(col > textline.size())\n"
+              "        return false;\n"
+              "    int x = textline[col];\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:2] -> [test.cpp:4]: (warning) Either the condition 'col>textline.size()' is redundant or 'col' can have the value textline.size(). Expression 'textline[col]' cause access out of bounds.\n",
+            errout.str());
+    }
+
     void outOfBoundsIndexExpression() {
         setMultiline();
 
@@ -1516,21 +1530,24 @@ private:
               "void foo() {\n"
               "    (void)std::find(f().begin(), g().end(), 0);\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n",
+                      errout.str());
 
         check("std::vector<int>& f();\n"
               "std::vector<int>& g();\n"
               "void foo() {\n"
               "    if(f().begin() == g().end()) {}\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n",
+                      errout.str());
 
         check("std::vector<int>& f();\n"
               "std::vector<int>& g();\n"
               "void foo() {\n"
               "    auto size = f().end() - g().begin();\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n",
+                      errout.str());
 
         check("struct A {\n"
               "    std::vector<int>& f();\n"
@@ -1576,7 +1593,7 @@ private:
         check("std::vector<int>& f();\n"
               "std::vector<int>& g();\n"
               "void foo() {\n"
-              "    auto it = f().end();\n"
+              "    auto it = f().end() - 1;\n"
               "    f().begin() - it\n"
               "    f().begin()+1 - it\n"
               "    f().begin() - (it + 1)\n"
@@ -1596,7 +1613,7 @@ private:
               "    (void)std::find(begin(f()), end(f()) - 1, 0);\n"
               "    (void)std::find(begin(f()) + 1, end(f()) - 1, 0);\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:10]: (error) Dereference of an invalid iterator: f().end()+1\n", errout.str());
 
         check("std::vector<int>& f();\n"
               "std::vector<int>& g();\n"
@@ -1606,7 +1623,9 @@ private:
               "    if(f().begin()+1 == f().end()) {}\n"
               "    if(f().begin()+1 == f().end()+1) {}\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:5]: (error) Dereference of an invalid iterator: f().end()+1\n"
+                      "[test.cpp:7]: (error) Dereference of an invalid iterator: f().end()+1\n",
+                      errout.str());
 
         check("template<int N>\n"
               "std::vector<int>& f();\n"
@@ -1824,8 +1843,9 @@ private:
               "       foo[ii] = 0;\n"
               "    }\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:6]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty.\n"
+        ASSERT_EQUALS(
-                      "[test.cpp:6]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout.str());
+            "[test.cpp:6]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty and 'ii' may be non-zero.\n",
+            errout.str());
 
         check("void foo(std::vector<int> foo) {\n"
               "    for (unsigned int ii = 0; ii <= foo.size(); ++ii) {\n"
@@ -1928,7 +1948,9 @@ private:
                   "        }\n"
                   "    }\n"
                   "}");
-            ASSERT_EQUALS("[test.cpp:11]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty.\n", errout.str());
+            ASSERT_EQUALS(
+                "[test.cpp:11]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty and 'ii' may be non-zero.\n",
+                errout.str());
         }
 
         {
@@ -2912,28 +2934,21 @@ private:
         // ok (array-like pointer)
         check("void f(std::set<int> *s)\n"
               "{\n"
-              "    if (s[0].find(12) != s.end()) { }\n"
+              "    if (s[0].find(12) != s->end()) { }\n"
               "}");
         ASSERT_EQUALS("", errout.str());
 
         // ok (array)
         check("void f(std::set<int> s [10])\n"
               "{\n"
-              "    if (s[0].find(123) != s.end()) { }\n"
+              "    if (s[0].find(123) != s->end()) { }\n"
               "}");
         ASSERT_EQUALS("", errout.str());
 
         // ok (undefined length array)
         check("void f(std::set<int> s [])\n"
               "{\n"
-              "    if (s[0].find(123) != s.end()) { }\n"
+              "    if (s[0].find(123) != s->end()) { }\n"
-              "}");
-        ASSERT_EQUALS("", errout.str());
-
-        // ok (vector)
-        check("void f(std::vector<std::set<int> > s)\n"
-              "{\n"
-              "    if (s[0].find(123) != s.end()) { }\n"
               "}");
         ASSERT_EQUALS("", errout.str());