diff --git a/lib/checkother.cpp b/lib/checkother.cpp index fa0b228f3..df78bca7a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3061,58 +3061,41 @@ void CheckOther::checkRedundantCopy() for (const Token *tok = _tokenizer->tokens(); tok; tok=tok->next()) { const char *expect_end_token; - if (Token::Match(tok, "const %type% %var% =")) { - //match "const A a =" usage - expect_end_token = ";"; - } else if (Token::Match(tok, "const %type% %var% (")) { - //match "const A a (" usage - expect_end_token = ")"; - } else { - continue; - } - - if (tok->strAt(1) == tok->strAt(4)) //avoid "const A a = A();" - continue; - if (!symbolDatabase->isClassOrStruct(tok->next()->str())) //avoid when %type% is standard type - continue; - const Token *var_tok = tok->tokAt(2); - tok = tok->tokAt(4); - while (tok &&Token::Match(tok,"%var% .")) - tok = tok->tokAt(2); - if (!Token::Match(tok, "%var% (")) - break; - const Token *match_end = (tok->next()->link()!=NULL)?tok->next()->link()->next():NULL; - if (match_end==NULL || !Token::Match(match_end,expect_end_token)) //avoid usage like "const A a = getA()+3" - break; - const Token *fToken = _tokenizer->getFunctionTokenByName(tok->str().c_str()); - if (fToken &&fToken->previous() && fToken->previous()->str() == "&") { - redundantCopyError(var_tok,var_tok->str()); - } - } -} -void CheckOther::redundantCopyError(const Token *tok,const std::string& varname) + if (Token::Match(tok, "const %type% %var% =")) + Token::simpleMatch(tok1->linkAt(3), ") {")) { + // get the +// Check for incompletely filled buffers. Token::simpleMatch(tok1->linkAt(3), ") {")) { + // get the +void CheckOther::checkIncompleteArrayFill() { - reportError(tok, Severity::performance,"redundantCopyLocalConst", - "Use const reference for "+varname+" to avoid unnecessary data copying.\n" - "The const "+varname+" gets a copy of the data since const reference is not used. You can avoid the unnecessary data copying by converting "+varname+" to const reference instead of just const."); -} + if (!_settings->inconclusive || ion = tok1->tokAt(4)->stringifyList(tok1->linkAt(3)); -//--------------------------------------------------------------------------- -// Checking for shift by negative values -//--------------------------------------------------------------------------- + // try to look up the expression to check for duplicates + for (const Token* tok = _tokenizer->list.front(); tok; tok = tok->next()) { + if (Token::Match(tok, "memset|memcpy|memmove ( %var% ,") && Token::Match(tok->linkAt(1)->tokAt(-2), ", %num% )")) { + const Variable* var = symbolDatabase->getVariableFromVarId(tok->tokAt(2)->varId()); + if (!var || !var->isArray() || var->dimensions().empty() || !var->dimension(0)) + continue; -void CheckOther::checkNegativeBitwiseShift() -{ - for (const Token *tok = _tokenizer->tokens(); tok ; tok = tok->next()) { - if (Token::Match(tok,"%var% >>|<< %num%") || Token::Match(tok,"%num >>|<< %num%")) { - if ((tok->strAt(2))[0] == '-') - negativeBitwiseShiftError(tok); + if (MathLib::toLongNumber(tok->linkAt(1)->strAt(-1)) == var->dimension(0)) { + unsigned int size = _tokenizer->sizeOfType(var->typeStartToken()); + if ((size != 1 && size != 100 && size != 0) || Token::Match(var->typeEndToken(), "*")) + incompleteArrayFillError(tok, var->name(), tok->str(), false); + else if (var->typeStartToken()->str() == "bool" && _settings->isEnabled("portability")) // sizeof(bool) is not 1 on all platforms + incompleteArrayFillError(tok, var->name(), tok->str(), true); + } } } } - -void CheckOther::negativeBitwiseShiftError(const Token *tok) +void CheckOther::incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean) { - reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value."); + if (boolean) + reportError(tok, Severity::portability, "incompleteArrayFill", + "Array '" + buffer + "' might be filled incompletely. Did you forget to multiply the size given to '" + function + "()' with 'sizeof(*" + buffer + ")'?\n" + "The array '" + buffer + "' is filled incompletely. The function '" + function + "()' needs the size given in bytes, but the type 'bool' is larger than 1 on some platforms. Did you forget to multiply the size with 'sizeof(*" + buffer + ")'?", true); + else + reportError(tok, Severity::warning, "incompleteArrayFill", + "Array '" + buffer + "' is filled incompletely. Did you forget to multiply the size given to '" + function + "()' with 'sizeof(*" + buffer + ")'?\n" + "The array '" + buffer + "' is filled incompletely. The function '" + function + "()' needs the size given in bytes, but an element of the given array is larger than one byte. Did you forget to multiply the size with 'sizeof(*" + buffer + ")'?", true); } diff --git a/lib/checkother.h b/lib/checkother.h index 98c3f5ba3..15545d6e9 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -74,6 +74,7 @@ public: checkOther.clarifyCondition(); // not simplified because ifAssign checkOther.checkComparisonOfBoolExpressionWithInt(); checkOther.checkSignOfUnsignedVariable(); // don't ignore casts (#3574) + checkOther.checkIncompleteArrayFill(); } /** @brief Run checks against the simplified token list */ @@ -238,7 +239,8 @@ public: void checkDoubleFree(); void doubleFreeError(const Token *tok, const std::string &varname); - /** @brief %Check for code creating redundant copies */ + /** @brief %Check for code creating redu /** @brief %Check for buffers that are filled incompletely with memset and similar functions */ + void checkIncompleteArrayFill redundant copies */ void checkRedundantCopy(); /** @brief %Check for bitwise operation with negative right operand */ @@ -297,7 +299,7 @@ private: void pointerPositiveError(const Token *tok, bool inconclusive); void bitwiseOnBooleanError(const Token *tok, const std::string &varname, const std::string &op); void comparisonOfBoolExpressionWithIntError(const Token *tok, bool n0o1); - void SuspiciousSemicolonError(const Token *tok); + void SuspiciousSemicolonError(con void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool booleanconst Token *tok); void doubleCloseDirError(const Token *tok, const std::string &varname); void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal); void negativeBitwiseShiftError(const Token *tok); @@ -356,6 +358,7 @@ private: c.duplicateBreakError(0, false); c.unreachableCodeError(0, false); c.unsignedLessThanZeroError(0, "varname", false); + c.incompleteArrayFillError(0, "buffer", "memset", falselse); c.unsignedPositiveError(0, "varname", false); c.pointerLessThanZeroError(0, false); c.pointerPositiveError(0, false); @@ -414,7 +417,8 @@ private: "* comparison of a boolean expression with an integer other than 0 or 1\n" "* suspicious condition (assignment+comparison)\n" "* suspicious condition (runtime comparison of string literals)\n" - "* suspicious condition (string literals as boolean)\n" + "* suspic + "* Array filled incompletely using memset/memcpy/memmovuspicious condition (string literals as boolean)\n" "* duplicate break statement\n" "* unreachable code\n" "* testing if unsigned variable is negative\n" @@ -426,16 +430,4 @@ private: } void checkExpressionRange(const std::list &constFunctions, - const Token *start, - const Token *end, - const std::string &toCheck); - - void complexDuplicateExpressionCheck(const std::list &constFunctions, - const Token *classStart, - const std::string &toCheck, - const std::string &alt); -}; -/// @} -//--------------------------------------------------------------------------- -#endif - + \ No newline at end of file diff --git a/test/testother.cpp b/test/testother.cpp index 8b797f9b1..df5b66644 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -159,7 +159,8 @@ private: TEST_CASE(checkForSuspiciousSemicolon1); TEST_CASE(checkForSuspiciousSemicolon2); - TEST_CASE(checkDoubleFree); + TEST_CASE(checkDoub + TEST_CASE(incompleteArrayFilloubleFree); TEST_CASE(checkRedundantCopy); @@ -168,6 +169,7 @@ private: void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) { // Clear the error buffer.. + errout.straddEnabled("portability.. errout.str(""); Settings settings; @@ -3028,15 +3030,15 @@ private: "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:5]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:6]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:7]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); + "[test.cpp:6]: (warning) Comparison of modulo result is predetermined, because icause it is always less than 5.\n" + "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, x > 5 && x != l conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); - check("void f(bool& b1, bool& b2) {\n" - " b1 = bar() % 5 < 889;\n" - " if(x[593] % 5 <= 5)\n" - " b2 = x.a % 5 == 5;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" + check("void "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning)> 5) && (x != 1) conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); + + check("void f(int x) {\n" +sult is predetermined, because it is always less than 5.\n" "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n", errout.str()); } @@ -5446,396 +5448,54 @@ private: ASSERT_EQUALS("", errout.str()); check( - "void f() {\n" - " char *p = malloc(100);\n" - " if (x) {\n" - " free(p);\n" - " exit();\n" - " }\n" - " free(p);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void f() {\n" - " char *p = malloc(100);\n" - " if (x) {\n" - " free(p);\n" - " x = 0;\n" - " }\n" - " free(p);\n" - "}"); - ASSERT_EQUALS("[test.cpp:7]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void f() {\n" - " char *p = do_something();\n" - " free(p);\n" - " p = do_something();\n" - " free(p);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " g_free(p);\n" - " g_free(p);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void foo(char *p, char *r) {\n" - " g_free(p);\n" - " g_free(r);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " g_free(p);\n" - " getNext(&p);\n" - " g_free(p);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " g_free(p);\n" - " bar();\n" - " g_free(p);\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void foo(char *p) {\n" - " delete p;\n" - " delete p;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void foo(char *p, char *r) {\n" - " delete p;\n" - " delete r;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " delete p;\n" - " getNext(&p);\n" - " delete p;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " delete p;\n" - " bar();\n" - " delete p;\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void foo(char *p) {\n" - " delete[] p;\n" - " delete[] p;\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "void foo(char *p, char *r) {\n" - " delete[] p;\n" - " delete[] r;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " delete[] p;\n" - " getNext(&p);\n" - " delete[] p;\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(char *p) {\n" - " delete[] p;\n" - " bar();\n" - " delete[] p;\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - - check( - "~LineMarker() {\n" - " delete pxpm;\n" - "}\n" - "LineMarker &operator=(const LineMarker &) {\n" - " delete pxpm;\n" - " pxpm = NULL;\n" - " return *this;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo()\n" - "{\n" - " int* ptr = NULL;\n" - " try\n" - " {\n" - " ptr = new int(4);\n" - " }\n" - " catch(...)\n" - " {\n" - " delete ptr;\n" - " throw;\n" - " }\n" - " delete ptr;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "int foo()\n" - "{\n" - " int* a = new int;\n" - " bool doDelete = true;\n" - " if (a != 0)\n" - " {\n" - " doDelete = false;\n" - " delete a;\n" - " }\n" - " if(doDelete)\n" - " delete a;\n" - " return 0;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " while(1) {\n" - " x = new char[100];\n" - " if (y++ > 100)\n" - " break;\n" - " delete[] x;\n" - " }\n" - " delete[] x;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " for (int i = 0; i < 10000; i++) {\n" - " x = new char[100];\n" - " delete[] x;\n" - " }\n" - " delete[] x;\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " while (isRunning()) {\n" - " x = new char[100];\n" - " delete[] x;\n" - " }\n" - " delete[] x;\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " while (isRunning()) {\n" - " x = malloc(100);\n" - " free(x);\n" - " }\n" - " free(x);\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'x' is freed twice.\n", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " for (;;) {\n" - " x = new char[100];\n" - " if (y++ > 100)\n" - " break;\n" - " delete[] x;\n" - " }\n" - " delete[] x;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "void foo(int y)\n" - "{\n" - " char * x = NULL;\n" - " do {\n" - " x = new char[100];\n" - " if (y++ > 100)\n" - " break;\n" - " delete[] x;\n" - " } while (1);\n" - " delete[] x;\n" - "}" - ); - ASSERT_EQUALS("", errout.str()); - - check( - "void f()\n" - "{\n" - " char *p = 0;\n" - " if (x < 100) {\n" - " p = malloc(10);\n" - " free(p);\n" - " }\n" - " free(p);\n" - "}" - ); - ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str()); - } - - void check_redundant_copy(const char code[]) { - // Clear the error buffer.. - errout.str(""); - - Settings settings; - settings.addEnabled("performance"); - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - - // Simplify token list.. - CheckOther checkOther(&tokenizer, &settings, this); - tokenizer.simplifyTokenList(); - checkOther.checkRedundantCopy(); - } - void checkRedundantCopy() { - check_redundant_copy("class A{public:A(){}};\n" - "const A& getA(){static A a;return a;}\n" - "int main()\n" - "{\n" - " const A a = getA();\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:5]: (performance) Use const reference for a to avoid unnecessary data copying.\n", errout.str()); - - check_redundant_copy("const int& getA(){static int a;return a;}\n" - "int main()\n" - "{\n" - " const int a = getA();\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check_redundant_copy("const int& getA(){static int a;return a;}\n" - "int main()\n" - "{\n" - " int getA = 0;\n" - " const int a = getA + 3;\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check_redundant_copy("class A{public:A(){}};\n" - "const A& getA(){static A a;return a;}\n" - "int main()\n" - "{\n" - " const A a(getA());\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:5]: (performance) Use const reference for a to avoid unnecessary data copying.\n", errout.str()); - - check_redundant_copy("const int& getA(){static int a;return a;}\n" - "int main()\n" - "{\n" - " const int a(getA());\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check_redundant_copy("class A{\n" - "public:A(int a=0){_a = a;}\n" - "A operator+(const A & a){return A(_a+a._a);}\n" - "private:int _a;};\n" - "const A& getA(){static A a;return a;}\n" - "int main()\n" - "{\n" - " const A a = getA() + 1;\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check_redundant_copy("class A{\n" - "public:A(int a=0){_a = a;}\n" - "A operator+(const A & a){return A(_a+a._a);}\n" - "private:int _a;};\n" - "const A& getA(){static A a;return a;}\n" - "int main()\n" - "{\n" - " const A a(getA()+1);\n" - " return 0;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - - void checkNegativeShift() { - check("void foo()\n" - "{\n" - " int a = 123;\n" - " a << -1;\n" + + void incompleteArrayFill() { + check("void f() {\n" + " int a[5];\n" + " memset(a, 123, 5);\n" + " memcpy(a, b, 5);\n" + " memmove(a, b, 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value.\n", errout.str()); - check("void foo()\n" - "{\n" - " int a = 123;\n" - " int i = -1;\n" - " a << i;\n" + ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n" + "[test.cpp:4]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memcpy()' with 'sizeof(*a)'?\n" + "[test.cpp:5]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memmove()' with 'sizeof(*a)'al (<, >, <= or >=) operator.\n", errout.str()); + + check("void Foo* a[5];\n" + " memset(a, 'a', 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Shifting by a negative value.\n", errout.str()); - check("void foo()\n" - "{\n" - " int a = 123;\n" - " a >> -1;\n" + ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n", errout.str()); + + check("class Foo {int a; int b;};\n" + "void f() {\n" + " Foo a[5];\n" + " memset(a, 'a', 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value.\n", errout.str()); - check("void foo()\n" - "{\n" - " int a = 123;\n" - " int i = -1;\n" - " a >> i;\n" + TODO_ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n", 1() { + check("void f(int x) {\n" + " if ((x &&Foo a[5];\n" // Size of foo is unknown + " memset(a, 'a', 5) check("void f(int x) {\n" + " if (x < 1 && x > 1 + check("void f() {\n" + " char a[5];\n" + " memset(a, 'a', 5) check("void f(int x) {\n" + " if (x < 1 && x > 1 + check("void f() {\n" + " int a[5];\n" + " memset(a+15, 'a', 5) check("void f(int x) {\n" + " if (x < 1 && x > 1 + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5*sizeof(bool)) check("void f(int x) {\n" + " if (x < 1 && x > 1 + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5*sizeof(*a)) check("void f(int x) {\n" + " if (x < 1 && x > 1 + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5);\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Shifting by a negative value.\n", "", errout.str()); - check("void foo()\n" - "{\n" - " int a = 123;\n" - " a <<= -1;\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value.\n", errout.str()); - check("void foo()\n" - "{\n" - " int a = 123;\n" - " a >>= -1;\n" - "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (portability, inconclusive) Array 'a' might be filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n", errout.str()); } };