Merge pull request #87 from HeisSpiter/master

Fix a bug introduced with a previous commit and add a new check

lib/checkmemoryleak.cpp

@@ -2881,7 +2881,7 @@ void CheckMemoryLeakNoVar::check()
             // Handle the case where the user is calling an allocation function
             // and never assigns the returned value to a variable, which will lead to a leak.
             else if (Token::Match(tok2, "[;{}] %var% (") && getAllocationType(tok2->next(), 0) != No)
-                missingAssignementLeak(tok2, tok2->next()->str());
+                missingAssignementLeak(tok2->next(), tok2->next()->str());
         }
     }
 }

lib/checkother.cpp

@@ -562,6 +562,40 @@ void CheckOther::sizeofForStrncmpError(const Token *tok)
                             "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);
+            if (Token::Match(tokVar, "sizeof ( %var% ) )") || Token::Match(tokVar, "sizeof %var% )")) {
+                // Get the variable name
+                tokVar = tokVar->next();
+                if (tokVar->str() == "(")
+                    tokVar = tokVar->next();
+
+                // Check if it matches the assignment variable
+                if (tok->tokAt(1)->str() == tokVar->str()) {
+                    sizeofForMallocError(tok->tokAt(1), tok->tokAt(1)->str());
+                }
+            }
+        }
+    }
+
+    // TODO: This test should be renamed into something more generic
+    // 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",
+                            "Using size of pointer " + varname + " for allocation.\n"
+                            "Using size of pointer " + varname + " for allocation instead "
+                            "of using the size of the type. This is likely to lead to a "
+                            "buffer overflow. You should use sizeof(*" + varname + ")");
+}
+
 //---------------------------------------------------------------------------
 //    switch (x)
 //    {

lib/checkother.h

@@ -61,6 +61,7 @@ public:
         checkOther.checkAssignmentInAssert();
         checkOther.checkSizeofForArrayParameter();
         checkOther.checkSizeofForStrncmpSize();
+        checkOther.checkSizeofForMallocSize();
         checkOther.checkSizeofForNumericParameter();
         checkOther.checkSelfAssignment();
         checkOther.checkDuplicateIf();
@@ -205,6 +206,9 @@ public:
     /** @brief %Check for using sizeof with a char pointer */
     void checkSizeofForStrncmpSize();
 
+    /** @brief %Check for using sizeof of a variable when allocating it */
+    void checkSizeofForMallocSize();
+
     /** @brief %Check for using sizeof with numeric given as function argument */
     void checkSizeofForNumericParameter();
 
@@ -294,6 +298,7 @@ private:
     void memsetZeroBytesError(const Token *tok, const std::string &varname);
     void sizeofForArrayParameterError(const Token *tok);
     void sizeofForStrncmpError(const Token *tok);
+    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);
@@ -328,6 +333,7 @@ private:
         c.misusedScopeObjectError(NULL, "varname");
         c.sizeofForArrayParameterError(0);
         c.sizeofForStrncmpError(0);
+        c.sizeofForMallocError(0, "varname");
         c.sizeofForNumericParameterError(0);
         c.coutCerrMisusageError(0, "cout");
         c.doubleFreeError(0, "varname");
@@ -396,6 +402,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"
                "* 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/testmemleak.cpp

@@ -5250,7 +5250,7 @@ private:
               "{\n"
               "    malloc(10);\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:2]: (error) Return value of allocation function malloc is not used.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (error) Return value of allocation function malloc is not used.\n", errout.str());
 
         check("void *f()\n"
               "{\n"
@@ -5260,7 +5260,7 @@ private:
               "{\n"
               "    f();\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:6]: (error) Return value of allocation function f is not used.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:7]: (error) Return value of allocation function f is not used.\n", errout.str());
     }
 };
 static TestMemleakNoVar testMemleakNoVar;

test/testother.cpp

@@ -151,6 +151,7 @@ private:
 
         TEST_CASE(alwaysTrueFalseStringCompare);
         TEST_CASE(checkStrncmpSizeof);
+        TEST_CASE(checkMallocSizeof);
         TEST_CASE(checkSignOfUnsignedVariable);
 
         TEST_CASE(checkForSuspiciousSemicolon1);
@@ -4181,6 +4182,33 @@ private:
         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);");
+        ASSERT_EQUALS("", errout.str());
+
+        check(
+            "int *x = malloc(sizeof(int));\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        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());
+
+        check(
+            "int *x = malloc(sizeof *x);\n"
+            "free(x);");
+        ASSERT_EQUALS("", errout.str());
+
+        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());
+    }
+
     void check_signOfUnsignedVariable(const char code[], bool inconclusive=false) {
         // Clear the error buffer..
         errout.str("");