diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp index c316072b8..678a00da2 100644 --- a/lib/checkstring.cpp +++ b/lib/checkstring.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include //--------------------------------------------------------------------------- @@ -331,6 +332,91 @@ void CheckString::incorrectStringBooleanError(const Token *tok, const std::strin reportError(tok, Severity::warning, "incorrectStringBooleanError", "Conversion of string literal " + string + " to bool always evaluates to true.", CWE571, false); } +//--------------------------------------------------------------------------- +// always true: strcmp(str,"a")==0 || strcmp(str,"b") +// TODO: Library configuration for string comparison functions +//--------------------------------------------------------------------------- +void CheckString::overlappingStringComparisons() +{ + if (!_settings->isEnabled(Settings::WARNING)) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (tok->str() != "||") + continue; + std::list equals0; + std::list notEquals0; + std::stack tokens; + tokens.push(tok); + while (!tokens.empty()) { + const Token * const t = tokens.top(); + tokens.pop(); + if (!t) + continue; + if (t->str() == "||") { + tokens.push(t->astOperand1()); + tokens.push(t->astOperand2()); + continue; + } + if (t->str() == "==") { + if (Token::simpleMatch(t->astOperand1(), "(") && Token::simpleMatch(t->astOperand2(), "0")) + equals0.push_back(t->astOperand1()); + else if (Token::simpleMatch(t->astOperand2(), "(") && Token::simpleMatch(t->astOperand1(), "0")) + equals0.push_back(t->astOperand2()); + continue; + } + if (t->str() == "!=") { + if (Token::simpleMatch(t->astOperand1(), "(") && Token::simpleMatch(t->astOperand2(), "0")) + notEquals0.push_back(t->astOperand1()); + else if (Token::simpleMatch(t->astOperand2(), "(") && Token::simpleMatch(t->astOperand1(), "0")) + notEquals0.push_back(t->astOperand2()); + continue; + } + if (t->str() == "!" && Token::simpleMatch(t->astOperand1(), "(")) + equals0.push_back(t->astOperand1()); + else if (t->str() == "(") + notEquals0.push_back(t); + } + + bool error = false; + for (std::list::const_iterator eq0 = equals0.begin(); !error && eq0 != equals0.end(); ++eq0) { + for (std::list::const_iterator ne0 = notEquals0.begin(); !error && ne0 != notEquals0.end(); ++ne0) { + const Token *tok1 = *eq0; + const Token *tok2 = *ne0; + if (!Token::simpleMatch(tok1->previous(), "strcmp (")) + continue; + if (!Token::simpleMatch(tok2->previous(), "strcmp (")) + continue; + const std::vector args1 = getArguments(tok1->previous()); + const std::vector args2 = getArguments(tok2->previous()); + if (args1.size() != 2 || args2.size() != 2) + continue; + if (args1[1]->isLiteral() && + args2[1]->isLiteral() && + args1[1]->str() != args2[1]->str() && + isSameExpression(_tokenizer->isCPP(), true, args1[0], args2[0], _settings->library, true)) + overlappingStringComparisonsError(tok1, tok2); + } + } + } + } +} + +void CheckString::overlappingStringComparisonsError(const Token *eq0, const Token *ne0) +{ + std::string eq0Expr(eq0 ? eq0->expressionString() : std::string("strcmp(x,\"abc\")")); + if (eq0 && eq0->astParent()->str() == "!") + eq0Expr = "!" + eq0Expr; + else + eq0Expr += " == 0"; + const std::string ne0Expr = (ne0 ? ne0->expressionString() : std::string("strcmp(x,\"def\")")) + " != 0"; + reportError(ne0, Severity::warning, "overlappingStringComparisons", "The comparison operator in '" + ne0Expr + "' should maybe be '==' instead, currently the expression '" + eq0Expr + "' is redundant."); +} + //--------------------------------------------------------------------------- // Overlapping source and destination passed to sprintf(). // TODO: Library configuration for overlapping arguments diff --git a/lib/checkstring.h b/lib/checkstring.h index d9f22387a..a77362ace 100644 --- a/lib/checkstring.h +++ b/lib/checkstring.h @@ -57,6 +57,7 @@ public: checkString.strPlusChar(); checkString.checkSuspiciousStringCompare(); checkString.stringLiteralWrite(); + checkString.overlappingStringComparisons(); } /** @brief Run checks against the simplified token list */ @@ -84,6 +85,9 @@ public: /** @brief %Check for suspicious code that compares string literals for equality */ void checkAlwaysTrueOrFalseStringCompare(); + /** @brief %Check for overlapping string comparisons */ + void overlappingStringComparisons(); + /** @brief %Check for overlapping source and destination passed to sprintf() */ void sprintfOverlappingData(); @@ -97,6 +101,7 @@ private: void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2); void suspiciousStringCompareError(const Token* tok, const std::string& var); void suspiciousStringCompareError_char(const Token* tok, const std::string& var); + void overlappingStringComparisonsError(const Token* tok1, const Token *tok2); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckString c(nullptr, settings, errorLogger); @@ -110,6 +115,7 @@ private: c.incorrectStringBooleanError(nullptr, "\"Hello World\""); c.alwaysTrueFalseStringCompareError(nullptr, "str1", "str2"); c.alwaysTrueStringVariableCompareError(nullptr, "varname1", "varname2"); + c.overlappingStringComparisonsError(nullptr, nullptr); } static std::string myName() { @@ -123,7 +129,8 @@ private: "- suspicious condition (runtime comparison of string literals)\n" "- suspicious condition (string literals as boolean)\n" "- suspicious comparison of a string literal with a char* variable\n" - "- suspicious comparison of '\\0' with a char* variable\n"; + "- suspicious comparison of '\\0' with a char* variable\n" + "- overlapping string comparisons (strcmp(str,\"x\")==0 || strcmp(str,\"y\")!=0)"; } }; /// @} diff --git a/test/teststring.cpp b/test/teststring.cpp index 3808b9d05..e2339fbb1 100644 --- a/test/teststring.cpp +++ b/test/teststring.cpp @@ -51,6 +51,8 @@ private: TEST_CASE(sprintf4); // struct member TEST_CASE(incorrectStringCompare); + + TEST_CASE(overlappingStringComparisons); } void check(const char code[], const char filename[] = "test.cpp") { @@ -578,6 +580,22 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void overlappingStringComparisons() { + check("void f(const char *str) {\n" + " if (strcmp(str, \"abc\") == 0 || strcmp(str, \"def\")) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) The comparison operator in 'strcmp(str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(str,\"abc\") == 0' is redundant.\n", errout.str()); + + check("struct X {\n" + " char *str;\n" + "};\n" + "\n" + "void f(const struct X *x) {\n" + " if (strcmp(x->str, \"abc\") == 0 || strcmp(x->str, \"def\")) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:6]: (warning) The comparison operator in 'strcmp(x->str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(x->str,\"abc\") == 0' is redundant.\n", errout.str()); + } }; REGISTER_TEST(TestString)