Fix issue 4845: alias to vector element invalid after vector is changed (#2113)

* Try harder to track ref lifetimes

* Dont add lifetimes for references

* Use correct token

* Check for front and back as well

* Improve handling of addresses

* Formatting

* Fix FP

lib/checkstl.cpp

@@ -789,6 +789,20 @@ static bool isInvalidMethod(const Token * tok)
     return false;
 }
 
+static bool isVariableDecl(const Token* tok)
+{
+    if (!tok)
+        return false;
+    const Variable* var = tok->variable();
+    if (!var)
+        return false;
+    if (var->nameToken() == tok)
+        return true;
+    if (Token::Match(var->declEndToken(), "; %var%") && var->declEndToken()->next() == tok)
+        return true;
+    return false;
+}
+
 void CheckStl::invalidContainer()
 {
     const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
@@ -817,6 +831,23 @@ void CheckStl::invalidContainer()
                     return false;
                 if (info.tok->varId() == skipVarId)
                     return false;
+                if (info.tok->variable()->isReference() && 
+                    !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 && !addressOf) {
+                        // An argument always reaches
+                        if (var->isArgument() || (!var->isReference() && !var->isRValueReference() &&
+                                                  !isVariableDecl(tok) && reaches(var->nameToken(), tok, library, &ep))) {
+                            errorPath = ep;
+                            return true;
+                        }
+                    }
+                }
                 for (const ValueFlow::Value& val:info.tok->values()) {
                     if (!val.isLocalLifetimeValue())
                         continue;
@@ -836,10 +867,14 @@ void CheckStl::invalidContainer()
                 }
                 return false;
             });
-            if (!info.tok || !v)
+            if (!info.tok)
                 continue;
             errorPath.insert(errorPath.end(), info.errorPath.begin(), info.errorPath.end());
+            if (v) {
                 invalidContainerError(info.tok, tok, v, errorPath);
+            } else {
+                invalidContainerReferenceError(info.tok, tok, errorPath);
+            }
         }
     }
 }
@@ -855,6 +890,17 @@ void CheckStl::invalidContainerError(const Token *tok, const Token * contTok, co
     reportError(errorPath, Severity::error, "invalidContainer", msg + " that may be invalid.", CWE664, false);
 }
 
+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);
+}
+
 void CheckStl::stlOutOfBounds()
 {
     const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase();

lib/checkstl.h

@@ -206,6 +206,7 @@ private:
     void sizeError(const Token* tok);
     void redundantIfRemoveError(const Token* tok);
     void invalidContainerError(const Token *tok, const Token * contTok, const ValueFlow::Value *val, ErrorPath errorPath);
+    void invalidContainerReferenceError(const Token* tok, const Token* contTok, ErrorPath errorPath);
 
     void uselessCallsReturnValueError(const Token* tok, const std::string& varname, const std::string& function);
     void uselessCallsSwapError(const Token* tok, const std::string& varname);

lib/valueflow.cpp

@@ -2927,7 +2927,10 @@ ValueFlow::Value getLifetimeObjValue(const Token *tok)
     return result;
 }
 
-static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth = 20)
+static const Token* getLifetimeToken(const Token* tok,
+                                     ValueFlow::Value::ErrorPath& errorPath,
+                                     bool* addressOf = nullptr,
+                                     int depth = 20)
 {
     if (!tok)
         return nullptr;
@@ -2936,6 +2939,8 @@ static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPa
         return tok;
     if (var && var->declarationId() == tok->varId()) {
         if (var->isReference() || var->isRValueReference()) {
+            if (addressOf)
+                *addressOf = true;
             if (!var->declEndToken())
                 return tok;
             if (var->isArgument()) {
@@ -2947,15 +2952,14 @@ static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPa
                 if (vartok == tok)
                     return tok;
                 if (vartok)
-                    return getLifetimeToken(vartok, errorPath, depth - 1);
+                    return getLifetimeToken(vartok, errorPath, addressOf, depth - 1);
             } else {
                 return nullptr;
             }
         }
     } else if (Token::Match(tok->previous(), "%name% (")) {
         const Function *f = tok->previous()->function();
-        if (!f)
+        if (f) {
-            return tok;
             if (!Function::returnsReference(f))
                 return tok;
             const Token* returnTok = findSimpleReturn(f);
@@ -2963,7 +2967,7 @@ static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPa
                 return tok;
             if (returnTok == tok)
                 return tok;
-        const Token *argvarTok = getLifetimeToken(returnTok, errorPath, depth - 1);
+            const Token* argvarTok = getLifetimeToken(returnTok, errorPath, addressOf, depth - 1);
             if (!argvarTok)
                 return tok;
             const Variable* argvar = argvarTok->variable();
@@ -2976,7 +2980,17 @@ static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPa
                 const Token* argTok = getArguments(tok->previous()).at(n);
                 errorPath.emplace_back(returnTok, "Return reference.");
                 errorPath.emplace_back(tok->previous(), "Called function passing '" + argTok->str() + "'.");
-            return getLifetimeToken(argTok, errorPath, depth - 1);
+                return getLifetimeToken(argTok, errorPath, addressOf, depth - 1);
+            }
+        } else if (Token::Match(tok->tokAt(-2), ". %name% (") && astIsContainer(tok->tokAt(-2)->astOperand1())) {
+            const Library::Container* library = getLibraryContainer(tok->tokAt(-2)->astOperand1());
+            Library::Container::Yield y = library->getYield(tok->previous()->str());
+            if (y == Library::Container::Yield::AT_INDEX || y == Library::Container::Yield::ITEM) {
+                errorPath.emplace_back(tok->previous(), "Accessing container.");
+                if (addressOf)
+                    *addressOf = false;
+                return getLifetimeToken(tok->tokAt(-2)->astOperand1(), errorPath, nullptr, depth - 1);
+            }
         }
     } else if (Token::Match(tok, ".|::|[")) {
         const Token *vartok = tok;
@@ -2998,18 +3012,22 @@ static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPa
                 if (!v.isLocalLifetimeValue())
                     continue;
                 errorPath.insert(errorPath.end(), v.errorPath.begin(), v.errorPath.end());
-                return getLifetimeToken(v.tokvalue, errorPath);
+                return getLifetimeToken(v.tokvalue, errorPath, addressOf);
             }
         } else {
-            return getLifetimeToken(vartok, errorPath);
+            if (addressOf && astIsContainer(vartok) && Token::simpleMatch(vartok->astParent(), "[")) {
+                *addressOf = false;
+                addressOf = nullptr;
+            }
+            return getLifetimeToken(vartok, errorPath, addressOf);
         }
     }
     return tok;
 }
 
-const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath)
+const Variable* getLifetimeVariable(const Token* tok, ValueFlow::Value::ErrorPath& errorPath, bool* addressOf)
 {
-    const Token *tok2 = getLifetimeToken(tok, errorPath);
+    const Token* tok2 = getLifetimeToken(tok, errorPath, addressOf);
     if (tok2 && tok2->variable())
         return tok2->variable();
     return nullptr;

lib/valueflow.h

@@ -249,7 +249,7 @@ namespace ValueFlow {
     std::string eitherTheConditionIsRedundant(const Token *condition);
 }
 
-const Variable *getLifetimeVariable(const Token *tok, ValueFlow::Value::ErrorPath &errorPath);
+const Variable* getLifetimeVariable(const Token* tok, ValueFlow::Value::ErrorPath& errorPath, bool* addressOf = nullptr);
 
 bool isLifetimeBorrowed(const Token *tok, const Settings *settings);
 

test/teststl.cpp

@@ -3912,6 +3912,47 @@ private:
               true);
         ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:1] -> [test.cpp:4]: (error) Using pointer to local variable 'v' that may be invalid.\n", errout.str());
 
+        check("void f() {\n"
+              "    std::vector<int> v = {1};\n"
+              "    int &v0 = v.front();\n"
+              "    v.push_back(123);\n"
+              "    std::cout << v0 << std::endl;\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:4] -> [test.cpp:5]: (error) Reference to v that may be invalid.\n",
+            errout.str());
+
+        check("void f() {\n"
+              "    std::vector<int> v = {1};\n"
+              "    int &v0 = v[0];\n"
+              "    v.push_back(123);\n"
+              "    std::cout << v0 << std::endl;\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4] -> [test.cpp:5]: (error) Reference to v that may be invalid.\n",
+                      errout.str());
+
+        check("void f(std::vector<int> &v) {\n"
+              "    int &v0 = v.front();\n"
+              "    v.push_back(123);\n"
+              "    std::cout << v0 << std::endl;\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS(
+            "[test.cpp:2] -> [test.cpp:2] -> [test.cpp:1] -> [test.cpp:3] -> [test.cpp:4]: (error) Reference to v that may be invalid.\n",
+            errout.str());
+
+        check("void f(std::vector<int> &v) {\n"
+              "    int &v0 = v[0];\n"
+              "    v.push_back(123);\n"
+              "    std::cout << v0 << std::endl;\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS(
+            "[test.cpp:2] -> [test.cpp:1] -> [test.cpp:3] -> [test.cpp:4]: (error) Reference to v that may be invalid.\n",
+            errout.str());
+
         check("void f(std::vector<int> &v) {\n"
               "    std::vector<int> *v0 = &v;\n"
               "    v.push_back(123);\n"
@@ -3919,6 +3960,22 @@ private:
               "}\n",
               true);
         ASSERT_EQUALS("", errout.str());
+
+        check("const std::vector<int> * g(int);\n"
+              "void f() {\n"
+              "    const std::vector<int> *v = g(1);\n"
+              "    if (v && v->size() == 1U) {\n"
+              "        const int &m = v->front();\n"
+              "    }\n"
+              "\n"
+              "    v = g(2);\n"
+              "    if (v && v->size() == 1U) {\n"
+              "        const int &m = v->front();\n"
+              "        if (m == 0) {}\n"
+              "    }\n"
+              "}\n",
+              true);
+        ASSERT_EQUALS("", errout.str());
     }
 
     void findInsert() {