Fix issue 9133: Invalid iterator; vector::push_back, functions (#3008)

lib/astutils.cpp

@@ -1877,6 +1877,16 @@ std::vector<const Token *> getArguments(const Token *ftok)
     return astFlatten(startTok, ",");
 }
 
+int getArgumentPos(const Variable* var, const Function* f)
+{
+    auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable& v) {
+        return v.nameToken() == var->nameToken();
+    });
+    if (arg_it == f->argumentList.end())
+        return -1;
+    return std::distance(f->argumentList.begin(), arg_it);
+}
+
 const Token *findLambdaStartToken(const Token *last)
 {
     if (!last || last->str() != "}")

lib/astutils.h

@@ -30,6 +30,7 @@
 #include "errortypes.h"
 #include "utils.h"
 
+class Function;
 class Library;
 class Scope;
 class Settings;
@@ -234,6 +235,8 @@ int numberOfArguments(const Token *start);
  */
 std::vector<const Token *> getArguments(const Token *ftok);
 
+int getArgumentPos(const Variable* var, const Function* f);
+
 const Token *findLambdaStartToken(const Token *last);
 
 /**

lib/checkstl.cpp

@@ -18,17 +18,18 @@
 
 #include "checkstl.h"
 
+#include "astutils.h"
 #include "check.h"
 #include "checknullpointer.h"
+#include "errortypes.h"
 #include "library.h"
 #include "mathlib.h"
+#include "pathanalysis.h"
 #include "settings.h"
 #include "standards.h"
 #include "symboldatabase.h"
 #include "token.h"
 #include "utils.h"
-#include "astutils.h"
-#include "pathanalysis.h"
 #include "valueflow.h"
 
 #include <algorithm>
@@ -38,6 +39,8 @@
 #include <map>
 #include <set>
 #include <sstream>
+#include <unordered_map>
+#include <unordered_set>
 #include <utility>
 
 // Register this check class (by creating a static instance of it)
@@ -843,6 +846,105 @@ static bool isInvalidMethod(const Token * tok)
     return false;
 }
 
+struct InvalidContainerAnalyzer {
+    struct Info {
+        struct Reference {
+            const Token* tok;
+            ErrorPath errorPath;
+        };
+        std::unordered_map<int, Reference> expressions;
+        ErrorPath errorPath;
+        void add(const std::vector<Reference>& refs)
+        {
+            for (const Reference& r : refs) {
+                add(r);
+            }
+        }
+        void add(const Reference& r)
+        {
+            if (!r.tok)
+                return;
+            expressions.insert(std::make_pair(r.tok->exprId(), r));
+        }
+
+        std::vector<Reference> invalidTokens() const
+        {
+            std::vector<Reference> result;
+            std::transform(expressions.begin(), expressions.end(), std::back_inserter(result), SelectMapValues{});
+            return result;
+        }
+    };
+    std::unordered_map<const Function*, Info> invalidMethods;
+
+    std::vector<Info::Reference> invalidatesContainer(const Token* tok) const
+    {
+        std::vector<Info::Reference> result;
+        if (Token::Match(tok, "%name% (")) {
+            const Function* f = tok->function();
+            if (!f)
+                return result;
+            ErrorPathItem epi = std::make_pair(tok, "Calling function " + tok->str());
+            const bool dependsOnThis = exprDependsOnThis(tok->next());
+            auto it = invalidMethods.find(f);
+            if (it != invalidMethods.end()) {
+                std::vector<Info::Reference> refs = it->second.invalidTokens();
+                std::copy_if(refs.begin(), refs.end(), std::back_inserter(result), [&](const Info::Reference& r) {
+                    const Variable* var = r.tok->variable();
+                    if (!var)
+                        return false;
+                    if (dependsOnThis && !var->isLocal() && !var->isGlobal() && !var->isStatic())
+                        return true;
+                    if (!var->isArgument())
+                        return false;
+                    if (!var->isReference())
+                        return false;
+                    return true;
+                });
+                std::vector<const Token*> args = getArguments(tok);
+                for (Info::Reference& r : result) {
+                    r.errorPath.push_front(epi);
+                    const Variable* var = r.tok->variable();
+                    if (!var)
+                        continue;
+                    if (var->isArgument()) {
+                        int n = getArgumentPos(var, f);
+                        const Token* tok2 = nullptr;
+                        if (n >= 0 && n < args.size())
+                            tok2 = args[n];
+                        r.tok = tok2;
+                    }
+                }
+            }
+        } else if (astIsContainer(tok)) {
+            if (isInvalidMethod(tok)) {
+                ErrorPath ep;
+                ep.emplace_front(tok,
+                                 "After calling '" + tok->strAt(2) +
+                                     "', iterators or references to the container's data may be invalid .");
+                result.push_back(Info::Reference{tok, ep});
+            }
+        }
+        return result;
+    }
+
+    void analyze(const SymbolDatabase* symboldatabase)
+    {
+        for (const Scope* scope : symboldatabase->functionScopes) {
+            const Function* f = scope->function;
+            if (!f)
+                continue;
+            for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
+                if (Token::Match(tok, "if|while|for|goto|return"))
+                    break;
+                std::vector<Info::Reference> c = invalidatesContainer(tok);
+                if (c.empty())
+                    continue;
+                invalidMethods[f].add(c);
+            }
+        }
+    }
+};
+
 static bool isVariableDecl(const Token* tok)
 {
     if (!tok)
@@ -861,15 +963,12 @@ void CheckStl::invalidContainer()
 {
     const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
     const Library& library = mSettings->library;
+    InvalidContainerAnalyzer analyzer;
+    analyzer.analyze(symbolDatabase);
     for (const Scope * scope : symbolDatabase->functionScopes) {
         for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
-            if (!Token::Match(tok, "%var%"))
+            for (const InvalidContainerAnalyzer::Info::Reference& r : analyzer.invalidatesContainer(tok)) {
-                continue;
+                if (!astIsContainer(r.tok))
-            if (tok->varId() == 0)
-                continue;
-            if (!astIsContainer(tok))
-                continue;
-            if (!isInvalidMethod(tok))
                     continue;
                 std::set<nonneg int> skipVarIds;
                 // Skip if the variable is assigned to
@@ -890,7 +989,8 @@ void CheckStl::invalidContainer()
                     endToken = tok->next();
                 const ValueFlow::Value* v = nullptr;
                 ErrorPath errorPath;
-            PathAnalysis::Info info = PathAnalysis{endToken, library} .forwardFind([&](const PathAnalysis::Info& info) {
+                PathAnalysis::Info info =
+                    PathAnalysis{endToken, library}.forwardFind([&](const PathAnalysis::Info& info) {
                         if (!info.tok->variable())
                             return false;
                         if (info.tok->varId() == 0)
@@ -901,18 +1001,18 @@ void CheckStl::invalidContainer()
                         // return false;
                         if (Token::Match(info.tok->astParent(), "%assign%") && astIsLHS(info.tok))
                             skipVarIds.insert(info.tok->varId());
-                if (info.tok->variable()->isReference() &&
+                        if (info.tok->variable()->isReference() && !isVariableDecl(info.tok) &&
-                    !isVariableDecl(info.tok) &&
                             reaches(info.tok->variable()->nameToken(), tok, library, nullptr)) {
 
                             ErrorPath ep;
                             bool addressOf = false;
                             const Variable* var = getLifetimeVariable(info.tok, ep, &addressOf);
                             // Check the reference is created before the change
-                    if (var && var->declarationId() == tok->varId() && !addressOf) {
+                            if (var && var->declarationId() == r.tok->varId() && !addressOf) {
                                 // An argument always reaches
-                        if (var->isArgument() || (!var->isReference() && !var->isRValueReference() &&
+                                if (var->isArgument() ||
-                                                  !isVariableDecl(tok) && reaches(var->nameToken(), tok, library, &ep))) {
+                                    (!var->isReference() && !var->isRValueReference() && !isVariableDecl(tok) &&
+                                     reaches(var->nameToken(), tok, library, &ep))) {
                                     errorPath = ep;
                                     return true;
                                 }
@@ -927,7 +1027,7 @@ void CheckStl::invalidContainer()
                                 continue;
                             if (!val.tokvalue->variable())
                                 continue;
-                    if (val.tokvalue->varId() != tok->varId())
+                            if (val.tokvalue->varId() != r.tok->varId())
                                 continue;
                             ErrorPath ep;
                             // Check the iterator is created before the change
@@ -942,10 +1042,12 @@ void CheckStl::invalidContainer()
                 if (!info.tok)
                     continue;
                 errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end());
+                errorPath.insert(errorPath.end(), r.errorPath.begin(), r.errorPath.end());
                 if (v) {
-                invalidContainerError(info.tok, tok, v, errorPath);
+                    invalidContainerError(info.tok, r.tok, v, errorPath);
                 } else {
-                invalidContainerReferenceError(info.tok, tok, errorPath);
+                    invalidContainerReferenceError(info.tok, r.tok, errorPath);
+                }
             }
         }
     }
@@ -1011,8 +1113,6 @@ void CheckStl::invalidContainerLoopError(const Token *tok, const Token * loopTok
 void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, const ValueFlow::Value *val, ErrorPath errorPath)
 {
     const bool inconclusive = val ? val->isInconclusive() : false;
-    std::string method = contTok ? contTok->strAt(2) : "erase";
-    errorPath.emplace_back(contTok, "After calling '" + method + "', iterators or references to the container's data may be invalid .");
     if (val)
         errorPath.insert(errorPath.begin(), val->errorPath.begin(), val->errorPath.end());
     std::string msg = "Using " + lifetimeMessage(tok, val, errorPath);
@@ -1022,10 +1122,7 @@ void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, co
 
 void CheckStl::invalidContainerReferenceError(const Token* tok, const Token* contTok, ErrorPath errorPath)
 {
-    std::string method = contTok ? contTok->strAt(2) : "erase";
     std::string name = contTok ? contTok->expressionString() : "x";
-    errorPath.emplace_back(
-        contTok, "After calling '" + method + "', iterators or references to the container's data may be invalid .");
     std::string msg = "Reference to " + name;
     errorPath.emplace_back(tok, "");
     reportError(errorPath, Severity::error, "invalidContainerReference", msg + " that may be invalid.", CWE664, false);

lib/utils.h

@@ -28,6 +28,22 @@
 #include <string>
 #include <vector>
 
+struct SelectMapKeys {
+    template <class Pair>
+    typename Pair::first_type operator()(const Pair& p) const
+    {
+        return p.first;
+    }
+};
+
+struct SelectMapValues {
+    template <class Pair>
+    typename Pair::second_type operator()(const Pair& p) const
+    {
+        return p.second;
+    }
+};
+
 inline bool endsWith(const std::string &str, char c)
 {
     return str[str.size()-1U] == c;

lib/valueflow.cpp

@@ -1753,20 +1753,6 @@ static bool bifurcate(const Token* tok, const std::set<nonneg int>& varids, cons
     return false;
 }
 
-struct SelectMapKeys {
-    template<class Pair>
-    typename Pair::first_type operator()(const Pair& p) const {
-        return p.first;
-    }
-};
-
-struct SelectMapValues {
-    template<class Pair>
-    typename Pair::second_type operator()(const Pair& p) const {
-        return p.second;
-    }
-};
-
 struct ValueFlowAnalyzer : Analyzer {
     const TokenList* tokenlist;
     ProgramMemoryState pms;
@@ -2470,16 +2456,6 @@ static void valueFlowReverse(Token* tok,
     }
 }
 
-static int getArgumentPos(const Variable *var, const Function *f)
-{
-    auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) {
-        return v.nameToken() == var->nameToken();
-    });
-    if (arg_it == f->argumentList.end())
-        return -1;
-    return std::distance(f->argumentList.begin(), arg_it);
-}
-
 std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
 {
     std::string result;

test/teststl.cpp

@@ -4446,6 +4446,38 @@ private:
               "    return info.ret;\n"
               "}\n",true);
         ASSERT_EQUALS("", errout.str());
+
+        // #9133
+        check("struct Fred {\n"
+              "    std::vector<int> v;\n"
+              "    void foo();\n"
+              "    void bar();\n"
+              "};\n"
+              "void Fred::foo() {\n"
+              "    std::vector<int>::iterator it = v.begin();\n"
+              "    bar();\n"
+              "    it++;\n"
+              "}\n"
+              "void Fred::bar() {\n"
+              "    v.push_back(0);\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS(
+            "[test.cpp:7] -> [test.cpp:8] -> [test.cpp:12] -> [test.cpp:2] -> [test.cpp:9]: (error) Using iterator to local container 'v' that may be invalid.\n",
+            errout.str());
+
+        check("void foo(std::vector<int>& v) {\n"
+              "    std::vector<int>::iterator it = v.begin();\n"
+              "    bar(v);\n"
+              "    it++;\n"
+              "}\n"
+              "void bar(std::vector<int>& v) {\n"
+              "    v.push_back(0);\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS(
+            "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:7] -> [test.cpp:1] -> [test.cpp:4]: (error) Using iterator to local container 'v' that may be invalid.\n",
+            errout.str());
     }
 
     void invalidContainerLoop() {