From 9094ff01d3ecc14d5df92b4cf1067dba17685ed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 31 Oct 2019 08:32:39 +0100 Subject: [PATCH] Fixed #9363 (knownConditionTrueFalse: False positive about function parameter) --- lib/checkcondition.cpp | 56 +++++++++++++++++------------------------- test/testcondition.cpp | 27 ++++++++------------ 2 files changed, 32 insertions(+), 51 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 64d5ca2cb..c930fc43e 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1333,11 +1333,28 @@ void CheckCondition::alwaysTrueFalse() const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (tok->link()) // don't write false positives when templates are used continue; if (!tok->hasKnownIntValue()) continue; + { + // is this a condition.. + const Token *parent = tok->astParent(); + while (Token::Match(parent, "%oror%|&&")) + parent = parent->astParent(); + if (!parent) + continue; + const Token *condition = nullptr; + if (parent->str() == "?" && precedes(tok, parent)) + condition = parent->astOperand1(); + else if (Token::Match(parent->previous(), "if|while (")) + condition = parent->astOperand2(); + else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && Token::simpleMatch(parent->astParent()->astParent()->previous(), "for (")) + condition = parent->astOperand1(); + else + continue; + (void)condition; + } // Skip already diagnosed values if (diag(tok, false)) continue; @@ -1355,38 +1372,9 @@ void CheckCondition::alwaysTrueFalse() (Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while")); const bool constValExpr = tok->isNumber() && Token::Match(tok->astParent(),"%oror%|&&|?"); // just one number in boolean expression const bool compExpr = Token::Match(tok, "%comp%|!"); // a compare expression - const bool returnStatement = Token::simpleMatch(tok->astTop(), "return") && - Token::Match(tok->astParent(), "%oror%|&&|return"); const bool ternaryExpression = Token::simpleMatch(tok->astParent(), "?"); - if (!(constIfWhileExpression || constValExpr || compExpr || returnStatement || ternaryExpression)) - continue; - - if (returnStatement && (!scope->function || !Token::simpleMatch(scope->function->retDef, "bool"))) - continue; - - if (returnStatement && isConstVarExpression(tok)) - continue; - - if (returnStatement && Token::simpleMatch(tok->astParent(), "return") && tok->variable() && ( - !tok->variable()->isLocal() || - tok->variable()->isReference() || - tok->variable()->isConst() || - !isVariableChanged(tok->variable(), mSettings, mTokenizer->isCPP()))) - continue; - - // Don't warn in assertions. Condition is often 'always true' by intention. - // If platform,defines,etc cause 'always false' then that is not dangerous neither. - bool assertFound = false; - for (const Token * tok2 = tok->astParent(); tok2 ; tok2 = tok2->astParent()) { // move backwards and try to find "assert" - if (tok2->str() == "(" && tok2->astOperand2()) { - const std::string& str = tok2->previous()->str(); - if ((str.find("assert")!=std::string::npos || str.find("ASSERT")!=std::string::npos)) - assertFound = true; - break; - } - } - if (assertFound) + if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression)) continue; // Don't warn when there are expanded macros.. @@ -1446,15 +1434,15 @@ void CheckCondition::alwaysTrueFalse() void CheckCondition::alwaysTrueFalseError(const Token *tok, const ValueFlow::Value *value) { - const bool condvalue = value && (value->intvalue != 0); + const bool alwaysTrue = value && (value->intvalue != 0); const std::string expr = tok ? tok->expressionString() : std::string("x"); - const std::string errmsg = "Condition '" + expr + "' is always " + (condvalue ? "true" : "false"); + const std::string errmsg = "Condition '" + expr + "' is always " + (alwaysTrue ? "true" : "false"); const ErrorPath errorPath = getErrorPath(tok, value, errmsg); reportError(errorPath, Severity::style, "knownConditionTrueFalse", errmsg, - (condvalue ? CWE571 : CWE570), false); + (alwaysTrue ? CWE571 : CWE570), false); } void CheckCondition::checkInvalidTestForOverflow() diff --git a/test/testcondition.cpp b/test/testcondition.cpp index beaa5a6ff..38bcd94a7 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -2681,8 +2681,7 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Suspicious condition (bitwise operator + comparison); Clarify expression with parentheses.\n" "[test.cpp:2]: (style) Boolean result is used in bitwise operation. Clarify expression with parentheses.\n" - "[test.cpp:2]: (style) Condition 'x&3==2' is always false\n" - "[test.cpp:2]: (style) Condition '3==2' is always false\n", errout.str()); + "[test.cpp:2]: (style) Condition 'x&3==2' is always false\n", errout.str()); check("void f() {\n" " if (a & fred1.x == fred2.y) {}\n" @@ -2834,21 +2833,6 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4]: (style) Condition '!x' is always true\n", errout.str()); - check("bool f(int x) {\n" - " if(x == 0) { x++; return x == 0; } \n" - " return false;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0' is always false\n", errout.str()); - - check("void f() {\n" // #6898 (Token::expressionString) - " int x = 0;\n" - " A(x++ == 1);\n" - " A(x++ == 2);\n" - "}"); - 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("void f1(const std::string &s) { if(s.empty()) if(s.size() == 0) {}} "); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (style) Condition 's.size()==0' is always true\n", errout.str()); @@ -2953,6 +2937,15 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + // #9363 - do not warn about value passed to function + check("void f(bool b) {\n" + " if (b) {\n" + " if (bar(!b)) {}\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // #7783 FP knownConditionTrueFalse on assert(0 && "message") check("void foo(int x) {\n" " if (x<0)\n"