From 4ba6ac7332b04384f3a845c9c83f38460ed9f333 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:03:18 +0200 Subject: [PATCH 1/5] Add strncmp to the list of static string comparison functions --- lib/checkother.cpp | 2 +- test/testother.cpp | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d6221af7b..764a2ccaf 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2255,7 +2255,7 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() if (!_settings->isEnabled("style") && !_settings->isEnabled("performance")) return; - const char pattern1[] = "strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %str% , %str% )"; + const char pattern1[] = "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %str% , %str% "; const char pattern2[] = "QString :: compare ( %str% , %str% )"; const Token *tok = _tokenizer->tokens(); diff --git a/test/testother.cpp b/test/testother.cpp index 8655852cb..5b18f3470 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3418,6 +3418,16 @@ private: " }" "}"); ASSERT_EQUALS("", errout.str()); + + check_preprocess_suppress( + "int main()\n" + "{\n" + " if (strncmp(\"hotdog\",\"hotdog\", 6) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of always identical static strings.\n", errout.str()); } void check_signOfUnsignedVariable(const char code[]) { From 90c7db15a018cbefcf8c733cee5aec6a9b32414e Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:05:11 +0200 Subject: [PATCH 2/5] Add check for comparison of identical string variables --- lib/checkother.cpp | 19 +++++++++++++++++++ lib/checkother.h | 2 ++ test/testother.cpp | 10 ++++++++++ 3 files changed, 31 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 764a2ccaf..a5ebea7ee 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2257,6 +2257,7 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() const char pattern1[] = "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %str% , %str% "; const char pattern2[] = "QString :: compare ( %str% , %str% )"; + const char pattern3[] = "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %var% , %var% "; const Token *tok = _tokenizer->tokens(); while (tok && (tok = Token::findmatch(tok, pattern1)) != NULL) { @@ -2269,6 +2270,14 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() alwaysTrueFalseStringCompareError(tok, tok->strAt(4), tok->strAt(6)); 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); + alwaysTrueStringVariableCompareError(tok, var1->str(), var2->str()); + tok = tok->tokAt(5); + } } void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2) @@ -2291,6 +2300,16 @@ void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std:: } } +void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2) +{ + if (str1 == str2) { + reportError(tok, Severity::warning, "stringCompare", + "Comparison of identical string variables.\n" + "The compared strings, '" + str1 + "' and '" + str2 + "', are identical. " + "This could be a logic bug."); + } +} + //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- void CheckOther::sizeofsizeof() diff --git a/lib/checkother.h b/lib/checkother.h index 8dcd03133..60ec133e3 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -273,6 +273,7 @@ public: 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 alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2); void duplicateBreakError(const Token *tok); void assignBoolToPointerError(const Token *tok); void unsignedLessThanZeroError(const Token *tok, const std::string &varname); @@ -326,6 +327,7 @@ public: c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); + c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2"); c.duplicateBreakError(0); c.unsignedLessThanZeroError(0, "varname"); c.unsignedPositiveError(0, "varname"); diff --git a/test/testother.cpp b/test/testother.cpp index 5b18f3470..c27c2f003 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3428,6 +3428,16 @@ private: " }" "}"); ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of always identical static strings.\n", errout.str()); + + check_preprocess_suppress( + "int foo(const char *buf)\n" + "{\n" + " if (strcmp(buf, buf) == 0)" + " {" + " std::cout << \"Equal\n\"" + " }" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); } void check_signOfUnsignedVariable(const char code[]) { From 665cdfabdc10320bdd570223d6f73ea6820072fb Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Fri, 28 Oct 2011 22:06:55 +0200 Subject: [PATCH 3/5] Warn when sizeof is used in strncmp ticket #2095 This checks for the case where the user thought sizeof(buf) gave the size in bytes of 'buf' in code like the following: const char *buf = "Hello World"; strncmp(buf, other, sizeof(buf)); --- lib/checkother.cpp | 31 +++++++++++++++++++++++++++++++ lib/checkother.h | 6 ++++++ test/testother.cpp | 11 +++++++++++ 3 files changed, 48 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a5ebea7ee..cc98c1377 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -421,6 +421,37 @@ void CheckOther::sizeofForArrayParameterError(const Token *tok) ); } +void CheckOther::checkSizeofForStrncmpSize() +{ + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const char pattern1[] = "strncmp ( %any , %any% , sizeof ( %var% ) )"; + const char pattern2[] = "strncmp ( %any , %any% , sizeof %var% )"; + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, pattern1) || Token::Match(tok, pattern2)) { + int tokIdx = 7; + if (tok->tokAt(tokIdx)->str() == "(") + ++tokIdx; + const Token *tokVar = tok->tokAt(tokIdx); + if (tokVar->varId() > 0) { + const Variable *var = symbolDatabase->getVariableFromVarId(tokVar->varId()); + if (var && var->nameToken()->strAt(-1) == "*") { + sizeofForStrncmpError(tokVar); + } + } + } + } +} + +void CheckOther::sizeofForStrncmpError(const Token *tok) +{ + reportError(tok, Severity::warning, "strncmpLen", + "Passing sizeof(pointer) as the last argument to strncmp.\n" + "Passing a pointer to sizeof returns the size of the pointer, not " + "the number of characters pointed to by that pointer. This " + "means that only 4 or 8 (on 64-bit systems) characters are " + "compared, which is probably not what was expected."); +} + //--------------------------------------------------------------------------- // switch (x) // { diff --git a/lib/checkother.h b/lib/checkother.h index 60ec133e3..7f44fd07a 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -58,6 +58,7 @@ public: checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); + checkOther.checkSizeofForStrncmpSize(); checkOther.checkSizeofForNumericParameter(); checkOther.checkSelfAssignment(); checkOther.checkDuplicateIf(); @@ -196,6 +197,9 @@ public: /** @brief %Check for using sizeof with array given as function argument */ void checkSizeofForArrayParameter(); + /** @brief %Check for using sizeof with a char pointer */ + void checkSizeofForStrncmpSize(); + /** @brief %Check for using sizeof with numeric given as function argument */ void checkSizeofForNumericParameter(); @@ -264,6 +268,7 @@ public: void catchExceptionByValueError(const Token *tok); void memsetZeroBytesError(const Token *tok, const std::string &varname); void sizeofForArrayParameterError(const Token *tok); + void sizeofForStrncmpError(const Token *tok); void sizeofForNumericParameterError(const Token *tok); void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len); void incorrectStringBooleanError(const Token *tok, const std::string& string); @@ -294,6 +299,7 @@ public: c.fflushOnInputStreamError(0, "stdin"); c.misusedScopeObjectError(NULL, "varname"); c.sizeofForArrayParameterError(0); + c.sizeofForStrncmpError(0); c.sizeofForNumericParameterError(0); c.coutCerrMisusageError(0, "cout"); diff --git a/test/testother.cpp b/test/testother.cpp index c27c2f003..10f8f0274 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -139,6 +139,7 @@ private: TEST_CASE(duplicateExpression2); // ticket #2730 TEST_CASE(alwaysTrueFalseStringCompare); + TEST_CASE(checkStrncmpSizeof); TEST_CASE(checkSignOfUnsignedVariable); TEST_CASE(checkForSuspiciousSemicolon1); @@ -3440,6 +3441,16 @@ private: ASSERT_EQUALS("[test.cpp:3]: (warning) Comparison of identical string variables.\n", errout.str()); } + void checkStrncmpSizeof() { + check( + "int fun(const char *buf1)\n" + "{\n" + " const char *buf1_ex = \"foobarbaz\";\n" + " return strncmp(buf1, buf1_ex, sizeof(buf1_ex)) == 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Passing sizeof(pointer) as the last argument to strncmp.\n", errout.str()); + } + void check_signOfUnsignedVariable(const char code[]) { // Clear the error buffer.. errout.str(""); From c2d7824130efbbc98d14c86be066431a1a6b3049 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Sat, 29 Oct 2011 11:52:19 +0200 Subject: [PATCH 4/5] Move string comparison out of the report function --- lib/checkother.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cc98c1377..2dc727874 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2306,7 +2306,10 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() while (tok && (tok = Token::findmatch(tok, pattern3)) != NULL) { const Token *var1 = tok->tokAt(2); const Token *var2 = tok->tokAt(4); - alwaysTrueStringVariableCompareError(tok, var1->str(), var2->str()); + const std::string &str1 = var1->str(); + const std::string &str2 = var2->str(); + if (str1 == str2) + alwaysTrueStringVariableCompareError(tok, str1, str2); tok = tok->tokAt(5); } } @@ -2333,12 +2336,10 @@ void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std:: void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2) { - if (str1 == str2) { - reportError(tok, Severity::warning, "stringCompare", - "Comparison of identical string variables.\n" - "The compared strings, '" + str1 + "' and '" + str2 + "', are identical. " - "This could be a logic bug."); - } + reportError(tok, Severity::warning, "stringCompare", + "Comparison of identical string variables.\n" + "The compared strings, '" + str1 + "' and '" + str2 + "', are identical. " + "This could be a logic bug."); } //----------------------------------------------------------------------------- From 45d0709ed56609506217b7dacf207f3b6e52b925 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Sat, 29 Oct 2011 11:59:24 +0200 Subject: [PATCH 5/5] Only run sizeof(char*) check if inconclusive is set --- lib/checkother.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 2dc727874..3368bd7cf 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2302,6 +2302,9 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare() tok = tok->tokAt(7); } + if (!_settings->inconclusive) + return; + tok = _tokenizer->tokens(); while (tok && (tok = Token::findmatch(tok, pattern3)) != NULL) { const Token *var1 = tok->tokAt(2);