diff --git a/lib/checkother.cpp b/lib/checkother.cpp index fa0b228f3..a0130aa9c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -229,6 +229,41 @@ void CheckOther::clarifyConditionError(const Token *tok, bool assign, bool boolo errmsg); } +//--------------------------------------------------------------------------- +// Clarify (meaningless) statements like *foo++; with parantheses. +//--------------------------------------------------------------------------- +void CheckOther::clarifyStatement() +{ + if (!_settings->isEnabled("style")) + return; + + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "[{};] * %var%")) { + tok = tok->tokAt(3); + while (tok) { + if (tok->str() == "[") + tok = tok->link()->next(); + if (Token::Match(tok, ".|:: %var%")) { + if (tok->strAt(2) == "(") + tok = tok->linkAt(2)->next(); + else + tok = tok->tokAt(2); + } else + break; + } + if (Token::Match(tok, "++|-- [;,]")) + clarifyStatementError(tok); + } + } +} + +void CheckOther::clarifyStatementError(const Token *tok) +{ + reportError(tok, Severity::warning, "clarifyStatement", "Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n" + "A statement like '*A++;' might not do what you intended. 'operator*' is executed before postfix 'operator++'. " + "Thus, the dereference is meaningless. Did you intend to write '(*A)++;'?"); +} + //--------------------------------------------------------------------------- // if (bool & bool) -> if (bool && bool) // if (bool | bool) -> if (bool || bool) @@ -3116,3 +3151,43 @@ void CheckOther::negativeBitwiseShiftError(const Token *tok) { reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value."); } + + +//--------------------------------------------------------------------------- +// Check for incompletely filled buffers. +//--------------------------------------------------------------------------- +void CheckOther::checkIncompleteArrayFill() +{ + if (!_settings->inconclusive || !_settings->isEnabled("style")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + 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; + + 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::incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean) +{ + 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..005a83d5a 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 */ @@ -82,6 +83,7 @@ public: // Checks checkOther.clarifyCalculation(); + checkOther.clarifyStatement(); checkOther.checkConstantFunctionParameter(); checkOther.checkIncompleteStatement(); @@ -113,6 +115,9 @@ public: /** @brief Suspicious condition (assignment+comparison) */ void clarifyCondition(); + /** @brief Suspicious statement like '*A++;' */ + void clarifyStatement(); + /** @brief Are there C-style pointer casts in a c++ file? */ void warningOldStylePointerCast(); @@ -244,10 +249,14 @@ public: /** @brief %Check for bitwise operation with negative right operand */ void checkNegativeBitwiseShift(); + /** @brief %Check for buffers that are filled incompletely with memset and similar functions */ + void checkIncompleteArrayFill(); + private: // Error messages.. void clarifyCalculationError(const Token *tok, const std::string &op); void clarifyConditionError(const Token *tok, bool assign, bool boolop); + void clarifyStatementError(const Token* tok); void sizeofsizeofError(const Token *tok); void sizeofCalculationError(const Token *tok, bool inconclusive); void cstyleCastError(const Token *tok); @@ -302,6 +311,7 @@ private: void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal); void negativeBitwiseShiftError(const Token *tok); void redundantCopyError(const Token *tok, const std::string &varname); + void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(0, settings, errorLogger); @@ -344,6 +354,7 @@ private: c.memsetZeroBytesError(0, "varname"); c.clarifyCalculationError(0, "+"); c.clarifyConditionError(0, true, false); + c.clarifyStatementError(0); c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12"); c.incorrectStringBooleanError(0, "\"Hello World\""); c.incrementBooleanError(0); @@ -364,6 +375,7 @@ private: c.SuspiciousSemicolonError(0); c.cctypefunctionCallError(0, "funname", "value"); c.moduloAlwaysTrueFalseError(0, "1"); + c.incompleteArrayFillError(0, "buffer", "memset", false); } static std::string myName() { @@ -422,7 +434,8 @@ private: "* using bool in bitwise expression\n" "* Suspicious use of ; at the end of 'if/for/while' statement.\n" "* incorrect usage of functions from ctype library.\n" - "* Comparisons of modulo results that are always true/false.\n"; + "* Comparisons of modulo results that are always true/false.\n" + "* Array filled incompletely using memset/memcpy/memmove.\n"; } void checkExpressionRange(const std::list &constFunctions, diff --git a/test/testother.cpp b/test/testother.cpp index 8b797f9b1..67bafa0a9 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -119,6 +119,7 @@ private: TEST_CASE(sizeofForNumericParameter); TEST_CASE(clarifyCalculation); + TEST_CASE(clarifyStatement); TEST_CASE(clarifyCondition1); // if (a = b() < 0) TEST_CASE(clarifyCondition2); // if (a & b == c) @@ -164,6 +165,8 @@ private: TEST_CASE(checkRedundantCopy); TEST_CASE(checkNegativeShift); + + TEST_CASE(incompleteArrayFill); } void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) { @@ -172,6 +175,7 @@ private: Settings settings; settings.addEnabled("style"); + settings.addEnabled("portability"); settings.inconclusive = inconclusive; settings.experimental = experimental; @@ -3942,6 +3946,46 @@ private: ASSERT_EQUALS("", errout.str()); } + void clarifyStatement() { + check("char* f(char* c) {\n" + " *c++;\n" + " return c;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("char* f(char** c) {\n" + " *c[5]--;\n" + " return *c;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a[5].v[3]++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a(1, 5).v[x + y]++;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("char* f(char* c) {\n" + " (*c)++;\n" + " return c;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(char* c) {\n" + " bar(*c++);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + // clarify conditions with = and comparison void clarifyCondition1() { check("void f() {\n" @@ -5837,6 +5881,67 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4]: (error) Shifting by a negative value.\n", errout.str()); } + + 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: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)'?\n", errout.str()); + + check("void f() {\n" + " Foo* a[5];\n" + " memset(a, 'a', 5);\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" + "}"); + 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", "", errout.str()); + + check("void f() {\n" + " Foo a[5];\n" // Size of foo is unknown + " memset(a, 'a', 5);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " char a[5];\n" + " memset(a, 'a', 5);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int a[5];\n" + " memset(a+15, 'a', 5);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5*sizeof(bool));\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5*sizeof(*a));\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " bool a[5];\n" + " memset(a, false, 5);\n" + "}"); + 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()); + } }; REGISTER_TEST(TestOther)