From 4e147a4c596f6c84b2a5733532b40b344a51ebd0 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 9 Jan 2019 20:41:01 +0100 Subject: [PATCH] Add a check for duplicate if statements This will warn for this: ```cpp int f(int val) { int i = 0; if( val & 0xff) i = 1; if( val & 0xff) i = 1; return i; } ``` --- lib/checkcondition.cpp | 57 +++++++++++++++++++++ lib/checkcondition.h | 5 ++ test/testcondition.cpp | 112 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 166 insertions(+), 8 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index e17d7d5c9..8770c721c 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -415,6 +415,63 @@ bool CheckCondition::isOverlappingCond(const Token * const cond1, const Token * return false; } +void CheckCondition::duplicateCondition() +{ + if (!mSettings->isEnabled(Settings::STYLE)) + return; + + const SymbolDatabase *const symbolDatabase = mTokenizer->getSymbolDatabase(); + + for (const Scope &scope : symbolDatabase->scopeList) { + if (scope.type != Scope::eIf) + continue; + + const Token *cond1 = scope.classDef->next()->astOperand2(); + if (!cond1) + continue; + if (cond1->hasKnownIntValue()) + continue; + + const Token *tok2 = scope.classDef->next(); + if (!tok2) + continue; + tok2 = tok2->link(); + if (!Token::simpleMatch(tok2, ") {")) + continue; + tok2 = tok2->linkAt(1); + if (!Token::simpleMatch(tok2, "} if (")) + continue; + const Token *cond2 = tok2->tokAt(2)->astOperand2(); + if (!cond2) + continue; + + bool modified = false; + visitAstNodes(cond1, [&](const Token *tok3) { + if (tok3->varId() > 0 && + isVariableChanged(scope.classDef->next(), cond2, tok3->varId(), false, mSettings, mTokenizer->isCPP())) { + modified = true; + return ChildrenToVisit::done; + } + return ChildrenToVisit::op1_and_op2; + }); + ErrorPath errorPath; + if (!modified && + isSameExpression(mTokenizer->isCPP(), true, cond1, cond2, mSettings->library, true, true, &errorPath)) + duplicateConditionError(cond1, cond2, errorPath); + } +} + +void CheckCondition::duplicateConditionError(const Token *tok1, const Token *tok2, ErrorPath errorPath) +{ + if (diag(tok1) & diag(tok2)) + return; + errorPath.emplace_back(tok1, "First condition"); + errorPath.emplace_back(tok2, "Second condition"); + + std::string msg = "The if condition is the same as the previous if condition"; + + reportError(errorPath, Severity::style, "duplicateCondition", msg, CWE398, false); +} void CheckCondition::multiCondition() { diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 15aa05bb8..f4dc0616c 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -59,6 +59,7 @@ public: checkCondition.checkIncorrectLogicOperator(); checkCondition.checkInvalidTestForOverflow(); checkCondition.alwaysTrueFalse(); + checkCondition.duplicateCondition(); checkCondition.checkPointerAdditionResultNotNull(); } @@ -88,6 +89,8 @@ public: /** mismatching lhs and rhs in comparison */ void comparison(); + void duplicateCondition(); + /** match 'if' and 'else if' conditions */ void multiCondition(); @@ -132,6 +135,7 @@ private: const std::string &op, MathLib::bigint value2, bool result); + void duplicateConditionError(const Token *tok1, const Token *tok2, ErrorPath errorPath); void multiConditionError(const Token *tok, unsigned int line1); void oppositeInnerConditionError(const Token *tok1, const Token* tok2, ErrorPath errorPath); @@ -160,6 +164,7 @@ private: c.assignIfError(nullptr, nullptr, emptyString, false); c.badBitmaskCheckError(nullptr); c.comparisonError(nullptr, "&", 6, "==", 1, false); + c.duplicateConditionError(nullptr, nullptr, errorPath); c.multiConditionError(nullptr,1); c.mismatchingBitAndError(nullptr, 0xf0, nullptr, 1); c.oppositeInnerConditionError(nullptr, nullptr, errorPath); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 7456701ce..9a40e0a21 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -107,6 +107,7 @@ private: TEST_CASE(alwaysTrue); TEST_CASE(multiConditionAlwaysTrue); + TEST_CASE(duplicateCondition); TEST_CASE(checkInvalidTestForOverflow); TEST_CASE(checkConditionIsAlwaysTrueOrFalseInsideIfWhile); @@ -1792,53 +1793,83 @@ private: " if (x<4) {\n" " if (x==5) {}\n" // <- Warning " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + check("void f(int x) {\n" "\n" " if (x<4) {\n" " if (x!=5) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<4) {\n" " if (x>5) {}\n" // <- Warning " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + check("void f(int x) {\n" "\n" " if (x<4) {\n" " if (x>=5) {}\n" // <- Warning " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + check("void f(int x) {\n" "\n" " if (x<4) {\n" " if (x<5) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<4) {\n" " if (x<=5) {}\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - "[test.cpp:11] -> [test.cpp:12]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - "[test.cpp:15] -> [test.cpp:16]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - , errout.str()); + ASSERT_EQUALS("", errout.str()); check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x==4) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x!=4) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x>4) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x>=4) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x<4) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x<5) {\n" " if (x<=4) {}\n" @@ -1852,18 +1883,30 @@ private: " if (x>4) {\n" " if (x==5) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>4) {\n" " if (x>5) {}\n" " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>4) {\n" " if (x>=5) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>4) {\n" " if (x<5) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>4) {\n" " if (x<=5) {}\n" @@ -1876,27 +1919,39 @@ private: " if (x>5) {\n" " if (x==4) {}\n" // <- Warning " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + check("void f(int x) {\n" "\n" " if (x>5) {\n" " if (x>4) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>5) {\n" " if (x>=4) {}\n" // <- TODO " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int x) {\n" "\n" " if (x>5) {\n" " if (x<4) {}\n" // <- Warning " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); + check("void f(int x) {\n" "\n" " if (x>5) {\n" " if (x<=4) {}\n" // <- Warning " }\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - "[test.cpp:15] -> [test.cpp:16]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - "[test.cpp:19] -> [test.cpp:20]: (warning) Opposite inner 'if' condition leads to a dead code block.\n" - , errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", + errout.str()); check("void f(int x) {\n" " if (x < 4) {\n" @@ -2755,6 +2810,47 @@ private: "[test.cpp:5]: (style) Condition 'foo' is always false\n", errout.str()); } + void duplicateCondition() { + check("void f(bool x) {\n" + " if(x) {}\n" + " if(x) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The if condition is the same as the previous if condition\n", + errout.str()); + + check("void f(int x) {\n" + " if(x == 1) {}\n" + " if(x == 1) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The if condition is the same as the previous if condition\n", + errout.str()); + + check("void f(int x) {\n" + " if(x == 1) {}\n" + " if(x == 2) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if(x == 1) {}\n" + " if(x != 1) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(bool x) {\n" + " if(x) {}\n" + " g();\n" + " if(x) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int x) {\n" + " if(x == 1) { x++; }\n" + " if(x == 1) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void checkInvalidTestForOverflow() { check("void f(char *p, unsigned int x) {\n" " assert((p + x) < p);\n"