From 9d002916f47bff979d965189381c68116b81391a Mon Sep 17 00:00:00 2001 From: Pierre Schweitzer Date: Tue, 20 Mar 2012 18:41:10 +0100 Subject: [PATCH 1/2] Fix line number displayed on leakNoVarr error --- lib/checkmemoryleak.cpp | 2 +- test/testmemleak.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index 711a44386..11b98cd23 100644 --- a/lib/checkmemoryleak.cpp +++ b/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()); } } } diff --git a/test/testmemleak.cpp b/test/testmemleak.cpp index 498444b9b..8a9700c6b 100644 --- a/test/testmemleak.cpp +++ b/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; From ec44967e1317df50e4b79148afd99b78ad8e517b Mon Sep 17 00:00:00 2001 From: Pierre Schweitzer Date: Tue, 20 Mar 2012 18:50:05 +0100 Subject: [PATCH 2/2] Add a new test to check improper sizeof usage. It's for the moment limited to malloc calls. --- lib/checkother.cpp | 34 ++++++++++++++++++++++++++++++++++ lib/checkother.h | 7 +++++++ test/testother.cpp | 28 ++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7096bff53..7455a90ef 100644 --- a/lib/checkother.cpp +++ b/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) // { diff --git a/lib/checkother.h b/lib/checkother.h index 380a64681..08513b616 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index 58f0b926e..81fad2b80 100644 --- a/test/testother.cpp +++ b/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("");