Fix issue 10208: FP: knownConditionTrueFalse in for loop with functio…

…n that assigns by ref (danmar#3198)
nikita215 · Apr 18, 2021 · 563c9dd · 563c9dd
1 parent 56773b8
commit 563c9dd
Show file tree

Hide file tree

Showing 10 changed files with 99 additions and 19 deletions.
diff --git a/gui/threadhandler.cpp b/gui/threadhandler.cpp
@@ -198,7 +198,7 @@ void ThreadHandler::initialize(ResultsView *view)
             this, &ThreadHandler::bughuntingReportLine);
 }
 
-void ThreadHandler::loadSettings(QSettings &settings)
+void ThreadHandler::loadSettings(const QSettings &settings)
 {
     setThreadCount(settings.value(SETTINGS_CHECK_THREADS, 1).toInt());
 }

diff --git a/gui/threadhandler.h b/gui/threadhandler.h
@@ -64,7 +64,7 @@ class ThreadHandler : public QObject {
     * @brief Load settings
     * @param settings QSettings to load settings from
     */
-    void loadSettings(QSettings &settings);
+    void loadSettings(const QSettings &settings);
 
     /**
     * @brief Save settings

diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -1774,12 +1774,14 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
         if (!arg)
             continue;
         conclusive = true;
-        if (addressOf || (indirect > 0 && arg->isPointer())) {
-            if (!arg->isConst())
+        if (addressOf || indirect > 0) {
+            if (!arg->isConst() && arg->isPointer())
                 return true;
             // If const is applied to the pointer, then the value can still be modified
             if (Token::simpleMatch(arg->typeEndToken(), "* const"))
                 return true;
+            if (!arg->isPointer())
+                return true;
         }
         if (!arg->isConst() && arg->isReference())
             return true;
@@ -1790,6 +1792,13 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
     return false;
 }
 
+bool isVariableChangedByFunctionCall(const Token* tok, int indirect, const Settings* settings)
+{
+    bool inconclusive = false;
+    bool r = isVariableChangedByFunctionCall(tok, indirect, settings, &inconclusive);
+    return r || inconclusive;
+}
+
 bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth)
 {
     if (!tok)
@@ -1837,13 +1846,17 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
             const ValueType * valueType = var->valueType();
             isConst = (valueType && valueType->pointer == 1 && valueType->constness == 1);
         }
+        if (isConst)
+            return false;
 
         const Token *ftok = tok->tokAt(2);
+        if (settings)
+            return !settings->library.isFunctionConst(ftok);
+
         const Function * fun = ftok->function();
-        if (!isConst && (!fun || !fun->isConst()))
+        if (!fun)
             return true;
-        else
-            return false;
+        return !fun->isConst();
     }
 
     const Token *ftok = tok2;
@@ -1949,8 +1962,9 @@ Token* findVariableChanged(Token *start, const Token *end, int indirect, const n
             if (globalvar && Token::Match(tok, "%name% ("))
                 // TODO: Is global variable really changed by function call?
                 return tok;
-            // Is aliased function call
-            if (Token::Match(tok, "%var% (") && std::any_of(tok->values().begin(), tok->values().end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) {
+            // Is aliased function call or alias passed to function
+            if ((Token::Match(tok, "%var% (") || isVariableChangedByFunctionCall(tok, 1, settings)) &&
+                std::any_of(tok->values().begin(), tok->values().end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) {
                 bool aliased = false;
                 // If we can't find the expression then assume it was modified
                 if (!getExprTok())

diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp
@@ -585,7 +585,16 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
                     }
                     if (!isLifetimeBorrowed(tok, mSettings))
                         continue;
-                    if (var && !var->isLocal() && !var->isArgument() && !isVariableChanged(tok->next(), tok->scope()->bodyEnd, var->declarationId(), var->isGlobal(), mSettings, mTokenizer->isCPP())) {
+                    const Token* nextTok = nextAfterAstRightmostLeaf(tok->astTop());
+                    if (!nextTok)
+                        nextTok = tok->next();
+                    if (var && !var->isLocal() && !var->isArgument() &&
+                        !isVariableChanged(nextTok,
+                                           tok->scope()->bodyEnd,
+                                           var->declarationId(),
+                                           var->isGlobal(),
+                                           mSettings,
+                                           mTokenizer->isCPP())) {
                         errorDanglngLifetime(tok2, &val);
                         break;
                     }

diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp
@@ -713,7 +713,8 @@ void CheckCondition::multiCondition2()
                         });
                     }
                 }
-                if (Token::Match(tok, "%name% (") && isVariablesChanged(tok, tok->linkAt(1), true, varsInCond, mSettings, mTokenizer->isCPP())) {
+                if (Token::Match(tok, "%name% (") &&
+                    isVariablesChanged(tok, tok->linkAt(1), 0, varsInCond, mSettings, mTokenizer->isCPP())) {
                     break;
                 }
                 if (Token::Match(tok, "%type% (") && nonlocal && isNonConstFunctionCall(tok, mSettings->library)) // non const function call -> bailout if there are nonlocal variables

diff --git a/lib/library.cpp b/lib/library.cpp
@@ -1400,7 +1400,7 @@ bool Library::isFunctionConst(const std::string& functionName, bool pure) const
 }
 bool Library::isFunctionConst(const Token *ftok) const
 {
-    if (ftok->function() && ftok->function()->isAttributeConst())
+    if (ftok->function() && ftok->function()->isConst())
         return true;
     if (isNotLibraryFunction(ftok))
         return false;

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -6215,7 +6215,9 @@ static void valueFlowIteratorInfer(TokenList *tokenlist, const Settings *setting
     }
 }
 
-static std::vector<ValueFlow::Value> getInitListSize(const Token* tok, const Library::Container *container)
+static std::vector<ValueFlow::Value> getInitListSize(const Token* tok,
+                                                     const Library::Container* container,
+                                                     bool known = true)
 {
     std::vector<const Token*> args = getArguments(tok);
     if ((args.size() == 1 && astIsContainer(args[0]) && args[0]->valueType()->container == container) ||
@@ -6229,7 +6231,8 @@ static std::vector<ValueFlow::Value> getInitListSize(const Token* tok, const Lib
     }
     ValueFlow::Value value(args.size());
     value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE;
-    value.setKnown();
+    if (known)
+        value.setKnown();
     return {value};
 }
 
@@ -6238,6 +6241,7 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold
     std::map<int, std::size_t> static_sizes;
     // declaration
     for (const Variable *var : symboldatabase->variableList()) {
+        bool known = true;
         if (!var || !var->isLocal() || var->isPointer() || var->isReference() || var->isStatic())
             continue;
         if (!var->valueType() || !var->valueType()->container)
@@ -6249,17 +6253,20 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold
         if (!Token::Match(var->nameToken(), "%name% ;") &&
             !(Token::Match(var->nameToken(), "%name% {") && Token::simpleMatch(var->nameToken()->next()->link(), "} ;")))
             continue;
+        if (var->nameToken()->astTop() && Token::Match(var->nameToken()->astTop()->previous(), "for|while"))
+            known = !isVariableChanged(var, settings, true);
         if (var->valueType()->container->size_templateArgNo >= 0) {
             if (var->dimensions().size() == 1 && var->dimensions().front().known)
                 static_sizes[var->declarationId()] = var->dimensions().front().num;
             continue;
         }
         std::vector<ValueFlow::Value> values{ValueFlow::Value{0}};
         values.back().valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE;
-        values.back().setKnown();
+        if (known)
+            values.back().setKnown();
         if (Token::simpleMatch(var->nameToken()->next(), "{")) {
             const Token* initList = var->nameToken()->next();
-            values = getInitListSize(initList, var->valueType()->container);
+            values = getInitListSize(initList, var->valueType()->container, known);
         }
         for (const ValueFlow::Value& value : values)
             valueFlowContainerForward(var->nameToken()->next(), var, value, tokenlist);

diff --git a/test/cfg/kde.cpp b/test/cfg/kde.cpp
@@ -14,7 +14,7 @@
 class k_global_static_testclass1 {};
 K_GLOBAL_STATIC(k_global_static_testclass1, k_global_static_testinstance1);
 
-void valid_code(KConfigGroup &cfgGroup)
+void valid_code(const KConfigGroup& cfgGroup)
 {
     k_global_static_testclass1 * pk_global_static_testclass1 = k_global_static_testinstance1;
     printf("%p", pk_global_static_testclass1);
@@ -23,7 +23,7 @@ void valid_code(KConfigGroup &cfgGroup)
     if (entryTest) {}
 }
 
-void ignoredReturnValue(KConfigGroup & cfgGroup)
+void ignoredReturnValue(const KConfigGroup& cfgGroup)
 {
     // cppcheck-suppress ignoredReturnValue
     cfgGroup.readEntry("test", "default");

diff --git a/test/cfg/wxwidgets.cpp b/test/cfg/wxwidgets.cpp
@@ -110,7 +110,7 @@ bool invalidFunctionArgBool_wxPGProperty_Hide(wxPGProperty *pg, bool hide, int f
     return pg->Hide(hide, flags);
 }
 
-wxTextCtrlHitTestResult nullPointer_wxTextCtrl_HitTest(wxTextCtrl &txtCtrl, const wxPoint &pos)
+wxTextCtrlHitTestResult nullPointer_wxTextCtrl_HitTest(const wxTextCtrl& txtCtrl, const wxPoint& pos)
 {
     // no nullPointer-warning is expected
     return txtCtrl.HitTest(pos, NULL);

diff --git a/test/testcondition.cpp b/test/testcondition.cpp
@@ -3561,6 +3561,43 @@ class TestCondition : public TestFixture {
               "  return false;\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        // #10208
+        check("bool GetFirst(std::string &first);\n"
+              "bool GetNext(std::string &next);\n"
+              "void g(const std::string& name);\n"
+              "void f() {\n"
+              "  for (std::string name; name.empty() ? GetFirst(name) : GetNext(name);)\n"
+              "    g(name);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool GetFirst(std::string &first);\n"
+              "bool GetNext(std::string &next);\n"
+              "void g(const std::string& name);\n"
+              "void f() {\n"
+              "  for (std::string name{}; name.empty() ? GetFirst(name) : GetNext(name);)\n"
+              "    g(name);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool GetFirst(std::string &first);\n"
+              "bool GetNext(std::string &next);\n"
+              "void g(const std::string& name);\n"
+              "void f() {\n"
+              "  for (std::string name{'a', 'b'}; name.empty() ? GetFirst(name) : GetNext(name);)\n"
+              "    g(name);\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool GetFirst(const std::string &first);\n"
+              "bool GetNext(const std::string &next);\n"
+              "void g(const std::string& name);\n"
+              "void f() {\n"
+              "  for (std::string name; name.empty() ? GetFirst(name) : GetNext(name);)\n"
+              "    g(name);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'name.empty()' is always true\n", errout.str());
     }
 
     void alwaysTrueInfer() {
@@ -3947,6 +3984,18 @@ class TestCondition : public TestFixture {
               "}\n");
         ASSERT_EQUALS("", errout.str());
 
+        check("void g(std::function<void()>);\n"
+              "void f(std::vector<int> v) {\n"
+              "    auto x = [&v] { v.push_back(1); };\n"
+              "    if(v.empty()) {\n"
+              "        g(x);\n"
+              "    }\n"
+              "    if(v.empty())\n"
+              "        return;\n"
+              "    return;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
         // do not crash
         check("void assign(const MMA& other) {\n"
               "    if (mPA.cols != other.mPA.cols || mPA.rows != other.mPA.rows)\n"