Fix issue 9548: False negative: Mismatching iterators when inserting into a vector (#2595)

lib/checkstl.cpp

@@ -27,6 +27,7 @@
 #include "utils.h"
 #include "astutils.h"
 #include "pathanalysis.h"
+#include "valueflow.h"
 
 #include <cstddef>
 #include <list>
@@ -534,6 +535,18 @@ void CheckStl::iterators()
     }
 }
 
+void CheckStl::mismatchingContainerIteratorError(const Token* tok, const Token* iterTok)
+{
+    const std::string container(tok ? tok->expressionString() : std::string("v1"));
+    const std::string iter(iterTok ? iterTok->expressionString() : std::string("it"));
+    reportError(tok,
+                Severity::error,
+                "mismatchingContainerIterator",
+                "Iterator '" + iter + "' from different container '" + container + "' are used together.",
+                CWE664,
+                false);
+}
+
 // Error message for bad iterator usage..
 void CheckStl::mismatchingContainersError(const Token* tok1, const Token* tok2)
 {
@@ -708,6 +721,49 @@ void CheckStl::mismatchingContainers()
     }
 }
 
+void CheckStl::mismatchingContainerIterator()
+{
+    // Check if different containers are used in various calls of standard functions
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+    for (const Scope * scope : symbolDatabase->functionScopes) {
+        for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
+            if (!astIsContainer(tok))
+                continue;
+            if (!Token::Match(tok, "%var% . %name% ( !!)"))
+                continue;
+            const Token * const ftok = tok->tokAt(2);
+            const std::vector<const Token *> args = getArguments(ftok);
+
+            const Library::Container * c = tok->valueType()->container;
+            Library::Container::Action action = c->getAction(tok->strAt(2));
+            const Token* iterTok = nullptr;
+            if (action == Library::Container::Action::INSERT && args.size() == 2) {
+                // Skip if iterator pair
+                if (astIsIterator(args.back()))
+                    continue;
+                if (!astIsIterator(args.front()))
+                    continue;
+                iterTok = args.front();
+            } else if (action == Library::Container::Action::ERASE) {
+                if (!astIsIterator(args.front()))
+                    continue;
+                iterTok = args.front();
+            } else {
+                continue;
+            }
+
+            ValueFlow::Value val = getLifetimeObjValue(iterTok);
+            if (!val.tokvalue)
+                continue;
+            if (val.lifetimeKind != ValueFlow::Value::LifetimeKind::Iterator)
+                continue;
+            if (isSameExpression(true, false, tok, val.tokvalue, mSettings->library, false, false))
+                continue;
+            mismatchingContainerIteratorError(tok, iterTok);
+        }
+    }
+}
+
 static bool isInvalidMethod(const Token * tok)
 {
     if (Token::Match(tok->next(), ". assign|clear|swap"))

lib/checkstl.h

@@ -80,6 +80,7 @@ public:
         checkStl.invalidContainer();
         checkStl.invalidContainerLoop();
         checkStl.mismatchingContainers();
+        checkStl.mismatchingContainerIterator();
 
         checkStl.stlBoundaries();
         checkStl.checkDereferenceInvalidIterator();
@@ -123,6 +124,8 @@ public:
      */
     void mismatchingContainers();
     
+    void mismatchingContainerIterator();
+
     /**
      * Dangerous usage of erase. The iterator is invalidated by erase so
      * it is bad to dereference it after the erase.
@@ -201,6 +204,7 @@ private:
     void iteratorsError(const Token* tok, const std::string& containerName1, const std::string& containerName2);
     void iteratorsError(const Token* tok, const Token* containerTok, const std::string& containerName1, const std::string& containerName2);
     void iteratorsError(const Token* tok, const Token* containerTok, const std::string& containerName);
+    void mismatchingContainerIteratorError(const Token* tok, const Token* iterTok);
     void mismatchingContainersError(const Token* tok1, const Token* tok2);
     void mismatchingContainerExpressionError(const Token *tok1, const Token *tok2);
     void sameIteratorExpressionError(const Token *tok);
@@ -235,6 +239,7 @@ private:
         c.iteratorsError(nullptr, nullptr, "container");
         c.invalidContainerLoopError(nullptr, nullptr);
         c.invalidContainerError(nullptr, nullptr, nullptr, errorPath);
+        c.mismatchingContainerIteratorError(nullptr, nullptr);
         c.mismatchingContainersError(nullptr, nullptr);
         c.mismatchingContainerExpressionError(nullptr, nullptr);
         c.sameIteratorExpressionError(nullptr);

test/teststl.cpp

@@ -70,6 +70,7 @@ private:
         TEST_CASE(iterator25); // #9742
         TEST_CASE(iteratorExpression);
         TEST_CASE(iteratorSameExpression);
+        TEST_CASE(mismatchingContainerIterator);
 
         TEST_CASE(dereference);
         TEST_CASE(dereference_break);  // #3644 - handle "break"
@@ -1277,6 +1278,21 @@ private:
         ASSERT_EQUALS("[test.cpp:2]: (style) Same iterators expression are used for algorithm.\n", errout.str());
     }
 
+    void mismatchingContainerIterator() {
+        check("std::vector<int> to_vector(int value) {\n"
+              "    std::vector<int> a, b;\n"
+              "    a.insert(b.end(), value);\n"
+              "    return a;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Iterator 'b.end()' from different container 'a' are used together.\n", errout.str());
+
+        check("std::vector<int> f(std::vector<int> a, std::vector<int> b) {\n"
+              "    a.erase(b.begin());\n"
+              "    return a;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2]: (error) Iterator 'b.begin()' from different container 'a' are used together.\n", errout.str());
+    }
+
     // Dereferencing invalid pointer
     void dereference() {
         check("void f()\n"