Fix #11579 FN knownConditionTrueFalse with non-bool as bool parameter…

… / #9450 string literal to bool conversion in function call (#5338)
danmar · Aug 18, 2023 · 827e87a · 827e87a
1 parent 5dbcea3
commit 827e87a
Show file tree

Hide file tree

Showing 10 changed files with 64 additions and 7 deletions.
diff --git 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<const Variable*> 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
@@ -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
@@ -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
@@ -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
@@ -488,7 +488,7 @@ struct ForwardTraversal {
             if (allAnalysis.isIncremental())
                 return Break(Analyzer::Terminate::Bail);
         } else if (allAnalysis.isModified()) {
-            std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
+            std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, /*isModified*/ true);
             bool forkContinue = true;
             for (ForwardTraversal& ft : ftv) {
                 if (condTok)

diff --git 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
@@ -1212,6 +1212,7 @@ class CPPCHECKLIB Token {
     }
 
     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
@@ -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
@@ -4514,6 +4514,21 @@ class TestCondition : public TestFixture {
               "    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
@@ -743,6 +743,13 @@ class TestString : public TestFixture {
               "}");
         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() {