From 03b952d5ebab180be9976f6007b8b367f5d50109 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 20 Aug 2023 15:32:41 -0500 Subject: [PATCH] Fix 11579: false negative: knownConditionTrueFalse with non-bool as bool parameter (#5349) This adds a new checker to check for pointer to bool conversions that are always known. I removed the previous knownConditionTrueFalse checks since this was too noisy. --- lib/astutils.cpp | 19 ++++++-- lib/astutils.h | 2 +- lib/checkcondition.cpp | 15 +++--- lib/checkcondition.h | 2 +- lib/checkother.cpp | 100 +++++++++++++++++++++++++++------------- lib/checkother.h | 5 ++ lib/forwardanalyzer.cpp | 2 +- lib/token.cpp | 9 ---- lib/token.h | 1 - lib/valueflow.cpp | 3 +- releasenotes.txt | 1 + test/testastutils.cpp | 2 +- test/testcondition.cpp | 16 +++---- test/testother.cpp | 69 +++++++++++++++++++++++++++ 14 files changed, 181 insertions(+), 65 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 78252680e..868c48f9a 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -696,9 +696,10 @@ std::vector getParentValueTypes(const Token* tok, const Settings* set return {*tok->astParent()->astOperand1()->valueType()}; return {}; } + const Token* ftok = nullptr; if (Token::Match(tok->astParent(), "(|{|,")) { int argn = -1; - const Token* ftok = getTokenArgumentFunction(tok, argn); + ftok = getTokenArgumentFunction(tok, argn); const Token* typeTok = nullptr; if (ftok && argn >= 0) { if (ftok->function()) { @@ -741,6 +742,9 @@ std::vector getParentValueTypes(const Token* tok, const Settings* set ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, *settings); return {std::move(vtParent)}; } + // The return type of a function is not the parent valuetype + if (Token::simpleMatch(tok->astParent(), "(") && ftok && !tok->isCast() && ftok->tokType() != Token::eType) + return {}; if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) { *parent = tok->astParent(); } @@ -1422,7 +1426,7 @@ static bool isForLoopIncrement(const Token* const tok) parent->astParent()->astParent()->astOperand1()->str() == "for"; } -bool isUsedAsBool(const Token* const tok) +bool isUsedAsBool(const Token* const tok, const Settings* settings) { if (!tok) return false; @@ -1435,6 +1439,15 @@ bool isUsedAsBool(const Token* const tok) const Token* parent = tok->astParent(); if (!parent) return false; + if (Token::simpleMatch(parent, "[")) + return false; + if (parent->isUnaryOp("*")) + return false; + if (Token::simpleMatch(parent, ".")) { + if (astIsRHS(tok)) + return isUsedAsBool(parent, settings); + return false; + } if (Token::Match(parent, "&&|!|%oror%")) return true; if (parent->isCast()) @@ -1451,7 +1464,7 @@ bool isUsedAsBool(const Token* const tok) if (isForLoopCondition(tok)) return true; if (!Token::Match(parent, "%cop%")) { - std::vector vtParents = getParentValueTypes(tok); + std::vector vtParents = getParentValueTypes(tok, settings); return std::any_of(vtParents.cbegin(), vtParents.cend(), [&](const ValueType& vt) { return vt.pointer == 0 && vt.type == ValueType::BOOL; }); diff --git a/lib/astutils.h b/lib/astutils.h index 0ef08ece0..af5f6aff6 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -254,7 +254,7 @@ const Token* isInLoopCondition(const Token* tok); /** * 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); +CPPCHECKLIB bool isUsedAsBool(const Token* const tok, const Settings* settings = nullptr); /** * Is token passed to a function taking a bool argument diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 8219d04b1..715ec55af 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -60,7 +60,7 @@ bool CheckCondition::diag(const Token* tok, bool insert) return false; const Token* parent = tok->astParent(); bool hasParent = false; - while (Token::Match(parent, "&&|%oror%")) { + while (Token::Match(parent, "!|&&|%oror%")) { if (mCondDiags.count(parent) != 0) { hasParent = true; break; @@ -410,6 +410,8 @@ void CheckCondition::comparison() void CheckCondition::comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result) { + if (tok && (diag(tok) | diag(tok->astParent()))) + return; std::ostringstream expression; expression << std::hex << "(X " << bitop << " 0x" << value1 << ") " << op << " 0x" << value2; @@ -1370,6 +1372,8 @@ void CheckCondition::checkModuloAlwaysTrueFalse() void CheckCondition::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal) { + if (diag(tok)) + return; reportError(tok, Severity::warning, "moduloAlwaysTrueFalse", "Comparison of modulo result is predetermined, because it is always less than " + maxVal + ".", CWE398, Certainty::normal); } @@ -1466,7 +1470,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->hasKnownBoolValue()) + if (!tok->hasKnownIntValue()) continue; if (Token::Match(tok->previous(), "%name% (") && tok->previous()->function()) { const Function* f = tok->previous()->function(); @@ -1490,7 +1494,7 @@ 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)) + else if (Token::Match(tok, "%comp%")) condition = tok; else continue; @@ -1582,10 +1586,7 @@ void CheckCondition::alwaysTrueFalse() if (hasSizeof) continue; - auto it = std::find_if(tok->values().begin(), tok->values().end(), [](const ValueFlow::Value& v) { - return v.isIntValue(); - }); - alwaysTrueFalseError(tok, condition, &*it); + alwaysTrueFalseError(tok, condition, &tok->values().front()); } } } diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 948dd7be2..4c75a8a93 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -67,12 +67,12 @@ public: checkCondition.checkPointerAdditionResultNotNull(); checkCondition.checkDuplicateConditionalAssign(); checkCondition.assignIf(); - checkCondition.alwaysTrueFalse(); checkCondition.checkBadBitmaskCheck(); checkCondition.comparison(); checkCondition.checkModuloAlwaysTrueFalse(); checkCondition.checkAssignmentInCondition(); checkCondition.checkCompareValueOutOfTypeRange(); + checkCondition.alwaysTrueFalse(); } /** mismatching assignment / comparison */ diff --git a/lib/checkother.cpp b/lib/checkother.cpp index deaadbf7c..05801b4ad 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3640,6 +3640,21 @@ static bool isVariableExpression(const Token* tok) return false; } +static bool isVariableExprHidden(const Token* tok) +{ + if (!tok) + return false; + if (!tok->astParent()) + return false; + if (Token::simpleMatch(tok->astParent(), "*") && Token::simpleMatch(tok->astSibling(), "0")) + return true; + if (Token::simpleMatch(tok->astParent(), "&&") && Token::simpleMatch(tok->astSibling(), "false")) + return true; + if (Token::simpleMatch(tok->astParent(), "||") && Token::simpleMatch(tok->astSibling(), "true")) + return true; + return false; +} + void CheckOther::checkKnownArgument() { if (!mSettings->severity.isEnabled(Severity::style)) @@ -3679,44 +3694,25 @@ void CheckOther::checkKnownArgument() mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true)) continue; // ensure that there is a integer variable in expression with unknown value - std::string varexpr; - bool isVariableExprHidden = false; // Is variable expression explicitly hidden - auto setVarExpr = [&varexpr, &isVariableExprHidden](const Token *child) { + const Token* vartok = findAstNode(tok, [](const Token* child) { if (Token::Match(child, "%var%|.|[")) { - if (child->valueType() && child->valueType()->pointer == 0 && child->valueType()->isIntegral() && child->values().empty()) { - varexpr = child->expressionString(); - return ChildrenToVisit::done; - } - return ChildrenToVisit::none; + return astIsIntegral(child, false) && !astIsPointer(child) && child->values().empty(); } - if (Token::simpleMatch(child->previous(), "sizeof (")) - return ChildrenToVisit::none; - - // hide variable explicitly with 'x * 0' etc - if (!isVariableExprHidden) { - if (Token::simpleMatch(child, "*") && (Token::simpleMatch(child->astOperand1(), "0") || Token::simpleMatch(child->astOperand2(), "0"))) - return ChildrenToVisit::none; - if (Token::simpleMatch(child, "&&") && (Token::simpleMatch(child->astOperand1(), "false") || Token::simpleMatch(child->astOperand2(), "false"))) - return ChildrenToVisit::none; - if (Token::simpleMatch(child, "||") && (Token::simpleMatch(child->astOperand1(), "true") || Token::simpleMatch(child->astOperand2(), "true"))) - return ChildrenToVisit::none; - } - - return ChildrenToVisit::op1_and_op2; - }; - visitAstNodes(tok, setVarExpr); - if (varexpr.empty()) { - isVariableExprHidden = true; - visitAstNodes(tok, setVarExpr); - } - if (varexpr.empty()) + return false; + }); + if (!vartok) + continue; + if (vartok->astSibling() && + findAstNode(vartok->astSibling(), [](const Token* child) { + return Token::simpleMatch(child, "sizeof"); + })) continue; // ensure that function name does not contain "assert" - std::string funcname = tok->astParent()->previous()->str(); + std::string funcname = ftok->str(); strTolower(funcname); if (funcname.find("assert") != std::string::npos) continue; - knownArgumentError(tok, ftok, &tok->values().front(), varexpr, isVariableExprHidden); + knownArgumentError(tok, ftok, &tok->values().front(), vartok->expressionString(), isVariableExprHidden(vartok)); } } } @@ -3747,6 +3743,48 @@ void CheckOther::knownArgumentError(const Token *tok, const Token *ftok, const V reportError(errorPath, Severity::style, id, errmsg, CWE570, Certainty::normal); } +void CheckOther::checkKnownPointerToBool() +{ + if (!mSettings->severity.isEnabled(Severity::style)) + return; + const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope* functionScope : symbolDatabase->functionScopes) { + for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { + if (!tok->hasKnownIntValue()) + continue; + if (!astIsPointer(tok)) + continue; + if (Token::Match(tok->astParent(), "?|!|&&|%oror%|%comp%")) + continue; + if (tok->astParent() && Token::Match(tok->astParent()->previous(), "if|while|switch|sizeof (")) + continue; + if (tok->isExpandedMacro()) + continue; + if (findParent(tok, [](const Token* parent) { + return parent->isExpandedMacro(); + })) + continue; + if (!isUsedAsBool(tok, mSettings)) + continue; + const ValueFlow::Value& value = tok->values().front(); + knownPointerToBoolError(tok, &value); + } + } +} + +void CheckOther::knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value) +{ + if (!tok) { + reportError(tok, Severity::style, "knownPointerToBool", "Pointer expression 'p' converted to bool is always true."); + return; + } + std::string cond = value->intvalue ? "true" : "false"; + const std::string& expr = tok->expressionString(); + std::string errmsg = "Pointer expression '" + expr + "' converted to bool is always " + cond + "."; + const ErrorPath errorPath = getErrorPath(tok, value, errmsg); + reportError(errorPath, Severity::style, "knownPointerToBool", errmsg, CWE570, Certainty::normal); +} + void CheckOther::checkComparePointers() { const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); diff --git a/lib/checkother.h b/lib/checkother.h index b38c82e2c..69a62b9b1 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -85,6 +85,7 @@ public: checkOther.checkFuncArgNamesDifferent(); checkOther.checkShadowVariables(); checkOther.checkKnownArgument(); + checkOther.checkKnownPointerToBool(); checkOther.checkComparePointers(); checkOther.checkIncompleteStatement(); checkOther.checkRedundantCopy(); @@ -218,6 +219,8 @@ public: void checkKnownArgument(); + void checkKnownPointerToBool(); + void checkComparePointers(); void checkModuloOfOne(); @@ -279,6 +282,7 @@ private: void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector & declarations, const std::vector & definitions); void shadowError(const Token *var, const Token *shadowed, std::string type); void knownArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value, const std::string &varexpr, bool isVariableExpressionHidden); + void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value); void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2); void checkModuloOfOneError(const Token *tok); @@ -348,6 +352,7 @@ private: c.shadowError(nullptr, nullptr, "function"); c.shadowError(nullptr, nullptr, "argument"); c.knownArgumentError(nullptr, nullptr, nullptr, "x", false); + c.knownPointerToBoolError(nullptr, nullptr); c.comparePointersError(nullptr, nullptr, nullptr); c.redundantAssignmentError(nullptr, nullptr, "var", false); c.redundantInitializationError(nullptr, nullptr, "var", false); diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 09abde664..fea0526eb 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, /*isModified*/ true); + std::vector ftv = tryForkScope(endBlock, allAnalysis.isModified()); bool forkContinue = true; for (ForwardTraversal& ft : ftv) { if (condTok) diff --git a/lib/token.cpp b/lib/token.cpp index 6c832f3a7..a13f98763 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -2388,15 +2388,6 @@ 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 b82326381..dfd601a76 100644 --- a/lib/token.h +++ b/lib/token.h @@ -1216,7 +1216,6 @@ 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/valueflow.cpp b/lib/valueflow.cpp index c650e12b6..3572dddd3 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -6843,7 +6843,8 @@ static void valueFlowInferCondition(TokenList& tokenlist, } } } else if (Token::Match(tok->astParent(), "?|&&|!|%oror%") || - Token::Match(tok->astParent()->previous(), "if|while (")) { + Token::Match(tok->astParent()->previous(), "if|while (") || + (astIsPointer(tok) && isUsedAsBool(tok, settings))) { std::vector result = infer(IntegralInferModel{}, "!=", tok->values(), 0); if (result.size() != 1) continue; diff --git a/releasenotes.txt b/releasenotes.txt index 57e6d0210..c54c20094 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -2,6 +2,7 @@ Release Notes for Cppcheck 2.12 New checks: - uselessOverride finds overriding functions that either duplicate code from or delegate back to the base class implementation +- knownPointerToBool finds pointer to bool conversions that are always true or false Improved checking: - truncLongCastAssignment and truncLongCastReturn check additional types, including float/double/long double diff --git a/test/testastutils.cpp b/test/testastutils.cpp index d14d35640..fe31e4783 100644 --- a/test/testastutils.cpp +++ b/test/testastutils.cpp @@ -484,7 +484,7 @@ private: ASSERT(Result::False == isUsedAsBool("void f() { int i; if (i + 2) {} }", "i +")); ASSERT(Result::True == isUsedAsBool("void f() { int i; bool b = i; }", "i ; }")); ASSERT(Result::True == isUsedAsBool("void f(bool b); void f() { int i; f(i); }","i )")); - ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )")); + ASSERT(Result::False == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )")); ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "* i )")); ASSERT(Result::True == isUsedAsBool("int g(); void h(bool); void f() { h(g()); }", "( ) )")); ASSERT(Result::True == isUsedAsBool("int g(int); void h(bool); void f() { h(g(0)); }", "( 0 ) )")); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index b60be8b83..34abf929b 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1879,7 +1879,6 @@ private: " return a % 5 > 5;\n" "}"); ASSERT_EQUALS( - "[test.cpp:7]: (style) Return value 'a%5>5' is always false\n" "[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" @@ -1894,7 +1893,6 @@ private: " b2 = x.a % 5 == 5;\n" "}"); ASSERT_EQUALS( - "[test.cpp:3]: (style) Condition 'x[593]%5<=5' is always true\n" "[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", @@ -3223,7 +3221,9 @@ private: " A(x++ == 1);\n" " A(x++ == 2);\n" "}"); - TODO_ASSERT_EQUALS("function argument is always true? however is code really weird/suspicious?", "", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'x++==1' is always false\n" + "[test.cpp:4]: (style) Condition 'x++==2' is always false\n", + errout.str()); check("bool foo(int bar) {\n" " bool ret = false;\n" @@ -4229,7 +4229,9 @@ private: " if (w) {}\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'w' is always true\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style) Condition 'v<2' is always true\n" + "[test.cpp:5]: (style) Condition 'w' is always true\n", + errout.str()); check("void f(double d) {\n" // #10792 " if (d != 0) {\n" @@ -4521,13 +4523,9 @@ private: " 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", + "[test.cpp:6]: (style) Condition 'p==nullptr' is always false\n", errout.str()); } diff --git a/test/testother.cpp b/test/testother.cpp index d789c3bb1..b8c48f797 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -290,6 +290,8 @@ private: TEST_CASE(checkOverlappingWrite); TEST_CASE(constVariableArrayMember); // #10371 + + TEST_CASE(knownPointerToBool); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -11340,6 +11342,73 @@ private: "};\n"); ASSERT_EQUALS("", errout.str()); } + + void knownPointerToBool() + { + check("void g(bool);\n" + "void f() {\n" + " int i = 5;\n" + " int* p = &i;\n" + " g(p);\n" + " g(&i);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) Pointer expression 'p' converted to bool is always true.\n" + "[test.cpp:6]: (style) Pointer expression '&i' converted to bool is always true.\n", + errout.str()); + + check("void f() {\n" + " const int* x = nullptr;\n" + " std::empty(x);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A { bool x; };\n" + "bool f(A* a) {\n" + " if (a) {\n" + " return a->x;\n" + " }\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A { int* x; };\n" + "bool f(A a) {\n" + " if (a.x) {\n" + " return a.x;\n" + " }\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'a.x' converted to bool is always true.\n", errout.str()); + + check("void f(bool* b) { if (b) *b = true; }"); + ASSERT_EQUALS("", errout.str()); + + check("bool f() {\n" + " int* x = nullptr;\n" + " return bool(x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str()); + + check("bool f() {\n" + " int* x = nullptr;\n" + " return bool{x};\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str()); + + check("struct A { A(bool); };\n" + "A f() {\n" + " int* x = nullptr;\n" + " return A(x);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str()); + + check("struct A { A(bool); };\n" + "A f() {\n" + " int* x = nullptr;\n" + " return A{x};\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str()); + } }; REGISTER_TEST(TestOther)