From 6f164de60979881660ef32a932b3ad807a64c55f Mon Sep 17 00:00:00 2001 From: PKEuS Date: Sun, 11 Mar 2012 11:01:39 +0100 Subject: [PATCH] Improved static string comparision check: Implemented #3214 Fixed false negative on argument count of fnprintf/snprintf when first variable argument is a string. (#3655) Uncommented call of virtualDestructorError in getErrorMessages in checkclass.h Refactorizations: - Rearranged code in checkother.h to make ordering more consistent and to increase encapsulation of private data - Replaced some single-token-patterns --- lib/checkclass.h | 6 +++--- lib/checkother.cpp | 37 +++++++++++++++++++++++-------------- lib/checkother.h | 39 +++++++++++++++++++-------------------- test/testother.cpp | 30 ++++++++++++++++++++++++++++-- 4 files changed, 73 insertions(+), 39 deletions(-) diff --git a/lib/checkclass.h b/lib/checkclass.h index f6418ab17..ccb483fec 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -131,7 +131,7 @@ private: c.unusedPrivateFunctionError(0, "classname", "funcname"); c.memsetError(0, "memfunc", "classname", "class"); c.operatorEqReturnError(0, "class"); - //c.virtualDestructorError(0, "Base", "Derived"); + c.virtualDestructorError(0, "Base", "Derived"); c.thisSubtractionError(0); c.operatorEqRetRefThisError(0); c.operatorEqToSelfError(0); @@ -175,10 +175,10 @@ private: Usage() : assign(false), init(false) { } /** @brief has this variable been assigned? */ - bool assign; + bool assign; /** @brief has this variable been initialized? */ - bool init; + bool init; }; bool isBaseClassFunc(const Token *tok, const Scope *scope); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 61c750c83..23b4c1f65 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -477,7 +477,7 @@ void CheckOther::checkSizeofForArrayParameter() while (declTok->str() == "[") { declTok = declTok->link()->next(); } - if (!(Token::Match(declTok, "= %str%")) && !(Token::simpleMatch(declTok, "= {")) && !(Token::simpleMatch(declTok, ";"))) { + if (!(Token::Match(declTok, "= %str%")) && !(Token::simpleMatch(declTok, "= {")) && declTok->str() != ";") { if (declTok->str() == ",") { while (declTok->str() != ";") { if (declTok->str() == ")") { @@ -1344,8 +1344,7 @@ void CheckOther::checkWrongPrintfScanfArguments() if (Token::Match(formatStringTok, "%str% ,")) { argListTok = formatStringTok->nextArgument(); // Find fourth parameter (first argument of va_args) formatString = formatStringTok->str(); - } - if (Token::Match(formatStringTok, "%str% )")) { + } else if (Token::Match(formatStringTok, "%str% )")) { argListTok = 0; // Find fourth parameter (first argument of va_args) formatString = formatStringTok->str(); } else { @@ -1401,7 +1400,7 @@ void CheckOther::checkWrongPrintfScanfArguments() numFormat++; // Perform type checks - if (_settings->isEnabled("style") && Token::Match(argListTok, "%any% ,|)")) { // We can currently only check the type of arguments matching this simple pattern. + if (_settings->isEnabled("style") && argListTok && Token::Match(argListTok->next(), "[,)]")) { // We can currently only check the type of arguments matching this simple pattern. const Variable* variableInfo = symbolDatabase->getVariableFromVarId(argListTok->varId()); const Token* varTypeTok = variableInfo ? variableInfo->typeStartToken() : NULL; if (varTypeTok && varTypeTok->str() == "static") @@ -1906,7 +1905,7 @@ void CheckOther::lookupVar(const Token *tok1, const std::string &varname) if (indentlevel == 0) return; used1 = true; - if (for_or_while && !Token::simpleMatch(tok->next(), "=")) + if (for_or_while && tok->strAt(1) != "=") used2 = true; if (used1 && used2) return; @@ -1916,7 +1915,7 @@ void CheckOther::lookupVar(const Token *tok1, const std::string &varname) // %unknown% ( %any% ) { // If %unknown% is anything except if, we assume // that it is a for or while loop or a macro hiding either one - if (Token::simpleMatch(tok->next(), "(") && + if (tok->strAt(1) == "(" && Token::simpleMatch(tok->next()->link(), ") {")) { if (tok->str() != "if") for_or_while = true; @@ -2971,35 +2970,45 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() const Token *tok = _tokenizer->tokens(); while (tok && (tok = Token::findmatch(tok, pattern1)) != NULL) { - alwaysTrueFalseStringCompareError(tok, tok->strAt(2), tok->strAt(4)); + const std::string &str1 = tok->strAt(2); + const std::string &str2 = tok->strAt(4); + alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2); tok = tok->tokAt(5); } tok = _tokenizer->tokens(); while (tok && (tok = Token::findmatch(tok, pattern2)) != NULL) { - alwaysTrueFalseStringCompareError(tok, tok->strAt(4), tok->strAt(6)); + const std::string &str1 = tok->strAt(4); + const std::string &str2 = tok->strAt(6); + alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2); tok = tok->tokAt(7); } tok = _tokenizer->tokens(); while (tok && (tok = Token::findmatch(tok, pattern3)) != NULL) { - const Token *var1 = tok->tokAt(2); - const Token *var2 = tok->tokAt(4); - const std::string &str1 = var1->str(); - const std::string &str2 = var2->str(); + const std::string &str1 = tok->strAt(2); + const std::string &str2 = tok->strAt(4); if (str1 == str2) alwaysTrueStringVariableCompareError(tok, str1, str2); tok = tok->tokAt(5); } + + tok = _tokenizer->tokens(); + while (tok && (tok = Token::findmatch(tok, "!!+ %str% ==|!= %str% !!+")) != NULL) { + const std::string &str1 = tok->strAt(1); + const std::string &str2 = tok->strAt(3); + alwaysTrueFalseStringCompareError(tok, str1, str2, str1==str2); + tok = tok->tokAt(5); + } } -void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2) +void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2, bool warning) { const std::size_t stringLen = 10; const std::string string1 = (str1.size() < stringLen) ? str1 : (str1.substr(0, stringLen-2) + ".."); const std::string string2 = (str2.size() < stringLen) ? str2 : (str2.substr(0, stringLen-2) + ".."); - if (str1 == str2) { + if (warning) { reportError(tok, Severity::warning, "staticStringCompare", "Comparison of always identical static strings.\n" "The compared strings, '" + string1 + "' and '" + string2 + "', are always identical. " diff --git a/lib/checkother.h b/lib/checkother.h index f9c9d4809..45aed569e 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -112,11 +112,9 @@ public: /** @brief Clarify calculation for ".. a * b ? .." */ void clarifyCalculation(); - void clarifyCalculationError(const Token *tok, const std::string &op); /** @brief Suspicious condition (assignment+comparison) */ void clarifyCondition(); - void clarifyConditionError(const Token *tok, bool assign, bool boolop); /** @brief Are there C-style pointer casts in a c++ file? */ void warningOldStylePointerCast(); @@ -167,28 +165,15 @@ public: /** @brief %Check for 'sizeof sizeof ..' */ void sizeofsizeof(); - void sizeofsizeofError(const Token *tok); /** @brief %Check for calculations inside sizeof */ void sizeofCalculation(); - void sizeofCalculationError(const Token *tok); /** @brief scanf can crash if width specifiers are not used */ void invalidScanf(); - void invalidScanfError(const Token *tok); /** @brief %Checks type and number of arguments given to functions like printf or scanf*/ void checkWrongPrintfScanfArguments(); - void wrongPrintfScanfArgumentsError(const Token* tok, - const std::string &function, - unsigned int numFormat, - unsigned int numFunction); - void invalidScanfArgTypeError(const Token* tok, const std::string &functionName, unsigned int numFormat); - void invalidPrintfArgTypeError_s(const Token* tok, unsigned int numFormat); - void invalidPrintfArgTypeError_n(const Token* tok, unsigned int numFormat); - void invalidPrintfArgTypeError_p(const Token* tok, unsigned int numFormat); - void invalidPrintfArgTypeError_int(const Token* tok, unsigned int numFormat, char c); - void invalidPrintfArgTypeError_float(const Token* tok, unsigned int numFormat, char c); /** @brief %Check for assigning to the same variable twice in a switch statement*/ void checkRedundantAssignmentInSwitch(); @@ -265,7 +250,23 @@ public: /** @brief %Check for double free or double close operations */ void checkDoubleFree(); +private: // Error messages.. + void clarifyCalculationError(const Token *tok, const std::string &op); + void clarifyConditionError(const Token *tok, bool assign, bool boolop); + void sizeofsizeofError(const Token *tok); + void sizeofCalculationError(const Token *tok); + void invalidScanfError(const Token *tok); + void wrongPrintfScanfArgumentsError(const Token* tok, + const std::string &function, + unsigned int numFormat, + unsigned int numFunction); + void invalidScanfArgTypeError(const Token* tok, const std::string &functionName, unsigned int numFormat); + void invalidPrintfArgTypeError_s(const Token* tok, unsigned int numFormat); + void invalidPrintfArgTypeError_n(const Token* tok, unsigned int numFormat); + void invalidPrintfArgTypeError_p(const Token* tok, unsigned int numFormat); + void invalidPrintfArgTypeError_int(const Token* tok, unsigned int numFormat, char c); + void invalidPrintfArgTypeError_float(const Token* tok, unsigned int numFormat, char c); void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to); void dangerousUsageStrtolError(const Token *tok); @@ -301,7 +302,7 @@ public: void duplicateIfError(const Token *tok1, const Token *tok2); void duplicateBranchError(const Token *tok1, const Token *tok2); void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op); - void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2); + void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2, bool warning); void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2); void duplicateBreakError(const Token *tok); void unreachableCodeError(const Token* tok); @@ -360,7 +361,7 @@ public: c.duplicateIfError(0, 0); c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); - c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); + c.alwaysTrueFalseStringCompareError(0, "str1", "str2", true); c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2"); c.duplicateBreakError(0); c.unreachableCodeError(0); @@ -438,8 +439,6 @@ public: "* optimisation: detect post increment/decrement\n"; } -private: - /** * @brief Used in warningRedundantCode() * Iterates through the %var% tokens in a fully qualified name and concatenates them. @@ -452,7 +451,7 @@ private: *tok = (*tok)->tokAt(2); } - if (Token::Match(*tok, "%var%")) + if ((*tok)->isName()) varname.append((*tok)->str()); return varname; diff --git a/test/testother.cpp b/test/testother.cpp index 9a5ec4fce..1584dcd83 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2207,7 +2207,7 @@ private: " fprintf(stderr,\"%u%s\");\n" " snprintf(str,10,\"%u%s\");\n" " sprintf(string1, \"%-*.*s\", 32, string2);\n" // #3364 - " sprintf(string1, \"%*\", 32);\n" // #3364 + " snprintf(a, 9, \"%s%d\", \"11223344\");\n" // #3655 "}"); ASSERT_EQUALS("[test.cpp:2]: (error) printf format string has 1 parameters but only 0 are given\n" "[test.cpp:3]: (error) printf format string has 2 parameters but only 1 are given\n" @@ -2216,7 +2216,8 @@ private: "[test.cpp:6]: (error) printf format string has 3 parameters but only 2 are given\n" "[test.cpp:7]: (error) fprintf format string has 2 parameters but only 0 are given\n" "[test.cpp:8]: (error) snprintf format string has 2 parameters but only 0 are given\n" - "[test.cpp:9]: (error) sprintf format string has 3 parameters but only 2 are given\n", errout.str()); + "[test.cpp:9]: (error) sprintf format string has 3 parameters but only 2 are given\n" + "[test.cpp:10]: (error) snprintf format string has 2 parameters but only 1 are given\n", errout.str()); check("void foo(char *str) {\n" " printf(\"\", 0);\n" @@ -2239,6 +2240,7 @@ private: " fprintf(stderr, \"error: %m\n\");\n" // #3339 " printf(\"string: %.*s\n\", len, string);\n" // #3311 " fprintf(stderr, \"%*cText.\n\", indent, ' ');\n" // #3313 + " sprintf(string1, \"%*\", 32);\n" // #3364 "}"); ASSERT_EQUALS("", errout.str()); @@ -4138,6 +4140,30 @@ private: " }" "}"); ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (\"str\" == \"str\") {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of always identical static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (\"str\" != \"str\") {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of always identical static strings.\n", errout.str()); + + check_preprocess_suppress( + "int main() {\n" + " if (a+\"str\" != \"str\"+b) {\n" + " std::cout << \"Equal\n\"\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); } void checkStrncmpSizeof() {