Merge the strncmp & malloc sizeof checks into a more generic test that handles several cases where sizeof is misused, or could be misused

lib/checkother.cpp

@@ -518,82 +518,94 @@ void CheckOther::sizeofForArrayParameterError(const Token *tok)
                );
 }
 
-void CheckOther::checkSizeofForStrncmpSize()
+void CheckOther::checkSizeofForPointerSize()
 {
-    // danmar : this is inconclusive in case the size parameter is
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
-    //          sizeof(char *) by intention.
+
-    if (!_settings->inconclusive || !_settings->isEnabled("style"))
+    if(!_settings->isEnabled("style"))
         return;
 
-    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
-    static const char pattern0[] = "strncmp (";
-    static const char pattern1[] = "sizeof ( %var% ) )";
-    static const char pattern2[] = "sizeof %var% )";
-
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
-        if (Token::Match(tok, pattern0)) {
+        const Token *tokVar;
-            const Token *tokVar = tok->tokAt(2);
+        const Token *variable;
-            if (tokVar)
+        const Token *variable2 = 0;
-                tokVar = tokVar->nextArgument();
-            if (tokVar)
-                tokVar = tokVar->nextArgument();
-            if (Token::Match(tokVar, pattern1) || Token::Match(tokVar, pattern2)) {
-                tokVar = tokVar->next();
-                if (tokVar->str() == "(")
-                    tokVar = tokVar->next();
-                if (tokVar->varId() > 0) {
-                    const Variable *var = symbolDatabase->getVariableFromVarId(tokVar->varId());
-                    if (var && var->isPointer()) {
-                        sizeofForStrncmpError(tokVar);
-                    }
-                }
-            }
-        }
-    }
-}
 
-void CheckOther::sizeofForStrncmpError(const Token *tok)
+        // Find any function that may use sizeof on a pointer
-{
+        // Once leaving those tests, it is mandatory to have:
-    reportInconclusiveError(tok, Severity::warning, "strncmpLen",
+        // - variable matching the used pointer
-                            "Passing sizeof(pointer) as the last argument to strncmp.\n"
+        // - tokVar pointing on the argument where sizeof may be used
-                            "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.");
-}
-
-void CheckOther::checkSizeofForMallocSize()
-{
-    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
         if (Token::Match(tok, "[*;{}] %var% = malloc|alloca (")) {
-            const Token *tokVar = tok->tokAt(5);
+            variable = tok->next();
-            if (Token::Match(tokVar, "sizeof ( %var% ) )") || Token::Match(tokVar, "sizeof %var% )")) {
+            tokVar = tok->tokAt(5);
-                // Get the variable name
-                tokVar = tokVar->next();
-                if (tokVar->str() == "(")
-                    tokVar = tokVar->next();
 
-                // Check if it matches the assignment variable
+        } else if (Token::Match(tok, "[*;{}] %var% = calloc (")) {
-                if (tok->tokAt(1)->str() == tokVar->str()) {
+            variable = tok->next();
-                    sizeofForMallocError(tok->tokAt(1), tok->tokAt(1)->str());
+            tokVar = tok->tokAt(5)->nextArgument();
+
+        } else if (Token::Match(tok, "memset (")) {
+            variable = tok->tokAt(2);
+            tokVar = variable->tokAt(2)->nextArgument();
+
+        // The following tests can be inconclusive in case the variable in sizeof
+        // is constant string by intention
+        } else if (!_settings->inconclusive) {
+            continue;
+
+        } else if (Token::Match(tok, "memcpy|memcmp|memmove|strncpy|strncmp|strncat (")) {
+            variable = tok->tokAt(2);
+            variable2 = variable->nextArgument();
+            tokVar = variable2->nextArgument();
+
+        } else {
+            continue;
         }
+
+        // Ensure the variables are in the symbol database
+        // Also ensure the variables are pointers
+        // Only keep variables which are pointers
+        const Variable *var = symbolDatabase->getVariableFromVarId(variable->varId());
+        if (!var || !var->isPointer()) {
+            variable = 0;
+        }
+
+        if (variable2) {
+            var = symbolDatabase->getVariableFromVarId(variable2->varId());
+            if (!var || !var->isPointer()) {
+               variable2 = 0;
+            }
+        }
+
+        // If there are no pointer variable at this point, there is
+        // no need to continue
+        if (variable == 0 && variable2 == 0) {
+            continue;
+        }
+
+        // Jump to the next sizeof token in the function and in the parameter
+        // This is to allow generic operations with sizeof
+        for (; tokVar && tokVar->str() != ")" && tokVar->str() != "," && tokVar->str() != "sizeof"; tokVar = tokVar->next()); 
+
+        // Now check for the sizeof usage. Once here, everything using sizeof(varid)
+        // looks suspicious
+        // Do it for first variable
+        if (variable && (Token::Match(tokVar, "sizeof ( %varid% )", variable->varId()) ||
+            Token::Match(tokVar, "sizeof %varid%", variable->varId()))) {
+            sizeofForPointerError(variable, variable->str());
+        // Then do it for second - TODO: Perhaps we should invert?
+        } else if (variable2 && (Token::Match(tokVar, "sizeof ( %varid% )", variable2->varId()) ||
+            Token::Match(tokVar, "sizeof %varid%", variable2->varId()))) {
+            sizeofForPointerError(variable2, variable2->str());
         }
     }
 }
 
-    // TODO: This test should be renamed into something more generic
+void CheckOther::sizeofForPointerError(const Token *tok, const std::string &varname)
-    // and should provide tests for functions like memcpy, memset and so
-    // on. Test would be done the same way. To prevent misuse with arrays
-    // var->isPointer() is to be used.
-}
-
-void CheckOther::sizeofForMallocError(const Token *tok, const std::string &varname)
 {
-    reportInconclusiveError(tok, Severity::warning, "mallocSize",
+    reportInconclusiveError(tok, Severity::warning, "pointerSize",
-                            "Using size of pointer " + varname + " for allocation.\n"
+                            "Using size of pointer " + varname + " instead of size of its data.\n"
-                            "Using size of pointer " + varname + " for allocation instead "
+                            "Using size of pointer " + varname + " instead of size of its data. "
-                            "of using the size of the type. This is likely to lead to a "
+                            "This is likely to lead to a buffer overflow. You probably intend to "
-                            "buffer overflow. You should use sizeof(*" + varname + ")");
+                            "write sizeof(*" + varname + ")");
 }
 
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -60,8 +60,7 @@ public:
         checkOther.checkRedundantAssignmentInSwitch();
         checkOther.checkAssignmentInAssert();
         checkOther.checkSizeofForArrayParameter();
-        checkOther.checkSizeofForStrncmpSize();
+        checkOther.checkSizeofForPointerSize();
-        checkOther.checkSizeofForMallocSize();
         checkOther.checkSizeofForNumericParameter();
         checkOther.checkSelfAssignment();
         checkOther.checkDuplicateIf();
@@ -203,11 +202,8 @@ 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 of a variable when allocating it */
-    void checkSizeofForMallocSize();
+    void checkSizeofForPointerSize();
 
     /** @brief %Check for using sizeof with numeric given as function argument */
     void checkSizeofForNumericParameter();
@@ -297,8 +293,7 @@ private:
     void misusedScopeObjectError(const Token *tok, const std::string &varname);
     void memsetZeroBytesError(const Token *tok, const std::string &varname);
     void sizeofForArrayParameterError(const Token *tok);
-    void sizeofForStrncmpError(const Token *tok);
+    void sizeofForPointerError(const Token *tok, const std::string &varname);
-    void sizeofForMallocError(const Token *tok, const std::string &varname);
     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);
@@ -332,8 +327,7 @@ private:
         c.fflushOnInputStreamError(0, "stdin");
         c.misusedScopeObjectError(NULL, "varname");
         c.sizeofForArrayParameterError(0);
-        c.sizeofForStrncmpError(0);
+        c.sizeofForPointerError(0, "varname");
-        c.sizeofForMallocError(0, "varname");
         c.sizeofForNumericParameterError(0);
         c.coutCerrMisusageError(0, "cout");
         c.doubleFreeError(0, "varname");
@@ -402,7 +396,7 @@ private:
                "* assignment in an assert statement\n"
                "* sizeof for array given as function argument\n"
                "* sizeof for numeric given as function argument\n"
-               "* using sizeof(pointer) for its own allocation\n"
+               "* using sizeof(pointer) instead of the size of pointed data\n"
                "* incorrect length arguments for 'substr' and 'strncmp'\n"
                "* invalid usage of output stream. For example: std::cout << std::cout;'\n"
                "* wrong number of arguments given to 'printf' or 'scanf;'\n"

test/testother.cpp

@@ -150,8 +150,7 @@ private:
         TEST_CASE(duplicateExpression4); // ticket #3354 (++)
 
         TEST_CASE(alwaysTrueFalseStringCompare);
-        TEST_CASE(checkStrncmpSizeof);
+        TEST_CASE(checkPointerSizeof);
-        TEST_CASE(checkMallocSizeof);
         TEST_CASE(checkSignOfUnsignedVariable);
 
         TEST_CASE(checkForSuspiciousSemicolon1);
@@ -4166,23 +4165,12 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void checkStrncmpSizeof() {
+    void checkPointerSizeof() {
         check(
-            "int fun(const char *buf1)\n"
+            "char *x = malloc(10);\n"
-            "{\n"
+            "free(x);");
-            "  const char *buf1_ex = \"foobarbaz\";\n"
+        ASSERT_EQUALS("", errout.str());
-            "  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());
 
-        check(
-            "int fun(const char *buf1) {\n"
-            "  return strncmp(buf1, foo(buf2), sizeof(buf1)) == 0;\n"
-            "}");
-        ASSERT_EQUALS("[test.cpp:2]: (warning) Passing sizeof(pointer) as the last argument to strncmp.\n", errout.str());
-    }
-
-    void checkMallocSizeof() {
         check(
             "int *x = malloc(sizeof(*x));\n"
             "free(x);");
@@ -4196,7 +4184,17 @@ private:
         check(
             "int *x = malloc(sizeof(x));\n"
             "free(x);");
-        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x for allocation.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(100 * sizeof(x));\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(x) * 100);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
 
         check(
             "int *x = malloc(sizeof *x);\n"
@@ -4206,7 +4204,121 @@ private:
         check(
             "int *x = malloc(sizeof x);\n"
             "free(x);");
-        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x for allocation.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(100 * sizeof x);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = calloc(1, sizeof(*x));\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = calloc(1, sizeof *x);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = calloc(1, sizeof(x));\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = calloc(1, sizeof x);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:1]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = calloc(1, sizeof(int));\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "char x[10];\n"
+            "memset(x, 0, sizeof(x));");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "char x[10];\n"
+            "memset(x, 0, sizeof x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "memset(x, 0, sizeof(int));\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "memset(x, 0, sizeof(*x));\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "memset(x, 0, sizeof *x);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "memset(x, 0, sizeof x);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "memset(x, 0, sizeof(x));\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int) * 10);\n"
+            "memset(x, 0, sizeof(x) * 10);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int) * 10);\n"
+            "memset(x, 0, sizeof x * 10);\n"
+            "free(x);");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Using size of pointer x instead of size of its data.\n", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int) * 10);\n"
+            "memset(x, 0, sizeof(*x) * 10);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int) * 10);\n"
+            "memset(x, 0, sizeof *x * 10);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int) * 10);\n"
+            "memset(x, 0, sizeof(int) * 10);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        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) Using size of pointer buf1_ex instead of size of its data.\n", errout.str());
+
+        check(
+            "int fun(const char *buf1) {\n"
+            "  return strncmp(buf1, foo(buf2), sizeof(buf1)) == 0;\n"
+            "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Using size of pointer buf1 instead of size of its data.\n", errout.str());
     }
 
     void check_signOfUnsignedVariable(const char code[], bool inconclusive=false) {