New check: eraseIteratorOutOfBounds (#5913)

lib/astutils.cpp

@@ -3027,8 +3027,10 @@ const Token* findExpressionChangedSkipDeadCode(const Token* expr,
 const Token* getArgumentStart(const Token* ftok)
 {
     const Token* tok = ftok;
-    if (Token::Match(tok, "%name% (|{"))
+    if (Token::Match(tok, "%name% (|{|)"))
         tok = ftok->next();
+    while (Token::simpleMatch(tok, ")"))
+        tok = tok->next();
     if (!Token::Match(tok, "(|{|["))
         return nullptr;
     const Token* startTok = tok->astOperand2();

lib/checkstl.cpp

@@ -3119,6 +3119,77 @@ void CheckStl::knownEmptyContainer()
     }
 }
 
+void CheckStl::eraseIteratorOutOfBoundsError(const Token *ftok, const Token* itertok, const ValueFlow::Value* val)
+{
+    if (!ftok || !itertok || !val) {
+        reportError(ftok, Severity::error, "eraseIteratorOutOfBounds",
+                    "Calling function 'erase()' on the iterator 'iter' which is out of bounds.", CWE628, Certainty::normal);
+        reportError(ftok, Severity::warning, "eraseIteratorOutOfBoundsCond",
+                    "Either the condition 'x' is redundant or function 'erase()' is called on the iterator 'iter' which is out of bounds.", CWE628, Certainty::normal);
+        return;
+    }
+    const std::string& func = ftok->str();
+    const std::string iter = itertok->expressionString();
+
+    const bool isConditional = val->isPossible();
+    std::string msg;
+    if (isConditional) {
+        msg = ValueFlow::eitherTheConditionIsRedundant(val->condition) + " or function '" + func + "()' is called on the iterator '" + iter + "' which is out of bounds.";
+    } else {
+        msg = "Calling function '" + func + "()' on the iterator '" + iter + "' which is out of bounds.";
+    }
+
+    const Severity severity = isConditional ? Severity::warning : Severity::error;
+    const std::string id = isConditional ? "eraseIteratorOutOfBoundsCond" : "eraseIteratorOutOfBounds";
+    reportError(ftok, severity,
+                id,
+                msg, CWE628, Certainty::normal);
+}
+
+static const ValueFlow::Value* getOOBIterValue(const Token* tok, const ValueFlow::Value* sizeVal)
+{
+    auto it = std::find_if(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& v) {
+        if (v.isPossible() || v.isKnown()) {
+            switch (v.valueType) {
+            case ValueFlow::Value::ValueType::ITERATOR_END:
+                return v.intvalue >= 0;
+            case ValueFlow::Value::ValueType::ITERATOR_START:
+                return (v.intvalue < 0) || (sizeVal && v.intvalue >= sizeVal->intvalue);
+            default:
+                break;
+            }
+        }
+        return false;
+    });
+    return it != tok->values().end() ? &*it : nullptr;
+}
+
+void CheckStl::eraseIteratorOutOfBounds()
+{
+    logChecker("CheckStl::eraseIteratorOutOfBounds");
+    for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) {
+        for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) {
+
+            if (!tok->valueType())
+                continue;
+            const Library::Container* container = tok->valueType()->container;
+            if (!container || !astIsLHS(tok) || !Token::simpleMatch(tok->astParent(), "."))
+                continue;
+            const Token* const ftok = tok->astParent()->astOperand2();
+            const Library::Container::Action action = container->getAction(ftok->str());
+            if (action != Library::Container::Action::ERASE)
+                continue;
+            const std::vector<const Token*> args = getArguments(ftok);
+            if (args.size() != 1) // TODO: check range overload
+                continue;
+
+            const ValueFlow::Value* sizeVal = tok->getKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE);
+            if (const ValueFlow::Value* errVal = getOOBIterValue(args[0], sizeVal))
+                eraseIteratorOutOfBoundsError(ftok, args[0], errVal);
+        }
+    }
+}
+
 static bool isMutex(const Variable* var)
 {
     const Token* tok = Token::typeDecl(var->nameToken()).first;

lib/checkstl.h

@@ -78,6 +78,7 @@ class CPPCHECKLIB CheckStl : public Check {
         checkStl.mismatchingContainers();
         checkStl.mismatchingContainerIterator();
         checkStl.knownEmptyContainer();
+        checkStl.eraseIteratorOutOfBounds();
 
         checkStl.stlBoundaries();
         checkStl.checkDereferenceInvalidIterator();
@@ -183,6 +184,8 @@ class CPPCHECKLIB CheckStl : public Check {
 
     void knownEmptyContainer();
 
+    void eraseIteratorOutOfBounds();
+
     void checkMutexes();
 
     bool isContainerSize(const Token *containerToken, const Token *expr) const;
@@ -234,6 +237,8 @@ class CPPCHECKLIB CheckStl : public Check {
 
     void knownEmptyContainerError(const Token *tok, const std::string& algo);
 
+    void eraseIteratorOutOfBoundsError(const Token* ftok, const Token* itertok, const ValueFlow::Value* val = nullptr);
+
     void globalLockGuardError(const Token *tok);
     void localMutexError(const Token *tok);
 
@@ -271,6 +276,7 @@ class CPPCHECKLIB CheckStl : public Check {
         c.uselessCallsEmptyError(nullptr);
         c.uselessCallsRemoveError(nullptr, "remove");
         c.dereferenceInvalidIteratorError(nullptr, "i");
+        c.eraseIteratorOutOfBoundsError(nullptr, nullptr);
         c.useStlAlgorithmError(nullptr, emptyString);
         c.knownEmptyContainerError(nullptr, emptyString);
         c.globalLockGuardError(nullptr);
@@ -296,6 +302,7 @@ class CPPCHECKLIB CheckStl : public Check {
                "- common mistakes when using string::c_str()\n"
                "- useless calls of string and STL functions\n"
                "- dereferencing an invalid iterator\n"
+               "- erasing an iterator that is out of bounds\n"
                "- reading from empty STL container\n"
                "- iterating over an empty STL container\n"
                "- consider using an STL algorithm instead of raw loop\n"

releasenotes.txt

@@ -1,7 +1,7 @@
 Release Notes for Cppcheck 2.14
 
 New checks:
--
+eraseIteratorOutOfBounds: warns when erase() is called on an iterator that is out of bounds
 
 Improved checking:
 -

test/teststl.cpp

@@ -74,6 +74,7 @@ class TestStl : public TestFixture {
         TEST_CASE(iteratorExpression);
         TEST_CASE(iteratorSameExpression);
         TEST_CASE(mismatchingContainerIterator);
+        TEST_CASE(eraseIteratorOutOfBounds);
 
         TEST_CASE(dereference);
         TEST_CASE(dereference_break);  // #3644 - handle "break"
@@ -2160,6 +2161,69 @@ class TestStl : public TestFixture {
         ASSERT_EQUALS("", errout.str());
     }
 
+    void eraseIteratorOutOfBounds() {
+        check("void f() {\n"
+              "    std::vector<int> v;\n"
+              "    v.erase(v.begin());\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()' which is out of bounds.\n", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v;\n"
+              "    v.erase(v.end());\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.end()' which is out of bounds.\n", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v;\n"
+              "    auto it = v.begin();\n"
+              "    v.erase(it);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Calling function 'erase()' on the iterator 'it' which is out of bounds.\n", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v{ 1, 2, 3 };\n"
+              "    auto it = v.end();\n"
+              "    v.erase(it);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Calling function 'erase()' on the iterator 'it' which is out of bounds.\n", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v{ 1, 2, 3 };\n"
+              "    auto it = v.begin();\n"
+              "    v.erase(it);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v{ 1, 2, 3 };\n"
+              "    v.erase(v.end() - 1);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v{ 1, 2, 3 };\n"
+              "    v.erase(v.begin() - 1);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()-1' which is out of bounds.\n"
+                      "[test.cpp:3]: (error) Dereference of an invalid iterator: v.begin()-1\n",
+                      errout.str());
+
+        check("void f(std::vector<int>& v, std::vector<int>::iterator it) {\n"
+              "    if (it == v.end()) {}\n"
+              "    v.erase(it);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (warning) Either the condition 'it==v.end()' is redundant or function 'erase()' is called on the iterator 'it' which is out of bounds.\n",
+                      errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v;\n"
+              "    ((v).erase)(v.begin());\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()' which is out of bounds.\n",
+                      errout.str());
+    }
+
     // Dereferencing invalid pointer
     void dereference() {
         check("void f()\n"
@@ -2219,7 +2283,9 @@ class TestStl : public TestFixture {
               "    ints.erase(iter);\n"
               "    std::cout << iter->first << std::endl;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n"
+                      "[test.cpp:6]: (error) Calling function 'erase()' on the iterator 'iter' which is out of bounds.\n",
+                      errout.str());
 
         // Reverse iterator
         check("void f()\n"
@@ -2230,7 +2296,9 @@ class TestStl : public TestFixture {
               "    ints.erase(iter);\n"
               "    std::cout << iter->first << std::endl;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n"
+                      "[test.cpp:6]: (error) Calling function 'erase()' on the iterator 'iter' which is out of bounds.\n",
+                      errout.str());
     }
 
     void dereference_auto() {