Fix 9135: Access of moved variable not detected in loop (#4215)

* Fix 9135: Access of moved variable not detected in loop

* Format

* Fix issue with pushing back on container

* Format

* Fix null pointer

* Remove yeild for now

lib/astutils.cpp

@@ -260,6 +260,25 @@ bool astIsContainerOwned(const Token* tok) {
     return astIsContainer(tok) && !astIsContainerView(tok);
 }
 
+static const Token* getContainerFunction(const Token* tok)
+{
+    if (!tok || !tok->valueType() || !tok->valueType()->container)
+        return nullptr;
+    const Token* parent = tok->astParent();
+    if (Token::Match(parent, ". %name% (") && astIsLHS(tok)) {
+        return parent->next();
+    }
+    return nullptr;
+}
+
+Library::Container::Action astContainerAction(const Token* tok)
+{
+    const Token* ftok = getContainerFunction(tok);
+    if (!ftok)
+        return Library::Container::Action::NO_ACTION;
+    return tok->valueType()->container->getAction(ftok->str());
+}
+
 bool astIsRangeBasedForDecl(const Token* tok)
 {
     return Token::simpleMatch(tok->astParent(), ":") && Token::simpleMatch(tok->astParent()->astParent(), "(");

lib/astutils.h

@@ -139,6 +139,8 @@ bool astIsContainer(const Token *tok);
 bool astIsContainerView(const Token* tok);
 bool astIsContainerOwned(const Token* tok);
 
+Library::Container::Action astContainerAction(const Token* tok);
+
 /** Is given token a range-declaration in a range-based for loop */
 bool astIsRangeBasedForDecl(const Token* tok);
 

lib/forwardanalyzer.cpp

@@ -607,7 +607,8 @@ struct ForwardTraversal {
                     } else if (condTok->values().front().intvalue == inElse) {
                         return Break();
                     }
-                    // Handle for loop
+                    // Handle loop
+                    if (inLoop) {
                         Token* stepTok = getStepTokFromEnd(tok);
                         bool checkThen, checkElse;
                         std::tie(checkThen, checkElse) = evalCond(condTok);
@@ -616,6 +617,13 @@ struct ForwardTraversal {
                                 return Break();
                             if (updateRecursive(condTok) == Progress::Break)
                                 return Break();
+                            // Reevaluate condition
+                            std::tie(checkThen, checkElse) = evalCond(condTok);
+                        }
+                        if (!checkElse) {
+                            if (updateLoopExit(end, tok, condTok, nullptr, stepTok) == Progress::Break)
+                                return Break();
+                        }
                     }
                     analyzer->assume(condTok, !inElse, Analyzer::Assume::Quiet);
                     if (Token::simpleMatch(tok, "} else {"))
@@ -852,7 +860,6 @@ struct ForwardTraversal {
             return nullptr;
         return getStepTok(end->link());
     }
-
 };
 
 Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings)

lib/valueflow.cpp

@@ -2108,6 +2108,24 @@ static bool evalAssignment(Value& lhsValue, const std::string& assign, const Val
     return !error;
 }
 
+static ValueFlow::Value::MoveKind isMoveOrForward(const Token* tok)
+{
+    if (!tok)
+        return ValueFlow::Value::MoveKind::NonMovedVariable;
+    const Token* parent = tok->astParent();
+    if (!Token::simpleMatch(parent, "("))
+        return ValueFlow::Value::MoveKind::NonMovedVariable;
+    const Token* ftok = parent->astOperand1();
+    if (!ftok)
+        return ValueFlow::Value::MoveKind::NonMovedVariable;
+    if (Token::simpleMatch(ftok->astOperand1(), "std :: move"))
+        return ValueFlow::Value::MoveKind::MovedVariable;
+    if (Token::simpleMatch(ftok->astOperand1(), "std :: forward"))
+        return ValueFlow::Value::MoveKind::ForwardedVariable;
+    // TODO: Check for cast
+    return ValueFlow::Value::MoveKind::NonMovedVariable;
+}
+
 template<class T>
 struct SingleRange {
     T* x;
@@ -2416,6 +2434,19 @@ struct ValueFlowAnalyzer : Analyzer {
 
     virtual Action isModified(const Token* tok) const {
         Action read = Action::Read;
+        const ValueFlow::Value* value = getValue(tok);
+        if (value) {
+            // Moving a moved value wont change the moved value
+            if (value->isMovedValue() && isMoveOrForward(tok) != ValueFlow::Value::MoveKind::NonMovedVariable)
+                return read;
+            // Inserting elements to container wont change the lifetime
+            if (astIsContainer(tok) && value->isLifetimeValue() &&
+                contains({Library::Container::Action::PUSH,
+                          Library::Container::Action::INSERT,
+                          Library::Container::Action::CHANGE_INTERNAL},
+                         astContainerAction(tok)))
+                return read;
+        }
         bool inconclusive = false;
         if (isVariableChangedByFunctionCall(tok, getIndirect(tok), getSettings(), &inconclusive))
             return read | Action::Invalid;
@@ -2424,7 +2455,6 @@ struct ValueFlowAnalyzer : Analyzer {
         if (isVariableChanged(tok, getIndirect(tok), getSettings(), isCPP())) {
             if (Token::Match(tok->astParent(), "*|[|.|++|--"))
                 return read | Action::Invalid;
-            const ValueFlow::Value* value = getValue(tok);
             // Check if its assigned to the same value
             if (value && !value->isImpossible() && Token::simpleMatch(tok->astParent(), "=") && astIsLHS(tok) &&
                 astIsIntegral(tok->astParent()->astOperand2(), false)) {

test/testautovariables.cpp

@@ -3943,7 +3943,9 @@ private:
               "    if (former_hover != pitem)\n"
               "        dosth();\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'item' that is out of scope.\n", errout.str());
+        ASSERT_EQUALS(
+            "[test.cpp:5] -> [test.cpp:3] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'item' that is out of scope.\n",
+            errout.str());
 
         // #6575
         check("void trp_deliver_signal()  {\n"

test/testbufferoverrun.cpp

@@ -867,7 +867,7 @@ private:
               "        i=4;\n"
               "    }\n"
               "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:6]: (error) Array 'a[5]' accessed at index 5, which is out of bounds.\n", errout.str());
 
         check("void f()\n"
               "{\n"

test/testother.cpp

@@ -254,6 +254,7 @@ private:
         TEST_CASE(moveAndAddressOf);
         TEST_CASE(partiallyMoved);
         TEST_CASE(moveAndLambda);
+        TEST_CASE(moveInLoop);
         TEST_CASE(forwardAndUsed);
 
         TEST_CASE(funcArgNamesDifferent);
@@ -9837,6 +9838,25 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void moveInLoop()
+    {
+        check("void g(std::string&& s);\n"
+              "void f() {\n"
+              "    std::string p;\n"
+              "    while(true)\n"
+              "        g(std::move(p));\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable 'p'.\n", errout.str());
+
+        check("std::list<int> g(std::list<int>&&);\n"
+              "void f(std::list<int>l) {\n"
+              "    for(int i = 0; i < 10; ++i) {\n"
+              "        for (auto &j : g(std::move(l))) { (void)j; }\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable 'l'.\n", errout.str());
+    }
+
     void forwardAndUsed() {
         Settings keepTemplates;
         keepTemplates.checkUnusedTemplates = true;