From 563c9dd9ccf7568e7bbe9a37efca892c3fb417ad Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 18 Apr 2021 14:42:27 -0500 Subject: [PATCH] Fix issue 10208: FP: knownConditionTrueFalse in for loop with function that assigns by ref (#3198) --- gui/threadhandler.cpp | 2 +- gui/threadhandler.h | 2 +- lib/astutils.cpp | 28 ++++++++++++++++------ lib/checkautovariables.cpp | 11 ++++++++- lib/checkcondition.cpp | 3 ++- lib/library.cpp | 2 +- lib/valueflow.cpp | 15 ++++++++---- test/cfg/kde.cpp | 4 ++-- test/cfg/wxwidgets.cpp | 2 +- test/testcondition.cpp | 49 ++++++++++++++++++++++++++++++++++++++ 10 files changed, 99 insertions(+), 19 deletions(-) diff --git a/gui/threadhandler.cpp b/gui/threadhandler.cpp index e473cf486ea..13298d741f8 100644 --- 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 index 44c67155e89..fed4be83598 100644 --- 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 index 16fafe21b43..d18a224e4a7 100644 --- 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 index cae4f4b33fb..109dcb475e0 100644 --- 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 index c6513dddb23..ec3590fcb06 100644 --- 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 index 07be01b9a1e..00646a6c973 100644 --- 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 index c463a830700..69cb7612051 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -6215,7 +6215,9 @@ static void valueFlowIteratorInfer(TokenList *tokenlist, const Settings *setting } } -static std::vector getInitListSize(const Token* tok, const Library::Container *container) +static std::vector getInitListSize(const Token* tok, + const Library::Container* container, + bool known = true) { std::vector args = getArguments(tok); if ((args.size() == 1 && astIsContainer(args[0]) && args[0]->valueType()->container == container) || @@ -6229,7 +6231,8 @@ static std::vector 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 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,6 +6253,8 @@ 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; @@ -6256,10 +6262,11 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold } std::vector 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 index c238bf7eac4..f7c01fc3394 100644 --- 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 index 5ac25fa86b4..75f71838fe1 100644 --- 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 index a0d402b4e0c..eb80501d94b 100644 --- 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);\n" + "void f(std::vector 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"