New check (Inconclusive): Array filled incompletely with memset/memcpy/memmove.

This check only warns if the number of elements is given as size in bytes to memset, memcpy or memmove and if the size of an element is larger than 1 Byte. It does not warn for random numbers
This commit is contained in:
PKEuS 2012-08-23 11:27:00 -07:00
parent d24badbfda
commit 674f7980d5
3 changed files with 92 additions and 457 deletions

View File

@ -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);
}

View File

@ -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<const Function*> &constFunctions,
const Token *start,
const Token *end,
const std::string &toCheck);
void complexDuplicateExpressionCheck(const std::list<const Function*> &constFunctions,
const Token *classStart,
const std::string &toCheck,
const std::string &alt);
};
/// @}
//---------------------------------------------------------------------------
#endif

View File

@ -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 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)'al (<, >, <= or >=) operator.\n", errout.str());
check("void 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"
" char *p = malloc(100);\n"
" if (x) {\n"
" free(p);\n"
" exit();\n"
" }\n"
" free(p);\n"
" Foo a[5];\n"
" memset(a, 'a', 5);\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"
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"
"}");
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"
"}");
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: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"
" int i = -1;\n"
" a >> i;\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());
}
};