From 40cb9cb1bc86647de3a357bd67bc063883b99acd Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 18 Oct 2018 14:01:47 -0500 Subject: [PATCH] Check conditions in return statements (#1411) * Identify return conditions in multiconditions * Improve error messages * Check return statements are always true or false * Add more tests for FPs * Fix FP when returning const like variables * Fix FP when returning pointers or classes * Fix FP with member variable access * Check non-local variables * Use simplematch * Check for null --- lib/astutils.cpp | 14 ++ lib/astutils.h | 3 + lib/checkcondition.cpp | 303 +++++++++++++++++++++++------------------ test/testcondition.cpp | 74 ++++++++++ 4 files changed, 264 insertions(+), 130 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 293e7b6a1..6ae995f77 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -878,6 +878,20 @@ bool isVariableChanged(const Token *start, const Token *end, const unsigned int return false; } +bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp) +{ + if(!var) + return false; + if(!var->scope()) + return false; + const Token * start = var->declEndToken(); + if(!start) + return false; + if(Token::Match(start, "; %varid% =", var->declarationId())) + start = start->tokAt(2); + return isVariableChanged(start->next(), var->scope()->bodyEnd, var->declarationId(), var->isGlobal(), settings, cpp); +} + int numberOfArguments(const Token *start) { int arguments=0; diff --git a/lib/astutils.h b/lib/astutils.h index 610e8adea..b170f25eb 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -30,6 +30,7 @@ class Library; class Settings; class Token; +class Variable; /** Is expression a 'signed char' if no promotion is used */ bool astIsSignedChar(const Token *tok); @@ -110,6 +111,8 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, /** Is variable changed in block of code? */ bool isVariableChanged(const Token *start, const Token *end, const unsigned int varid, bool globalvar, const Settings *settings, bool cpp); +bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp); + /** Determines the number of arguments - if token is a function call or macro * @param start token which is supposed to be the function/macro name. * \return Number of arguments diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 9a2e67710..ada0213a2 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -535,163 +535,193 @@ void CheckCondition::multiCondition2() continue; // parse until second condition is reached.. - enum MULTICONDITIONTYPE { INNER, AFTER } type; + enum MULTICONDITIONTYPE { INNER, AFTER }; const Token *tok; - if (Token::Match(scope.bodyStart, "{ return|throw|continue|break")) { - tok = scope.bodyEnd->next(); - type = MULTICONDITIONTYPE::AFTER; - } else { - tok = scope.bodyStart; - type = MULTICONDITIONTYPE::INNER; - } - const Token * const endToken = tok->scope()->bodyEnd; - for (; tok && tok != endToken; tok = tok->next()) { - if (Token::simpleMatch(tok, "if (")) { - // Does condition modify tracked variables? - if (const Token *op = Token::findmatch(tok, "++|--", tok->linkAt(1))) { - bool bailout = false; - while (op) { - if (vars.find(op->astOperand1()->varId()) != vars.end()) { - bailout = true; - break; + // Parse inner condition first and then early return condition + std::vector types = {MULTICONDITIONTYPE::INNER}; + if (Token::Match(scope.bodyStart, "{ return|throw|continue|break")) + types.push_back(MULTICONDITIONTYPE::AFTER); + for(MULTICONDITIONTYPE type:types) { + if(type == MULTICONDITIONTYPE::AFTER) { + tok = scope.bodyEnd->next(); + } else { + tok = scope.bodyStart; + } + const Token * const endToken = tok->scope()->bodyEnd; + + for (; tok && tok != endToken; tok = tok->next()) { + if (Token::Match(tok, "if|return")) { + const Token * condStartToken = tok->str() == "if" ? tok->next() : tok; + const Token * condEndToken = tok->str() == "if" ? condStartToken->link() : Token::findsimplematch(condStartToken, ";"); + // Does condition modify tracked variables? + if (const Token *op = Token::findmatch(tok, "++|--", condEndToken)) { + bool bailout = false; + while (op) { + if (vars.find(op->astOperand1()->varId()) != vars.end()) { + bailout = true; + break; + } + if (nonlocal && op->astOperand1()->varId() == 0) { + bailout = true; + break; + } + op = Token::findmatch(op->next(), "++|--", condEndToken); } - if (nonlocal && op->astOperand1()->varId() == 0) { - bailout = true; + if (bailout) break; - } - op = Token::findmatch(op->next(), "++|--", tok->linkAt(1)); } - if (bailout) - break; - } - // Condition.. - const Token *cond2 = tok->next()->astOperand2(); + // Condition.. + const Token *cond2 = tok->str() == "if" ? condStartToken->astOperand2() : condStartToken->astOperand1(); + // Check if returning boolean values + if (tok->str() == "return") { + const Variable * condVar = nullptr; + if (Token::Match(cond2, "%var% ;")) + condVar = cond2->variable(); + else if(Token::Match(cond2, ". %var% ;")) + condVar = cond2->next()->variable(); + if (condVar && (condVar->isClass() || condVar->isPointer())) + break; + } - ErrorPath errorPath; + ErrorPath errorPath; - if (type == MULTICONDITIONTYPE::INNER) { - std::stack tokens1; - tokens1.push(cond1); - while (!tokens1.empty()) { - const Token *firstCondition = tokens1.top(); - tokens1.pop(); - if (!firstCondition) - continue; - if (firstCondition->str() == "&&") { - tokens1.push(firstCondition->astOperand1()); - tokens1.push(firstCondition->astOperand2()); - } else if (!firstCondition->hasKnownValue()) { - if (isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true, true, &errorPath)) { + if (type == MULTICONDITIONTYPE::INNER) { + std::stack tokens1; + tokens1.push(cond1); + while (!tokens1.empty()) { + const Token *firstCondition = tokens1.top(); + tokens1.pop(); + if (!firstCondition) + continue; + if (firstCondition->str() == "&&") { + tokens1.push(firstCondition->astOperand1()); + tokens1.push(firstCondition->astOperand2()); + } else if (!firstCondition->hasKnownValue()) { + if (isOppositeCond(false, mTokenizer->isCPP(), firstCondition, cond2, mSettings->library, true, true, &errorPath)) { + if (!isAliased(vars)) + oppositeInnerConditionError(firstCondition, cond2, errorPath); + } else if (isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true, true, &errorPath)) { + identicalInnerConditionError(firstCondition, cond2, errorPath); + } + } + } + } else { + std::stack tokens2; + tokens2.push(cond2); + while (!tokens2.empty()) { + const Token *secondCondition = tokens2.top(); + tokens2.pop(); + if (!secondCondition) + continue; + if (secondCondition->str() == "||" || secondCondition->str() == "&&") { + tokens2.push(secondCondition->astOperand1()); + tokens2.push(secondCondition->astOperand2()); + } else if ((!cond1->hasKnownValue() || !secondCondition->hasKnownValue()) && + isSameExpression(mTokenizer->isCPP(), true, cond1, secondCondition, mSettings->library, true, true, &errorPath)) { if (!isAliased(vars)) - oppositeInnerConditionError(firstCondition, cond2, errorPath); - } else if (isSameExpression(mTokenizer->isCPP(), true, firstCondition, cond2, mSettings->library, true, true, &errorPath)) { - identicalInnerConditionError(firstCondition, cond2, errorPath); + identicalConditionAfterEarlyExitError(cond1, secondCondition, errorPath); } } } - } else { - std::stack tokens2; - tokens2.push(cond2); - while (!tokens2.empty()) { - const Token *secondCondition = tokens2.top(); - tokens2.pop(); - if (!secondCondition) - continue; - if (secondCondition->str() == "||" || secondCondition->str() == "&&") { - tokens2.push(secondCondition->astOperand1()); - tokens2.push(secondCondition->astOperand2()); - } else if ((!cond1->hasKnownValue() || !secondCondition->hasKnownValue()) && - isSameExpression(mTokenizer->isCPP(), true, cond1, secondCondition, mSettings->library, true, true, &errorPath)) { - if (!isAliased(vars)) - identicalConditionAfterEarlyExitError(cond1, secondCondition, errorPath); + } + if (Token::Match(tok, "%type% (") && nonlocal && isNonConstFunctionCall(tok, mSettings->library)) // non const function call -> bailout if there are nonlocal variables + break; + if (Token::Match(tok, "case|break|continue|return|throw") && tok->scope() == endToken->scope()) + break; + if (Token::Match(tok, "[;{}] %name% :")) + break; + // bailout if loop is seen. + // TODO: handle loops better. + if (Token::Match(tok, "for|while|do")) { + const Token *tok1 = tok->next(); + const Token *tok2; + if (Token::simpleMatch(tok, "do {")) { + if (!Token::simpleMatch(tok->linkAt(1), "} while (")) + break; + tok2 = tok->linkAt(1)->linkAt(2); + } else if (Token::Match(tok, "if|while (")) { + tok2 = tok->linkAt(1); + if (Token::simpleMatch(tok2, ") {")) + tok2 = tok2->linkAt(1); + if (!tok2) + break; + } else { + // Incomplete code + break; + } + bool changed = false; + for (unsigned int varid : vars) { + if (isVariableChanged(tok1, tok2, varid, nonlocal, mSettings, mTokenizer->isCPP())) { + changed = true; + break; } } + if (changed) + break; } - } - if (Token::Match(tok, "%type% (") && nonlocal && isNonConstFunctionCall(tok, mSettings->library)) // non const function call -> bailout if there are nonlocal variables - break; - if (Token::Match(tok, "case|break|continue|return|throw") && tok->scope() == endToken->scope()) - break; - if (Token::Match(tok, "[;{}] %name% :")) - break; - // bailout if loop is seen. - // TODO: handle loops better. - if (Token::Match(tok, "for|while|do")) { - const Token *tok1 = tok->next(); - const Token *tok2; - if (Token::simpleMatch(tok, "do {")) { - if (!Token::simpleMatch(tok->linkAt(1), "} while (")) - break; - tok2 = tok->linkAt(1)->linkAt(2); - } else if (Token::Match(tok, "if|while (")) { - tok2 = tok->linkAt(1); - if (Token::simpleMatch(tok2, ") {")) - tok2 = tok2->linkAt(1); - if (!tok2) - break; - } else { - // Incomplete code - break; - } - bool changed = false; - for (unsigned int varid : vars) { - if (isVariableChanged(tok1, tok2, varid, nonlocal, mSettings, mTokenizer->isCPP())) { - changed = true; + if ((tok->varId() && vars.find(tok->varId()) != vars.end()) || + (!tok->varId() && nonlocal)) { + if (Token::Match(tok, "%name% %assign%|++|--")) break; + if (Token::Match(tok->astParent(), "*|.|[")) { + const Token *parent = tok; + while (Token::Match(parent->astParent(), ".|[") || (parent->astParent() && parent->astParent()->isUnaryOp("*"))) + parent = parent->astParent(); + if (Token::Match(parent->astParent(), "%assign%|++|--")) + break; } - } - if (changed) - break; - } - if ((tok->varId() && vars.find(tok->varId()) != vars.end()) || - (!tok->varId() && nonlocal)) { - if (Token::Match(tok, "%name% %assign%|++|--")) - break; - if (Token::Match(tok->astParent(), "*|.|[")) { - const Token *parent = tok; - while (Token::Match(parent->astParent(), ".|[") || (parent->astParent() && parent->astParent()->isUnaryOp("*"))) - parent = parent->astParent(); - if (Token::Match(parent->astParent(), "%assign%|++|--")) + if (mTokenizer->isCPP() && Token::Match(tok, "%name% <<") && (!tok->valueType() || !tok->valueType()->isIntegral())) + break; + if (isLikelyStreamRead(mTokenizer->isCPP(), tok->next()) || isLikelyStreamRead(mTokenizer->isCPP(), tok->previous())) + break; + if (Token::Match(tok, "%name% [")) { + const Token *tok2 = tok->linkAt(1); + while (Token::simpleMatch(tok2, "] [")) + tok2 = tok2->linkAt(1); + if (Token::Match(tok2, "] %assign%|++|--")) + break; + } + if (Token::Match(tok->previous(), "++|--|& %name%")) + break; + if (tok->variable() && + !tok->variable()->isConst() && + Token::Match(tok, "%name% . %name% (")) { + const Function* function = tok->tokAt(2)->function(); + if (!function || !function->isConst()) + break; + } + if (Token::Match(tok->previous(), "[(,] %name% [,)]") && isParameterChanged(tok)) break; } - if (mTokenizer->isCPP() && Token::Match(tok, "%name% <<") && (!tok->valueType() || !tok->valueType()->isIntegral())) - break; - if (isLikelyStreamRead(mTokenizer->isCPP(), tok->next()) || isLikelyStreamRead(mTokenizer->isCPP(), tok->previous())) - break; - if (Token::Match(tok, "%name% [")) { - const Token *tok2 = tok->linkAt(1); - while (Token::simpleMatch(tok2, "] [")) - tok2 = tok2->linkAt(1); - if (Token::Match(tok2, "] %assign%|++|--")) - break; - } - if (Token::Match(tok->previous(), "++|--|& %name%")) - break; - if (tok->variable() && - !tok->variable()->isConst() && - Token::Match(tok, "%name% . %name% (")) { - const Function* function = tok->tokAt(2)->function(); - if (!function || !function->isConst()) - break; - } - if (Token::Match(tok->previous(), "[(,] %name% [,)]") && isParameterChanged(tok)) - break; } } } } +static std::string innerSmtString(const Token * tok) +{ + if(!tok) + return "if"; + if(!tok->astTop()) + return "if"; + const Token * top = tok->astTop(); + if(top->str() == "(" && top->astOperand1()) + return top->astOperand1()->str(); + return top->str(); +} + void CheckCondition::oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath) { const std::string s1(tok1 ? tok1->expressionString() : "x"); const std::string s2(tok2 ? tok2->expressionString() : "!x"); + const std::string innerSmt = innerSmtString(tok2); errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1)); errorPath.emplace_back(ErrorPathItem(tok2, "opposite inner condition: " + s2)); - const std::string msg("Opposite inner 'if' condition leads to a dead code block.\n" - "Opposite inner 'if' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); + const std::string msg("Opposite inner '" + innerSmt + "' condition leads to a dead code block.\n" + "Opposite inner '" + innerSmt + "' condition leads to a dead code block (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); reportError(errorPath, Severity::warning, "oppositeInnerCondition", msg, CWE398, false); } @@ -699,11 +729,12 @@ void CheckCondition::identicalInnerConditionError(const Token *tok1, const Token { const std::string s1(tok1 ? tok1->expressionString() : "x"); const std::string s2(tok2 ? tok2->expressionString() : "x"); + const std::string innerSmt = innerSmtString(tok2); errorPath.emplace_back(ErrorPathItem(tok1, "outer condition: " + s1)); errorPath.emplace_back(ErrorPathItem(tok2, "identical inner condition: " + s2)); - const std::string msg("Identical inner 'if' condition is always true.\n" - "Identical inner 'if' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); + const std::string msg("Identical inner '" + innerSmt + "' condition is always true.\n" + "Identical inner '" + innerSmt + "' condition is always true (outer condition is '" + s1 + "' and inner condition is '" + s2 + "')."); reportError(errorPath, Severity::warning, "identicalInnerCondition", msg, CWE398, false); } @@ -1222,7 +1253,7 @@ void CheckCondition::alwaysTrueFalse() continue; if (Token::Match(tok, "! %num%|%bool%|%char%")) continue; - if (Token::Match(tok, "%oror%|&&")) + if (Token::Match(tok, "%oror%|&&|:")) continue; if (Token::Match(tok, "%comp%") && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true)) continue; @@ -1232,8 +1263,20 @@ 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"); - if (!(constIfWhileExpression || constValExpr || compExpr)) + if (!(constIfWhileExpression || constValExpr || compExpr || returnStatement)) + continue; + + if(returnStatement && (tok->isEnumerator() || Token::Match(tok, "nullptr|NULL"))) + 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. diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 6bfc72bd6..5f4a0065a 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1441,6 +1441,13 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); + check("bool foo(int a, int b) {\n" + " if(a==b)\n" + " return a!=b;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Opposite inner 'return' condition leads to a dead code block.\n", errout.str()); + check("void foo(int a, int b) {\n" " if(a==b)\n" " if(b!=a)\n" @@ -2049,6 +2056,25 @@ private: "}\n"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical inner 'if' condition is always true.\n", errout.str()); + check("bool f(bool a) {\n" + " if(a) { return a; }\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (warning) Identical inner 'return' condition is always true.\n", errout.str()); + + check("int* f(int* a, int * b) {\n" + " if(a) { return a; }\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A { int * x; };\n" + "int* f(A a, int * b) {\n" + " if(a.x) { return a.x; }\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void f() {\n" " uint32_t value;\n" " get_value(&value);\n" @@ -2069,6 +2095,12 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical condition 'x>100', second condition is always false\n", errout.str()); + check("bool f(int x) {\n" + " if (x > 100) { return false; }\n" + " return x > 100;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Identical condition 'x>100', second condition is always false\n", errout.str()); + check("void f(int x) {\n" " if (x > 100) { return; }\n" " if (x > 100 || y > 100) {}\n" @@ -2444,6 +2476,12 @@ 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" @@ -2546,6 +2584,42 @@ private: " {}\n" "}\n"); ASSERT_EQUALS("[test.cpp:4]: (style) Condition '!b' is always true\n", errout.str()); + + check("bool f() { return nullptr; }"); + ASSERT_EQUALS("", errout.str()); + + check("enum E { A };\n" + "bool f() { return A; }\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f() { \n" + " const int x = 0;\n" + " return x; \n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f() { \n" + " int x = 0;\n" + " return x; \n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool& g();\n" + "bool f() {\n" + " bool & b = g();\n" + " b = false;\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct A {\n" + " bool b;\n" + " bool f() {\n" + " b = false;\n" + " return b;\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); } void multiConditionAlwaysTrue() {