From 827e87afe793a0f04c59456335f808d282eae59b Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Fri, 18 Aug 2023 10:32:52 +0200 Subject: [PATCH] Fix #11579 FN knownConditionTrueFalse with non-bool as bool parameter / #9450 string literal to bool conversion in function call (#5338) --- lib/astutils.cpp | 13 +++++++++++++ lib/astutils.h | 7 ++++++- lib/checkcondition.cpp | 11 ++++++++--- lib/checkstring.cpp | 4 +++- lib/forwardanalyzer.cpp | 2 +- lib/token.cpp | 9 +++++++++ lib/token.h | 1 + lib/tokenlist.cpp | 2 +- test/testcondition.cpp | 15 +++++++++++++++ test/teststring.cpp | 7 +++++++ 10 files changed, 64 insertions(+), 7 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 397ad5337..bc85cfab9 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1464,6 +1464,19 @@ static bool astIsBoolLike(const Token* tok) return astIsBool(tok) || isUsedAsBool(tok); } +bool isBooleanFuncArg(const Token* tok) { + if (tok->variable() && tok->variable()->valueType() && tok->variable()->valueType()->type == ValueType::BOOL) // skip trivial case: bool passed as bool + return false; + int argn{}; + const Token* ftok = getTokenArgumentFunction(tok, argn); + if (!ftok) + return false; + std::vector argvars = getArgumentVars(ftok, argn); + if (argvars.size() != 1) + return false; + return !argvars[0]->isReference() && argvars[0]->valueType() && argvars[0]->valueType()->type == ValueType::BOOL; +} + bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors) { if (tok1 == nullptr && tok2 == nullptr) diff --git a/lib/astutils.h b/lib/astutils.h index 4ffd0aa79..0ef08ece0 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -252,10 +252,15 @@ bool isStructuredBindingVariable(const Variable* var); const Token* isInLoopCondition(const Token* tok); /** - * Is token used a boolean, that is to say cast to a bool, or used as a condition in a if/while/for + * Is token used as boolean, that is to say cast to a bool, or used as a condition in a if/while/for */ CPPCHECKLIB bool isUsedAsBool(const Token * const tok); +/** + * Is token passed to a function taking a bool argument + */ +CPPCHECKLIB bool isBooleanFuncArg(const Token* tok); + /** * Are two conditions opposite * @param isNot do you want to know if cond1 is !cond2 or if cond1 and cond2 are non-overlapping. true: cond1==!cond2 false: cond1==true => cond2==false diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 2d018f1fb..8219d04b1 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1466,7 +1466,7 @@ void CheckCondition::alwaysTrueFalse() for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { if (Token::simpleMatch(tok, "<") && tok->link()) // don't write false positives when templates are used continue; - if (!tok->hasKnownIntValue()) + if (!tok->hasKnownBoolValue()) continue; if (Token::Match(tok->previous(), "%name% (") && tok->previous()->function()) { const Function* f = tok->previous()->function(); @@ -1490,6 +1490,8 @@ void CheckCondition::alwaysTrueFalse() else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && Token::simpleMatch(parent->astParent()->astParent()->previous(), "for (")) condition = parent->astParent()->astParent()->previous(); + else if (isBooleanFuncArg(tok)) + condition = tok; else continue; } @@ -1580,14 +1582,17 @@ void CheckCondition::alwaysTrueFalse() if (hasSizeof) continue; - alwaysTrueFalseError(tok, condition, &tok->values().front()); + auto it = std::find_if(tok->values().begin(), tok->values().end(), [](const ValueFlow::Value& v) { + return v.isIntValue(); + }); + alwaysTrueFalseError(tok, condition, &*it); } } } void CheckCondition::alwaysTrueFalseError(const Token* tok, const Token* condition, const ValueFlow::Value* value) { - const bool alwaysTrue = value && (value->intvalue != 0); + const bool alwaysTrue = value && (value->intvalue != 0 || value->isImpossible()); const std::string expr = tok ? tok->expressionString() : std::string("x"); const std::string conditionStr = (Token::simpleMatch(condition, "return") ? "Return value" : "Condition"); const std::string errmsg = conditionStr + " '" + expr + "' is always " + (alwaysTrue ? "true" : "false"); diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp index cd6617abb..afdad191c 100644 --- a/lib/checkstring.cpp +++ b/lib/checkstring.cpp @@ -292,8 +292,10 @@ void CheckString::checkIncorrectStringCompare() incorrectStringBooleanError(tok->next(), tok->strAt(1)); } else if (Token::Match(tok, "if|while ( %str%|%char% )") && !tok->tokAt(2)->getValue(0)) { incorrectStringBooleanError(tok->tokAt(2), tok->strAt(2)); - } else if (tok->str() == "?" && Token::Match(tok->astOperand1(), "%str%|%char%")) + } else if (tok->str() == "?" && Token::Match(tok->astOperand1(), "%str%|%char%")) { incorrectStringBooleanError(tok->astOperand1(), tok->astOperand1()->str()); + } else if (Token::Match(tok, "%str%") && isBooleanFuncArg(tok)) + incorrectStringBooleanError(tok, tok->str()); } } } diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index fea0526eb..09abde664 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -488,7 +488,7 @@ struct ForwardTraversal { if (allAnalysis.isIncremental()) return Break(Analyzer::Terminate::Bail); } else if (allAnalysis.isModified()) { - std::vector ftv = tryForkScope(endBlock, allAnalysis.isModified()); + std::vector ftv = tryForkScope(endBlock, /*isModified*/ true); bool forkContinue = true; for (ForwardTraversal& ft : ftv) { if (condTok) diff --git a/lib/token.cpp b/lib/token.cpp index 3f5a539c9..618979613 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -2371,6 +2371,15 @@ bool Token::hasKnownIntValue() const }); } +bool Token::hasKnownBoolValue() const +{ + if (!mImpl->mValues) + return false; + return std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) { + return value.isIntValue() && (value.isKnown() || (value.intvalue == 0 && value.isImpossible())); + }); +} + bool Token::hasKnownValue() const { return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown)); diff --git a/lib/token.h b/lib/token.h index 4163ab754..e0ca0ed9d 100644 --- a/lib/token.h +++ b/lib/token.h @@ -1212,6 +1212,7 @@ public: } bool hasKnownIntValue() const; + bool hasKnownBoolValue() const; bool hasKnownValue() const; bool hasKnownValue(ValueFlow::Value::ValueType t) const; bool hasKnownSymbolicValue(const Token* tok) const; diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp index c6e374f8a..34dbc2e0e 100644 --- a/lib/tokenlist.cpp +++ b/lib/tokenlist.cpp @@ -912,7 +912,7 @@ static void compilePrecedence2(Token *&tok, AST_state& state) } compileBinOp(tok, state, compileScope); } else if (tok->str() == "[") { - if (state.cpp && isPrefixUnary(tok, state.cpp) && Token::Match(tok->link(), "] (|{|<")) { // Lambda + if (state.cpp && isPrefixUnary(tok, /*cpp*/ true) && Token::Match(tok->link(), "] (|{|<")) { // Lambda // What we do here: // - Nest the round bracket under the square bracket. // - Nest what follows the lambda (if anything) with the lambda opening [ diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 5cca85c05..98fef8280 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -4514,6 +4514,21 @@ private: " return a;\n" "}\n"); ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'a==\"x\"' is always true\n", errout.str()); + + check("void g(bool);\n" + "void f() {\n" + " int i = 5;\n" + " int* p = &i;\n" + " g(i == 7);\n" + " g(p == nullptr);\n" + " g(p);\n" + " g(&i);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'i==7' is always false\n" + "[test.cpp:6]: (style) Condition 'p==nullptr' is always false\n" + "[test.cpp:7]: (style) Condition 'p' is always true\n" + "[test.cpp:8]: (style) Condition '&i' is always true\n", + errout.str()); } void alwaysTrueSymbolic() diff --git a/test/teststring.cpp b/test/teststring.cpp index 24a4f1c3a..ca734d352 100644 --- a/test/teststring.cpp +++ b/test/teststring.cpp @@ -743,6 +743,13 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n" "[test.cpp:3]: (warning) Conversion of char literal L'\\0' to bool always evaluates to false.\n", errout.str()); + + check("void f(bool b);\n" // #9450 + "void f(std::string s);\n" + "void g() {\n" + " f(\"abc\");\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Conversion of string literal \"abc\" to bool always evaluates to true.\n", errout.str()); } void deadStrcmp() {