From 2b5ddb78584eb131238c980db489185a83c4e988 Mon Sep 17 00:00:00 2001 From: Pierre Schweitzer Date: Fri, 23 Mar 2012 21:47:13 +0100 Subject: [PATCH] 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 | 138 ++++++++++++++++++++++------------------- lib/checkother.h | 16 ++--- test/testother.cpp | 150 +++++++++++++++++++++++++++++++++++++++------ 3 files changed, 211 insertions(+), 93 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 9889d8688..5497dfb10 100644 --- a/lib/checkother.cpp +++ b/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 - // sizeof(char *) by intention. - if (!_settings->inconclusive || !_settings->isEnabled("style")) + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + 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 = tok->tokAt(2); - if (tokVar) - 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); - } - } - } - } - } -} + const Token *tokVar; + const Token *variable; + const Token *variable2 = 0; -void CheckOther::sizeofForStrncmpError(const Token *tok) -{ - reportInconclusiveError(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."); -} - -void CheckOther::checkSizeofForMallocSize() -{ - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + // Find any function that may use sizeof on a pointer + // Once leaving those tests, it is mandatory to have: + // - variable matching the used pointer + // - tokVar pointing on the argument where sizeof may be used 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(); + variable = tok->next(); + tokVar = tok->tokAt(5); - // Check if it matches the assignment variable - if (tok->tokAt(1)->str() == tokVar->str()) { - sizeofForMallocError(tok->tokAt(1), tok->tokAt(1)->str()); - } + } else if (Token::Match(tok, "[*;{}] %var% = calloc (")) { + variable = tok->next(); + 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; } } - } - // 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. + // 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()); + } + } } -void CheckOther::sizeofForMallocError(const Token *tok, const std::string &varname) +void CheckOther::sizeofForPointerError(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 + ")"); + reportInconclusiveError(tok, Severity::warning, "pointerSize", + "Using size of pointer " + varname + " instead of size of its data.\n" + "Using size of pointer " + varname + " instead of size of its data. " + "This is likely to lead to a buffer overflow. You probably intend to " + "write sizeof(*" + varname + ")"); } //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 08513b616..1c8db497b 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -60,8 +60,7 @@ public: checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); - checkOther.checkSizeofForStrncmpSize(); - checkOther.checkSizeofForMallocSize(); + checkOther.checkSizeofForPointerSize(); 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 sizeofForMallocError(const Token *tok, const std::string &varname); + void sizeofForPointerError(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.sizeofForMallocError(0, "varname"); + c.sizeofForPointerError(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" diff --git a/test/testother.cpp b/test/testother.cpp index 81fad2b80..cb4e479ba 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -150,8 +150,7 @@ private: TEST_CASE(duplicateExpression4); // ticket #3354 (++) TEST_CASE(alwaysTrueFalseStringCompare); - TEST_CASE(checkStrncmpSizeof); - TEST_CASE(checkMallocSizeof); + TEST_CASE(checkPointerSizeof); 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" - "{\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()); + "char *x = malloc(10);\n" + "free(x);"); + ASSERT_EQUALS("", 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) {