From 5c238692e6a15174b09b9bc83bb2d64c5225bf8a Mon Sep 17 00:00:00 2001 From: PKEuS Date: Fri, 8 Aug 2014 09:23:07 +0200 Subject: [PATCH] New check: Division by sizeof() as parameter to memset/memcpy/memmove/etc. as they expect a size in bytes (#5698) Refactorizations in sizeof checking: - Changed severity of sizeofwithsilentarraypointer to warning - Made pointerSize message conclusive - there seems to be no reason for inconclusive --- lib/checksizeof.cpp | 64 +++++++++++++++++++++++++++++---------------- lib/checksizeof.h | 3 ++- test/testsizeof.cpp | 51 ++++++++++++++++++++++-------------- 3 files changed, 76 insertions(+), 42 deletions(-) diff --git a/lib/checksizeof.cpp b/lib/checksizeof.cpp index 4325d35b5..584e1d851 100644 --- a/lib/checksizeof.cpp +++ b/lib/checksizeof.cpp @@ -87,7 +87,7 @@ void CheckSizeof::checkSizeofForArrayParameter() void CheckSizeof::sizeofForArrayParameterError(const Token *tok) { - reportError(tok, Severity::error, + reportError(tok, Severity::warning, "sizeofwithsilentarraypointer", "Using 'sizeof' on array given as function argument " "returns size of a pointer.\n" "Using 'sizeof' for array given as function argument returns the size of a pointer. " @@ -111,8 +111,9 @@ void CheckSizeof::checkSizeofForPointerSize() for (std::size_t i = 0; i < functions; ++i) { const Scope * scope = symbolDatabase->functionScopes[i]; for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { - const Token *tokVar; - const Token *variable; + const Token* tokSize; + const Token* tokFunc; + const Token *variable = nullptr; const Token *variable2 = nullptr; // Find any function that may use sizeof on a pointer @@ -121,32 +122,45 @@ void CheckSizeof::checkSizeofForPointerSize() // - tokVar pointing on the argument where sizeof may be used if (Token::Match(tok, "[*;{}] %var% = malloc|alloca (")) { variable = tok->next(); - tokVar = tok->tokAt(5); - + tokSize = tok->tokAt(5); + tokFunc = tok->tokAt(3); } else if (Token::Match(tok, "[*;{}] %var% = calloc (")) { variable = tok->next(); - tokVar = tok->tokAt(5)->nextArgument(); - + tokSize = tok->tokAt(5)->nextArgument(); + tokFunc = tok->tokAt(3); + } else if (Token::Match(tok, "return malloc|alloca (")) { + tokSize = tok->tokAt(3); + tokFunc = tok->next(); + } else if (Token::simpleMatch(tok, "return calloc (")) { + tokSize = tok->tokAt(3)->nextArgument(); + tokFunc = tok->next(); } else if (Token::simpleMatch(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; - + tokSize = variable->nextArgument(); + if (tokSize) + tokSize = tokSize->nextArgument(); + tokFunc = tok; } else if (Token::Match(tok, "memcpy|memcmp|memmove|strncpy|strncmp|strncat (")) { variable = tok->tokAt(2); variable2 = variable->nextArgument(); if (!variable2) continue; - tokVar = variable2->nextArgument(); - + tokSize = variable2->nextArgument(); + tokFunc = tok; } else { continue; } + if (tokFunc && tokSize) { + for (const Token* tok2 = tokSize; tok2 != tokFunc->linkAt(1); tok2 = tok2->next()) { + if (Token::simpleMatch(tok2, "/ sizeof")) + divideBySizeofError(tok2, tokFunc->str()); + } + } + + if (!variable) + continue; + // Ensure the variables are in the symbol database // Also ensure the variables are pointers // Only keep variables which are pointers @@ -170,16 +184,16 @@ void CheckSizeof::checkSizeofForPointerSize() // 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()) {} + for (; tokSize && tokSize->str() != ")" && tokSize->str() != "," && tokSize->str() != "sizeof"; tokSize = tokSize->next()) {} // Now check for the sizeof usage. Once here, everything using sizeof(varid) or 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()))) { + if (variable && (Token::Match(tokSize, "sizeof ( &| %varid% )", variable->varId()) || + Token::Match(tokSize, "sizeof &| %varid%", variable->varId()))) { sizeofForPointerError(variable, variable->str()); - } else if (variable2 && (Token::Match(tokVar, "sizeof ( &| %varid% )", variable2->varId()) || - Token::Match(tokVar, "sizeof &| %varid%", variable2->varId()))) { + } else if (variable2 && (Token::Match(tokSize, "sizeof ( &| %varid% )", variable2->varId()) || + Token::Match(tokSize, "sizeof &| %varid%", variable2->varId()))) { sizeofForPointerError(variable2, variable2->str()); } } @@ -192,7 +206,13 @@ void CheckSizeof::sizeofForPointerError(const Token *tok, const std::string &var "Size of pointer '" + varname + "' used instead of size of its data.\n" "Size of pointer '" + varname + "' used instead of size of its data. " "This is likely to lead to a buffer overflow. You probably intend to " - "write 'sizeof(*" + varname + ")'.", true); + "write 'sizeof(*" + varname + ")'."); +} + +void CheckSizeof::divideBySizeofError(const Token *tok, const std::string &memfunc) +{ + reportError(tok, Severity::warning, "sizeofDivisionMemfunc", + "Division by result of sizeof(). " + memfunc + "() expects a size in bytes, did you intend to multiply instead?\n"); } //----------------------------------------------------------------------------- diff --git a/lib/checksizeof.h b/lib/checksizeof.h index d82eaa202..4350c2b03 100644 --- a/lib/checksizeof.h +++ b/lib/checksizeof.h @@ -92,6 +92,7 @@ private: void divideSizeofError(const Token* tok); void sizeofForArrayParameterError(const Token* tok); void sizeofForPointerError(const Token* tok, const std::string &varname); + void divideBySizeofError(const Token* tok, const std::string &memfunc); void sizeofForNumericParameterError(const Token* tok); void sizeofVoidError(const Token *tok); void sizeofDereferencedVoidPointerError(const Token *tok, const std::string &varname); @@ -102,6 +103,7 @@ private: c.sizeofForArrayParameterError(0); c.sizeofForPointerError(0, "varname"); + c.divideBySizeofError(0, "memset"); c.sizeofForNumericParameterError(0); c.sizeofsizeofError(0); c.sizeofCalculationError(0, false); @@ -118,7 +120,6 @@ private: std::string classInfo() const { return "sizeof() usage checks\n" - "* sizeof for array given as function argument\n" "* sizeof for numeric given as function argument\n" "* using sizeof(pointer) instead of the size of pointed data\n" diff --git a/test/testsizeof.cpp b/test/testsizeof.cpp index bca511067..f73fb5ce1 100644 --- a/test/testsizeof.cpp +++ b/test/testsizeof.cpp @@ -36,6 +36,7 @@ private: TEST_CASE(sizeofsizeof); TEST_CASE(sizeofCalculation); TEST_CASE(checkPointerSizeof); + TEST_CASE(sizeofDivisionMemset); TEST_CASE(sizeofForArrayParameter); TEST_CASE(sizeofForNumericParameter); TEST_CASE(suspiciousSizeofCalculation); @@ -165,21 +166,21 @@ private: " std::cout << sizeof(a) / sizeof(int) << std::endl;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (error) Using 'sizeof' on array given as " + ASSERT_EQUALS("[test.cpp:2]: (warning) Using 'sizeof' on array given as " "function argument returns size of a pointer.\n", errout.str()); check("void f( int a[]) {\n" " std::cout << sizeof a / sizeof(int) << std::endl;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (error) Using 'sizeof' on array given as " + ASSERT_EQUALS("[test.cpp:2]: (warning) Using 'sizeof' on array given as " "function argument returns size of a pointer.\n", errout.str()); check("void f( int a[3] ) {\n" " std::cout << sizeof(a) / sizeof(int) << std::endl;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (error) Using 'sizeof' on array given as " + ASSERT_EQUALS("[test.cpp:2]: (warning) Using 'sizeof' on array given as " "function argument returns size of a pointer.\n", errout.str()); check("void f(int *p) {\n" @@ -223,7 +224,7 @@ private: " std::cout << sizeof(a) / sizeof(int) << std::endl;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (error) Using 'sizeof' on array given as " + ASSERT_EQUALS("[test.cpp:2]: (warning) Using 'sizeof' on array given as " "function argument returns size of a pointer.\n", errout.str()); // ticket #2510 @@ -231,7 +232,7 @@ private: " std::cout << sizeof(a) / sizeof(int) << std::endl;\n" "}\n" ); - ASSERT_EQUALS("[test.cpp:2]: (error) Using 'sizeof' on array given as " + ASSERT_EQUALS("[test.cpp:2]: (warning) Using 'sizeof' on array given as " "function argument returns size of a pointer.\n", errout.str()); // ticket #2510 @@ -322,25 +323,25 @@ private: " int *x = malloc(sizeof(x));\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(&x));\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(100 * sizeof(x));\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(x) * 100);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof *x);\n" @@ -352,13 +353,13 @@ private: " int *x = malloc(sizeof x);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(100 * sizeof x);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = calloc(1, sizeof(*x));\n" @@ -376,13 +377,13 @@ private: " int *x = calloc(1, sizeof(x));\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = calloc(1, sizeof x);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = calloc(1, sizeof(int));\n" @@ -434,28 +435,28 @@ private: " memset(x, 0, sizeof x);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(int));\n" " memset(x, 0, sizeof(x));\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(int) * 10);\n" " memset(x, 0, sizeof(x) * 10);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(int) * 10);\n" " memset(x, 0, sizeof x * 10);\n" " free(x);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Size of pointer 'x' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Size of pointer 'x' used instead of size of its data.\n", errout.str()); check("void f() {\n" " int *x = malloc(sizeof(int) * 10);\n" @@ -484,13 +485,13 @@ private: " const char *buf1_ex = \"foobarbaz\";\n" " return strncmp(buf1, buf1_ex, sizeof(buf1_ex)) == 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning, inconclusive) Size of pointer 'buf1_ex' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) Size of pointer 'buf1_ex' used 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, inconclusive) Size of pointer 'buf1' used instead of size of its data.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Size of pointer 'buf1' used instead of size of its data.\n", errout.str()); // #ticket 3874 check("void f()\n" @@ -501,6 +502,18 @@ private: ASSERT_EQUALS("", errout.str()); } + void sizeofDivisionMemset() { + check("void foo(memoryMapEntry_t* entry, memoryMapEntry_t* memoryMapEnd) {\n" + " memmove(entry, entry + 1, (memoryMapEnd - entry) / sizeof(entry));\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Division by result of sizeof(). memmove() expects a size in bytes, did you intend to multiply instead?\n", errout.str()); + + check("Foo* allocFoo(int num) {\n" + " return malloc(num / sizeof(Foo));\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Division by result of sizeof(). malloc() expects a size in bytes, did you intend to multiply instead?\n", errout.str()); + } + void sizeofVoid() { check("void f() {\n" " int size = sizeof(void);\n"