Fix 9755: false negative: access of moved variable in conditional code (#4459)

* Fix 9755: false negative: access of moved variable in conditional code

* Format

lib/astutils.cpp

@@ -2897,6 +2897,60 @@ bool isConstVarExpression(const Token *tok, std::function<bool(const Token*)> sk
     return false;
 }
 
+static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings)
+{
+    const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&");
+
+    int argnr;
+    const Token* ftok = getTokenArgumentFunction(tok, argnr);
+    if (!ftok)
+        return ExprUsage::None;
+    if (ftok->function()) {
+        std::vector<const Variable*> args = getArgumentVars(ftok, argnr);
+        for (const Variable* arg : args) {
+            if (!arg)
+                continue;
+            if (arg->isReference())
+                return ExprUsage::PassedByReference;
+        }
+        if (!args.empty() && indirect == 0 && !addressOf)
+            return ExprUsage::Used;
+    } else {
+        const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1);
+        if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad)
+            return ExprUsage::Used;
+        bool hasIndirect = false;
+        const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
+        if (isuninitbad && (!addressOf || isnullbad))
+            return ExprUsage::Used;
+    }
+    return ExprUsage::Inconclusive;
+}
+
+ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings)
+{
+    if (indirect > 0 && tok->astParent()) {
+        if (Token::Match(tok->astParent(), "%assign%") && astIsRHS(tok))
+            return ExprUsage::NotUsed;
+        if (tok->astParent()->isConstOp())
+            return ExprUsage::NotUsed;
+        if (tok->astParent()->isCast())
+            return ExprUsage::NotUsed;
+    }
+    if (indirect == 0) {
+        if (Token::Match(tok->astParent(), "%cop%|%assign%|++|--") && !Token::simpleMatch(tok->astParent(), "=") &&
+            !tok->astParent()->isUnaryOp("&"))
+            return ExprUsage::Used;
+        if (Token::simpleMatch(tok->astParent(), "=") && astIsRHS(tok))
+            return ExprUsage::Used;
+        // Function call or index
+        if (Token::Match(tok->astParent(), "(|[") && !tok->astParent()->isCast() &&
+            (astIsLHS(tok) || Token::simpleMatch(tok->astParent(), "( )")))
+            return ExprUsage::Used;
+    }
+    return getFunctionUsage(tok, indirect, settings);
+}
+
 static void getLHSVariablesRecursive(std::vector<const Variable*>& vars, const Token* tok)
 {
     if (!tok)

lib/astutils.h

@@ -392,6 +392,10 @@ bool isCPPCast(const Token* tok);
 
 bool isConstVarExpression(const Token* tok, std::function<bool(const Token*)> skipPredicate = nullptr);
 
+enum class ExprUsage { None, NotUsed, PassedByReference, Used, Inconclusive };
+
+ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings);
+
 const Variable *getLHSVariable(const Token *tok);
 
 const Token* getLHSVariableToken(const Token* tok);

lib/checkother.cpp

@@ -3228,6 +3228,8 @@ void CheckOther::checkAccessOfMovedVariable()
                 scopeStart = memberInitializationStart;
         }
         for (const Token* tok = scopeStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
+            if (!tok->astParent())
+                continue;
             const ValueFlow::Value * movedValue = tok->getMovedValue();
             if (!movedValue || movedValue->moveKind == ValueFlow::Value::MoveKind::NonMovedVariable)
                 continue;
@@ -3242,13 +3244,13 @@ void CheckOther::checkAccessOfMovedVariable()
                 else
                     inconclusive = true;
             } else {
-                const bool variableChanged = isVariableChangedByFunctionCall(tok, 0, mSettings, &inconclusive);
+                const ExprUsage usage = getExprUsage(tok, 0, mSettings);
-                accessOfMoved = !variableChanged && checkUninitVar.isVariableUsage(tok, false, CheckUninitVar::NO_ALLOC);
+                if (usage == ExprUsage::Used)
-                if (inconclusive) {
+                    accessOfMoved = true;
-                    accessOfMoved = !isMovedParameterAllowedForInconclusiveFunction(tok);
+                if (usage == ExprUsage::PassedByReference)
-                    if (accessOfMoved)
+                    accessOfMoved = !isVariableChangedByFunctionCall(tok, 0, mSettings, &inconclusive);
-                        inconclusive = false;
+                else if (usage == ExprUsage::Inconclusive)
-                }
+                    inconclusive = true;
             }
             if (accessOfMoved || (inconclusive && reportInconclusive))
                 accessMovedError(tok, tok->str(), movedValue, inconclusive || movedValue->isInconclusive());
@@ -3256,19 +3258,6 @@ void CheckOther::checkAccessOfMovedVariable()
     }
 }
 
-bool CheckOther::isMovedParameterAllowedForInconclusiveFunction(const Token * tok)
-{
-    if (Token::simpleMatch(tok->tokAt(-4), "std :: move ("))
-        return false;
-    const Token * tokAtM2 = tok->tokAt(-2);
-    if (Token::simpleMatch(tokAtM2, "> (") && tokAtM2->link()) {
-        const Token * leftAngle = tokAtM2->link();
-        if (Token::simpleMatch(leftAngle->tokAt(-3), "std :: forward <"))
-            return false;
-    }
-    return true;
-}
-
 void CheckOther::accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive)
 {
     if (!tok) {

lib/checkother.h

@@ -275,7 +275,6 @@ private:
     void raceAfterInterlockedDecrementError(const Token* tok);
     void unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef);
     void unknownEvaluationOrder(const Token* tok);
-    static bool isMovedParameterAllowedForInconclusiveFunction(const Token * tok);
     void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive);
     void funcArgNamesDifferent(const std::string & functionName, nonneg int index, const Token* declaration, const Token* definition);
     void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);

lib/checkuninitvar.cpp

@@ -1561,51 +1561,6 @@ void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string
                 "$symbol:" + membername + "\nUninitialized struct member: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal);
 }
 
-enum class ExprUsage { None, NotUsed, PassedByReference, Used };
-
-static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings)
-{
-    const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&");
-
-    int argnr;
-    const Token* ftok = getTokenArgumentFunction(tok, argnr);
-    if (!ftok)
-        return ExprUsage::None;
-    if (ftok->function()) {
-        std::vector<const Variable*> args = getArgumentVars(ftok, argnr);
-        for (const Variable* arg : args) {
-            if (!arg)
-                continue;
-            if (arg->isReference())
-                return ExprUsage::PassedByReference;
-        }
-    } else {
-        const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1);
-        if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad)
-            return ExprUsage::Used;
-        bool hasIndirect = false;
-        const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
-        if (isuninitbad && (!addressOf || isnullbad))
-            return ExprUsage::Used;
-    }
-    return ExprUsage::None;
-}
-
-static ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings)
-{
-    if (indirect > 0 && tok->astParent()) {
-        if (Token::Match(tok->astParent(), "%assign%") && astIsRhs(tok))
-            return ExprUsage::NotUsed;
-        if (tok->astParent()->isConstOp())
-            return ExprUsage::NotUsed;
-        if (tok->astParent()->isCast())
-            return ExprUsage::NotUsed;
-    }
-    if (indirect == 0 && Token::Match(tok->astParent(), "%cop%|%assign%|++|--") && tok->astParent()->str() != "=")
-        return ExprUsage::Used;
-    return getFunctionUsage(tok, indirect, settings);
-}
-
 static bool isLeafDot(const Token* tok)
 {
     if (!tok)

test/testother.cpp

@@ -259,6 +259,7 @@ private:
         TEST_CASE(partiallyMoved);
         TEST_CASE(moveAndLambda);
         TEST_CASE(moveInLoop);
+        TEST_CASE(moveCallback);
         TEST_CASE(forwardAndUsed);
 
         TEST_CASE(funcArgNamesDifferent);
@@ -9958,6 +9959,16 @@ private:
         ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable 'l'.\n", errout.str());
     }
 
+    void moveCallback()
+    {
+        check("bool f(std::function<void()>&& callback);\n"
+              "void func(std::function<void()> callback) {\n"
+              "    if(!f(std::move(callback)))\n"
+              "        callback();\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable 'callback'.\n", errout.str());
+    }
+
     void forwardAndUsed() {
         Settings keepTemplates;
         keepTemplates.checkUnusedTemplates = true;