From cf1ad5087a484980e5a0154d3f5d64f038aca15f Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 15 Feb 2019 13:31:40 +0100 Subject: [PATCH] Extend constStatement checker This reworks constStatement to find more issues. It catches issue [8827](https://trac.cppcheck.net/ticket/8827): ```cpp extern void foo(int,const char*,int); void f(int value) { foo(42,"test",42),(value&42); } ``` It also catches from issue [8451](https://trac.cppcheck.net/ticket/8451): ```cpp void f1(int x) { 1; (1); (char)1; ((char)1); !x; (!x); ~x; } ``` And also: ```cpp void f(int x) { x; } ``` The other examples are not caught due to incomplete AST. --- lib/checkother.cpp | 179 +++++++++++++++---------------- lib/checkother.h | 12 +-- test/testincompletestatement.cpp | 40 ++++++- test/testother.cpp | 18 ++-- 4 files changed, 137 insertions(+), 112 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 499443271..85e35c93c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -734,57 +734,6 @@ void CheckOther::suspiciousCaseInSwitchError(const Token* tok, const std::string "Using an operator like '" + operatorString + "' in a case label is suspicious. Did you intend to use a bitwise operator, multiple case labels or if/else instead?", CWE398, true); } -//--------------------------------------------------------------------------- -// if (x == 1) -// x == 0; // <- suspicious equality comparison. -//--------------------------------------------------------------------------- -void CheckOther::checkSuspiciousEqualityComparison() -{ - if (!mSettings->isEnabled(Settings::WARNING) || !mSettings->inconclusive) - return; - - const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase(); - for (const Scope * scope : symbolDatabase->functionScopes) { - for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { - if (Token::simpleMatch(tok, "for (")) { - const Token* const openParen = tok->next(); - const Token* const closeParen = tok->linkAt(1); - - // Search for any suspicious equality comparison in the initialization - // or increment-decrement parts of the for() loop. - // For example: - // for (i == 2; i < 10; i++) - // or - // for (i = 0; i < 10; i == a) - if (Token::Match(openParen->next(), "%name% ==")) - suspiciousEqualityComparisonError(openParen->tokAt(2)); - if (closeParen->strAt(-2) == "==") - suspiciousEqualityComparisonError(closeParen->tokAt(-2)); - - // Skip over for() loop conditions because "for (;running==1;)" - // is a bit strange, but not necessarily incorrect. - tok = closeParen; - } else if (Token::Match(tok, "[;{}] *| %name% == %any% ;")) { - - // Exclude compound statements surrounded by parentheses, such as - // printf("%i\n", ({x==0;})); - // because they may appear as an expression in GNU C/C++. - // See http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html - const Token* afterStatement = tok->strAt(1) == "*" ? tok->tokAt(6) : tok->tokAt(5); - if (!Token::simpleMatch(afterStatement, "} )")) - suspiciousEqualityComparisonError(tok->next()); - } - } - } -} - -void CheckOther::suspiciousEqualityComparisonError(const Token* tok) -{ - reportError(tok, Severity::warning, "suspiciousEqualityComparison", - "Found suspicious equality comparison. Did you intend to assign a value instead?", CWE482, true); -} - - //--------------------------------------------------------------------------- // Find consecutive return, break, continue, goto or throw statements. e.g.: // break; break; @@ -1389,64 +1338,106 @@ void CheckOther::charBitOpError(const Token *tok) //--------------------------------------------------------------------------- // Incomplete statement.. //--------------------------------------------------------------------------- + +static bool isConstStatement(const Token *tok) +{ + if (!tok) + return false; + if (tok->isExpandedMacro()) + return false; + if (Token::Match(tok, "%bool%|%num%|%str%|%char%|nullptr|NULL")) + return true; + if (Token::Match(tok, "%var%")) + return true; + if (Token::Match(tok, "*|&|&&") && + (Token::Match(tok->previous(), "::|.") || Token::Match(tok->astOperand1(), "%type%") || + Token::Match(tok->astOperand2(), "%type%"))) + return false; + if (Token::Match(tok, "<<|>>") && !astIsIntegral(tok, false)) + return false; + if (Token::Match(tok, "!|~|%cop%") && (tok->astOperand1() || tok->astOperand2())) + return true; + if (Token::simpleMatch(tok->previous(), "sizeof (")) + return true; + if (isCPPCast(tok)) + return isConstStatement(tok->astOperand2()); + if (Token::Match(tok, "( %type%")) + return isConstStatement(tok->astOperand1()); + if (Token::simpleMatch(tok, ",")) + return isConstStatement(tok->astOperand2()); + return false; +} + +static bool isVoidStmt(const Token *tok) +{ + if (Token::simpleMatch(tok, "( void")) + return true; + const Token *tok2 = tok; + while (tok2->astOperand1()) + tok2 = tok2->astOperand1(); + if (Token::simpleMatch(tok2->previous(), ")") && Token::simpleMatch(tok2->previous()->link(), "( void")) + return true; + if (Token::simpleMatch(tok2, "( void")) + return true; + return Token::Match(tok2->previous(), "delete|throw|return"); +} + +static bool isConstTop(const Token *tok) +{ + if (!tok) + return false; + if (tok == tok->astTop()) + return true; + if (Token::simpleMatch(tok->astParent(), ";") && tok->astTop() && + Token::Match(tok->astTop()->previous(), "for|if (") && Token::simpleMatch(tok->astTop()->astOperand2(), ";")) { + if (Token::simpleMatch(tok->astParent()->astParent(), ";")) + return tok->astParent()->astOperand2() == tok; + else + return tok->astParent()->astOperand1() == tok; + } + return false; +} + void CheckOther::checkIncompleteStatement() { if (!mSettings->isEnabled(Settings::WARNING)) return; for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "(|[")) - tok = tok->link(); - - else if (tok->str() == "{" && tok->astParent()) - tok = tok->link(); - - // C++11 struct/array/etc initialization in initializer list - else if (Token::Match(tok->previous(), "%var%|] {")) - tok = tok->link(); - - if (!Token::Match(tok, "[;{}] %str%|%num%")) + const Scope *scope = tok->scope(); + if (scope && !scope->isExecutable()) continue; - - // No warning if numeric constant is followed by a "." or "," - if (Token::Match(tok->next(), "%num% [,.]")) + if (!isConstTop(tok)) continue; - - // No warning for [;{}] (void *) 0 ; - if (Token::Match(tok, "[;{}] 0 ;") && (tok->next()->isCast() || tok->next()->isExpandedMacro())) + const Token *rtok = nextAfterAstRightmostLeaf(tok); + if (!Token::simpleMatch(tok->astParent(), ";") && !Token::simpleMatch(rtok, ";") && + !Token::Match(tok->previous(), ";|}|{ %any% ;")) continue; - - // bailout if there is a "? :" in this statement - bool bailout = false; - for (const Token *tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) { - if (tok2->str() == "?") { - bailout = true; - break; - } else if (tok2->str() == ";") - break; - } - if (bailout) + // Skipe statement expressions + if (Token::simpleMatch(rtok, "; } )")) continue; - - // no warning if this is the last statement in a ({}) - for (const Token *tok2 = tok->next(); tok2; tok2 = tok2->next()) { - if (tok2->str() == "(") - tok2 = tok2->link(); - else if (Token::Match(tok2, "[;{}]")) { - bailout = Token::simpleMatch(tok2, "; } )"); - break; - } - } - if (bailout) + if (!isConstStatement(tok)) continue; - - constStatementError(tok->next(), tok->next()->isNumber() ? "numeric" : "string"); + if (isVoidStmt(tok)) + continue; + bool inconclusive = Token::Match(tok, "%cop%"); + if (mSettings->inconclusive || !inconclusive) + constStatementError(tok, tok->isNumber() ? "numeric" : "string", inconclusive); } } -void CheckOther::constStatementError(const Token *tok, const std::string &type) +void CheckOther::constStatementError(const Token *tok, const std::string &type, bool inconclusive) { - reportError(tok, Severity::warning, "constStatement", "Redundant code: Found a statement that begins with " + type + " constant.", CWE398, false); + std::string msg; + if (Token::simpleMatch(tok, "==")) + msg = "Found suspicious equality comparison. Did you intend to assign a value instead?"; + else if (Token::Match(tok, ",|!|~|%cop%")) + msg = "Found suspicious operator '" + tok->str() + "'"; + else if (Token::Match(tok, "%var%")) + msg = "Unused variable value '" + tok->str() + "'"; + else + msg = "Redundant code: Found a statement that begins with " + type + " constant."; + reportError(tok, Severity::warning, "constStatement", msg, CWE398, inconclusive); } //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 58b88fae3..c6d52491d 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -83,6 +83,7 @@ public: checkOther.checkFuncArgNamesDifferent(); checkOther.checkShadowVariables(); checkOther.checkConstArgument(); + checkOther.checkIncompleteStatement(); } /** @brief Run checks against the simplified token list */ @@ -93,7 +94,6 @@ public: checkOther.clarifyCalculation(); checkOther.clarifyStatement(); checkOther.checkPassByReference(); - checkOther.checkIncompleteStatement(); checkOther.checkCastIntToCharAndBack(); checkOther.checkMisusedScopedObject(); @@ -101,7 +101,6 @@ public: checkOther.checkInvalidFree(); checkOther.checkRedundantCopy(); - checkOther.checkSuspiciousEqualityComparison(); checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); checkOther.checkAccessOfMovedVariable(); } @@ -149,9 +148,6 @@ public: /** @brief %Check for code like 'case A||B:'*/ void checkSuspiciousCaseInSwitch(); - /** @brief %Check for code like 'case A||B:'*/ - void checkSuspiciousEqualityComparison(); - /** @brief %Check for objects that are destroyed immediately */ void checkMisusedScopedObject(); @@ -228,7 +224,7 @@ private: void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive); void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive); - void constStatementError(const Token *tok, const std::string &type); + void constStatementError(const Token *tok, const std::string &type, bool inconclusive); void signedCharArrayIndexError(const Token *tok); void unknownSignCharArrayIndexError(const Token *tok); void charBitOpError(const Token *tok); @@ -241,7 +237,6 @@ private: void redundantCopyInSwitchError(const Token *tok1, const Token* tok2, const std::string &var); void redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname); void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString); - 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, ErrorPath errors); @@ -298,7 +293,7 @@ private: c.checkCastIntToCharAndBackError(nullptr, "func_name"); c.cstyleCastError(nullptr); c.passedByValueError(nullptr, "parametername", false); - c.constStatementError(nullptr, "type"); + c.constStatementError(nullptr, "type", false); c.signedCharArrayIndexError(nullptr); c.unknownSignCharArrayIndexError(nullptr); c.charBitOpError(nullptr); @@ -306,7 +301,6 @@ private: c.redundantAssignmentInSwitchError(nullptr, nullptr, "var"); c.redundantCopyInSwitchError(nullptr, nullptr, "var"); c.suspiciousCaseInSwitchError(nullptr, "||"); - c.suspiciousEqualityComparisonError(nullptr); c.selfAssignmentError(nullptr, "varname"); c.clarifyCalculationError(nullptr, "+"); c.clarifyStatementError(nullptr); diff --git a/test/testincompletestatement.cpp b/test/testincompletestatement.cpp index cec9fc9cd..77f302962 100644 --- a/test/testincompletestatement.cpp +++ b/test/testincompletestatement.cpp @@ -33,10 +33,11 @@ public: private: Settings settings; - void check(const char code[]) { + void check(const char code[], bool inconclusive = false) { // Clear the error buffer.. errout.str(""); + settings.inconclusive = inconclusive; // Raw tokens.. std::vector files(1, "test.cpp"); @@ -52,7 +53,6 @@ private: Tokenizer tokenizer(&settings, this); tokenizer.createTokens(&tokens2); tokenizer.simplifyTokens1(""); - tokenizer.simplifyTokenList2(); // Check for incomplete statements.. CheckOther checkOther(&tokenizer, &settings, this); @@ -82,6 +82,8 @@ private: TEST_CASE(cpp11init2); // #8449 TEST_CASE(block); // ({ do_something(); 0; }) TEST_CASE(mapindex); + TEST_CASE(commaoperator); + TEST_CASE(redundantstmts); } void test1() { @@ -299,6 +301,40 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + // #8827 + void commaoperator() { + check("void foo(int,const char*,int);\n" + "void f(int value) {\n" + " foo(42,\"test\",42),(value&42);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Found suspicious operator ','\n", errout.str()); + } + + // #8451 + void redundantstmts() { + check("void f1(int x) {\n" + " 1;\n" + " (1);\n" + " (char)1;\n" + " ((char)1);\n" + " !x;\n" + " (!x);\n" + " (unsigned int)!x;\n" + " ~x;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:2]: (warning) Redundant code: Found a statement that begins with numeric constant.\n" + "[test.cpp:3]: (warning) Redundant code: Found a statement that begins with numeric constant.\n" + "[test.cpp:4]: (warning) Redundant code: Found a statement that begins with string constant.\n" + "[test.cpp:5]: (warning) Redundant code: Found a statement that begins with string constant.\n" + "[test.cpp:6]: (warning, inconclusive) Found suspicious operator '!'\n" + "[test.cpp:7]: (warning, inconclusive) Found suspicious operator '!'\n" + "[test.cpp:9]: (warning, inconclusive) Found suspicious operator '~'\n", errout.str()); + + check("void f1(int x) { x; }", true); + ASSERT_EQUALS("[test.cpp:1]: (warning) Unused variable value 'x'\n", errout.str()); + + } }; REGISTER_TEST(TestIncompleteStatement) diff --git a/test/testother.cpp b/test/testother.cpp index 0874fa13d..ec6f50da2 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1988,7 +1988,7 @@ private: " switch (a)\n" " {\n" " case 2:\n" - " y;\n" + " (void)y;\n" " case 3:\n" " ++y;\n" " }\n" @@ -2042,7 +2042,7 @@ private: " switch (a)\n" " {\n" " case 2:\n" - " y;\n" + " (void)y;\n" " case 3:\n" " --y;\n" " }\n" @@ -2741,8 +2741,12 @@ private: // #4711 lambda functions check("int f() {\n" - " return g([](int x){x+1; return x;});\n" - "}", nullptr, false, false, false); + " return g([](int x){(void)x+1; return x;});\n" + "}", + nullptr, + false, + false, + false); ASSERT_EQUALS("", errout.str()); // #4756 @@ -2754,7 +2758,7 @@ private: " __v = ((unsigned short int) ((((__x) >> 8) & 0xff) | (((__x) & 0xff) << 8)));\n" " else\n" " __asm__ (\"rorw $8, %w0\" : \"=r\" (__v) : \"0\" (__x) : \"cc\");\n" - " __v;\n" + " (void)__v;\n" " }));\n" "}", nullptr, false, false, false); ASSERT_EQUALS("", errout.str()); @@ -5468,13 +5472,13 @@ private: check("void foo()\n" "{\n" " int a; a = 123;\n" - " a << -1;\n" + " (void)(a << -1);\n" "}"); ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value is undefined behaviour\n", errout.str()); check("void foo()\n" "{\n" " int a; a = 123;\n" - " a >> -1;\n" + " (void)(a >> -1);\n" "}"); ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value is undefined behaviour\n", errout.str()); check("void foo()\n"