Merge pull request #50 from richq/strncmp

Improved strncmp checks
 * strings are always the same
 * inconclusive: using sizeof(char *) as size parameter

lib/checkother.cpp

@@ -419,6 +419,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)
 //    {
@@ -2253,8 +2284,9 @@ 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 char pattern3[] = "strncmp|strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %var% , %var% ";
 
     const Token *tok = _tokenizer->tokens();
     while (tok && (tok = Token::findmatch(tok, pattern1)) != NULL) {
@@ -2267,6 +2299,20 @@ void CheckOther::checkAlwaysTrueOrFalseStringCompare()
         alwaysTrueFalseStringCompareError(tok, tok->strAt(4), tok->strAt(6));
         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);
+        const Token *var2 = tok->tokAt(4);
+        const std::string &str1 = var1->str();
+        const std::string &str2 = var2->str();
+        if (str1 == str2)
+            alwaysTrueStringVariableCompareError(tok, str1, str2);
+        tok = tok->tokAt(5);
+    }
 }
 
 void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2)
@@ -2289,6 +2335,14 @@ void CheckOther::alwaysTrueFalseStringCompareError(const Token *tok, const std::
     }
 }
 
+void CheckOther::alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& 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()

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);
@@ -273,6 +278,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);
@@ -293,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");
 
@@ -326,6 +333,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");

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);
@@ -3418,6 +3419,36 @@ 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());
+
+        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 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[]) {