From 16c62281d0cd91836ab9737bdd0a0640b7ea1b35 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 18 Oct 2018 04:56:23 -0500 Subject: [PATCH] Use followVar in checking duplicateBranch (#1423) * Use isSameExpression for duplicate branches * Add errorPath * Add another test --- lib/astutils.cpp | 14 ++++++++++++++ lib/astutils.h | 2 ++ lib/checkother.cpp | 38 +++++++++++++++++++++++++++++++++----- lib/checkother.h | 4 ++-- lib/valueflow.cpp | 14 -------------- test/testother.cpp | 24 ++++++++++++++++++++++++ 6 files changed, 75 insertions(+), 21 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 071d5159c..0ffb97645 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -133,6 +133,20 @@ const Token * astIsVariableComparison(const Token *tok, const std::string &comp, return ret; } +const Token * nextAfterAstRightmostLeaf(const Token * tok) +{ + const Token * rightmostLeaf = tok; + if (!rightmostLeaf || !rightmostLeaf->astOperand1()) + return nullptr; + do { + if (rightmostLeaf->astOperand2()) + rightmostLeaf = rightmostLeaf->astOperand2(); + else + rightmostLeaf = rightmostLeaf->astOperand1(); + } while (rightmostLeaf->astOperand1()); + return rightmostLeaf->next(); +} + static const Token * getVariableInitExpression(const Variable * var) { if (!var || !var->declEndToken()) diff --git a/lib/astutils.h b/lib/astutils.h index 5498e11cd..610e8adea 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -56,6 +56,8 @@ std::string astCanonicalType(const Token *expr); /** Is given syntax tree a variable comparison against value */ const Token * astIsVariableComparison(const Token *tok, const std::string &comp, const std::string &rhs, const Token **vartok=nullptr); +const Token * nextAfterAstRightmostLeaf(const Token * tok); + bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr); bool isEqualKnownValue(const Token * const tok1, const Token * const tok2); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 4707126b5..21c54f201 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1769,6 +1769,19 @@ void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& va "Instance of '$symbol' object is destroyed immediately.", CWE563, false); } +static const Token * getSingleExpressionInBlock(const Token * tok) +{ + if(!tok) + return nullptr; + const Token * top = tok->astTop(); + if(!top) + return nullptr; + const Token * nextExpression = nextAfterAstRightmostLeaf(top); + if (!Token::simpleMatch(nextExpression, "; }")) + return nullptr; + return top; +} + //----------------------------------------------------------------------------- // check for duplicate code in if and else branches // if (a) { b = true; } else { b = true; } @@ -1815,18 +1828,33 @@ void CheckOther::checkDuplicateBranch() // save else branch code const std::string branch2 = scope.bodyEnd->tokAt(3)->stringifyList(scope.bodyEnd->linkAt(2)); + ErrorPath errorPath; // check for duplicates - if (branch1 == branch2) - duplicateBranchError(scope.classDef, scope.bodyEnd->next()); + if (branch1 == branch2) { + duplicateBranchError(scope.classDef, scope.bodyEnd->next(), errorPath); + continue; + } + + // check for duplicates using isSameExpression + const Token * branchTop1 = getSingleExpressionInBlock(scope.bodyStart->next()); + const Token * branchTop2 = getSingleExpressionInBlock(scope.bodyEnd->tokAt(3)); + if (!branchTop1 || !branchTop2) + continue; + if(branchTop1->str() != branchTop2->str()) + continue; + if(isSameExpression(mTokenizer->isCPP(), false, branchTop1->astOperand1(), branchTop2->astOperand1(), mSettings->library, true, true, &errorPath) && + isSameExpression(mTokenizer->isCPP(), false, branchTop1->astOperand2(), branchTop2->astOperand2(), mSettings->library, true, true, &errorPath)) + duplicateBranchError(scope.classDef, scope.bodyEnd->next(), errorPath); } } } -void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2) +void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2, ErrorPath errors) { - const std::list toks = { tok2, tok1 }; + errors.emplace_back(tok2, ""); + errors.emplace_back(tok1, ""); - reportError(toks, Severity::style, "duplicateBranch", "Found duplicate branches for 'if' and 'else'.\n" + reportError(errors, Severity::style, "duplicateBranch", "Found duplicate branches for 'if' and 'else'.\n" "Finding the same code in an 'if' and related 'else' branch is suspicious and " "might indicate a cut and paste or logic error. Please examine this code " "carefully to determine if it is correct.", CWE398, true); diff --git a/lib/checkother.h b/lib/checkother.h index 72a08f8bc..f75ac7d96 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -241,7 +241,7 @@ private: void suspiciousEqualityComparisonError(const Token* tok); void selfAssignmentError(const Token *tok, const std::string &varname); void misusedScopeObjectError(const Token *tok, const std::string &varname); - void duplicateBranchError(const Token *tok1, const Token *tok2); + void duplicateBranchError(const Token *tok1, const Token *tok2, ErrorPath errors); void duplicateAssignExpressionError(const Token *tok1, const Token *tok2, bool inconclusive); void oppositeExpressionError(const Token *opTok, ErrorPath errors); void duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors); @@ -306,7 +306,7 @@ private: c.selfAssignmentError(nullptr, "varname"); c.clarifyCalculationError(nullptr, "+"); c.clarifyStatementError(nullptr); - c.duplicateBranchError(nullptr, nullptr); + c.duplicateBranchError(nullptr, nullptr, errorPath); c.duplicateAssignExpressionError(nullptr, nullptr, true); c.oppositeExpressionError(nullptr, errorPath); c.duplicateExpressionError(nullptr, nullptr, nullptr, errorPath); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d850fbfe3..11cb19d3a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2245,20 +2245,6 @@ static bool isOpenParenthesisMemberFunctionCallOfVarId(const Token * openParenth varTok->next()->originalName() == emptyString; } -static const Token * nextAfterAstRightmostLeaf(Token const * tok) -{ - const Token * rightmostLeaf = tok; - if (!rightmostLeaf || !rightmostLeaf->astOperand1()) - return nullptr; - do { - if (rightmostLeaf->astOperand2()) - rightmostLeaf = rightmostLeaf->astOperand2(); - else - rightmostLeaf = rightmostLeaf->astOperand1(); - } while (rightmostLeaf->astOperand1()); - return rightmostLeaf->next(); -} - static const Token * findOpenParentesisOfMove(const Token * moveVarTok) { const Token * tok = moveVarTok; diff --git a/test/testother.cpp b/test/testother.cpp index 2755014fe..e7fbd8570 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -124,6 +124,7 @@ private: TEST_CASE(duplicateBranch); TEST_CASE(duplicateBranch1); // tests extracted by http://www.viva64.com/en/b/0149/ ( Comparison between PVS-Studio and cppcheck ): Errors detected in Quake 3: Arena by PVS-Studio: Fragment 2 TEST_CASE(duplicateBranch2); // empty macro + TEST_CASE(duplicateBranch3); TEST_CASE(duplicateExpression1); TEST_CASE(duplicateExpression2); // ticket #2730 TEST_CASE(duplicateExpression3); // ticket #3317 @@ -3491,6 +3492,29 @@ private: ASSERT_EQUALS("", errout.str()); } + void duplicateBranch3() { + check("void f(bool b, int i) {\n" + " int j = i;\n" + " if (b) {\n" + " x = i;\n" + " } else {\n" + " x = j;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:5] -> [test.cpp:3]: (style, inconclusive) Found duplicate branches for 'if' and 'else'.\n", errout.str()); + + check("void f(bool b, int i) {\n" + " int j = i;\n" + " i++;\n" + " if (b) {\n" + " x = i;\n" + " } else {\n" + " x = j;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void duplicateExpression1() { check("void foo(int a) {\n" " if (a == a) { }\n"