walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

CheckOther::checkInvalidFree: Move check to normal checking. And clarify the message.

This commit is contained in:
Daniel Marjamäki 2019-03-07 06:35:11 +01:00
parent 267074964d
commit 6eaf2c03d9
3 changed files with 38 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
lib/checkother.cpp
View File

@ -1677,7 +1677,8 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2, Erro
//----------------------------------------------------------------------------- //-----------------------------------------------------------------------------
void CheckOther::checkInvalidFree() void CheckOther::checkInvalidFree()
{ {
std::map<unsigned int, bool> allocatedVariables; std::map<unsigned int, bool> inconclusive;
std::map<unsigned int, std::string> allocation;
const bool printInconclusive = mSettings->inconclusive; const bool printInconclusive = mSettings->inconclusive;
const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase(); const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase();
@ -1686,7 +1687,8 @@ void CheckOther::checkInvalidFree()
// Keep track of which variables were assigned addresses to newly-allocated memory // Keep track of which variables were assigned addresses to newly-allocated memory
if (Token::Match(tok, "%var% = malloc|g_malloc|new")) { if (Token::Match(tok, "%var% = malloc|g_malloc|new")) {
allocatedVariables.insert(std::make_pair(tok->varId(), false)); allocation.insert(std::make_pair(tok->varId(), tok->strAt(2)));
inconclusive.insert(std::make_pair(tok->varId(), false));
} }
// If a previously-allocated pointer is incremented or decremented, any subsequent // If a previously-allocated pointer is incremented or decremented, any subsequent
@ -1694,17 +1696,20 @@ void CheckOther::checkInvalidFree()
// report an inconclusive result. // report an inconclusive result.
else if (Token::Match(tok, "%var% = %name% +|-") && else if (Token::Match(tok, "%var% = %name% +|-") &&
tok->varId() == tok->tokAt(2)->varId() && tok->varId() == tok->tokAt(2)->varId() &&
allocatedVariables.find(tok->varId()) != allocatedVariables.end()) { allocation.find(tok->varId()) != allocation.end()) {
if (printInconclusive) if (printInconclusive)
allocatedVariables[tok->varId()] = true; inconclusive[tok->varId()] = true;
else else {
allocatedVariables.erase(tok->varId()); allocation.erase(tok->varId());
inconclusive.erase(tok->varId());
}
} }
// If a previously-allocated pointer is assigned a completely new value, // If a previously-allocated pointer is assigned a completely new value,
// we can't know if any subsequent free() on that pointer is valid or not. // we can't know if any subsequent free() on that pointer is valid or not.
else if (Token::Match(tok, "%var% =")) { else if (Token::Match(tok, "%var% =")) {
allocatedVariables.erase(tok->varId()); allocation.erase(tok->varId());
inconclusive.erase(tok->varId());
} }
// If a variable that was previously assigned a newly-allocated memory location is // If a variable that was previously assigned a newly-allocated memory location is
@ -1717,12 +1722,12 @@ void CheckOther::checkInvalidFree()
tok->strAt(3) == "(" ? 4 : 1; tok->strAt(3) == "(" ? 4 : 1;
const unsigned int var1 = tok->tokAt(varIndex)->varId(); const unsigned int var1 = tok->tokAt(varIndex)->varId();
const unsigned int var2 = tok->tokAt(varIndex + 2)->varId(); const unsigned int var2 = tok->tokAt(varIndex + 2)->varId();
const std::map<unsigned int, bool>::const_iterator alloc1 = allocatedVariables.find(var1); const std::map<unsigned int, bool>::const_iterator alloc1 = inconclusive.find(var1);
const std::map<unsigned int, bool>::const_iterator alloc2 = allocatedVariables.find(var2); const std::map<unsigned int, bool>::const_iterator alloc2 = inconclusive.find(var2);
if (alloc1 != allocatedVariables.end()) { if (alloc1 != inconclusive.end()) {
invalidFreeError(tok, alloc1->second); invalidFreeError(tok, allocation[var1], alloc1->second);
} else if (alloc2 != allocatedVariables.end()) { } else if (alloc2 != inconclusive.end()) {
invalidFreeError(tok, alloc2->second); invalidFreeError(tok, allocation[var2], alloc2->second);
} }
} }
@ -1732,7 +1737,8 @@ void CheckOther::checkInvalidFree()
else if (Token::Match(tok, "%name% (") && !mSettings->library.isFunctionConst(tok->str(), true)) { else if (Token::Match(tok, "%name% (") && !mSettings->library.isFunctionConst(tok->str(), true)) {
const Token* tok2 = Token::findmatch(tok->next(), "%var%", tok->linkAt(1)); const Token* tok2 = Token::findmatch(tok->next(), "%var%", tok->linkAt(1));
while (tok2 != nullptr) { while (tok2 != nullptr) {
allocatedVariables.erase(tok2->varId()); allocation.erase(tok->varId());
inconclusive.erase(tok2->varId());
tok2 = Token::findmatch(tok2->next(), "%var%", tok->linkAt(1)); tok2 = Token::findmatch(tok2->next(), "%var%", tok->linkAt(1));
} }
} }
@ -1740,9 +1746,13 @@ void CheckOther::checkInvalidFree()
} }
} }
void CheckOther::invalidFreeError(const Token *tok, bool inconclusive) void CheckOther::invalidFreeError(const Token *tok, const std::string &allocation, bool inconclusive)
{ {
reportError(tok, Severity::error, "invalidFree", "Invalid memory address freed.", CWE(0U), inconclusive); std::string alloc = allocation;
if (alloc != "new")
alloc += "()";
std::string deallocated = (alloc == "new") ? "deleted" : "freed";
reportError(tok, Severity::error, "invalidFree", "Mismatching address is " + deallocated + ". The address you get from " + alloc + " must be " + deallocated + " without offset.", CWE(0U), inconclusive);
} }

6
lib/checkother.h
View File

@ -90,6 +90,7 @@ public:
checkOther.clarifyCalculation(); checkOther.clarifyCalculation();
checkOther.checkPassByReference(); checkOther.checkPassByReference();
checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse();
checkOther.checkInvalidFree();
} }
/** @brief Run checks against the simplified token list */ /** @brief Run checks against the simplified token list */
@ -102,7 +103,6 @@ public:
checkOther.checkMisusedScopedObject(); checkOther.checkMisusedScopedObject();
checkOther.checkInvalidFree();
checkOther.checkAccessOfMovedVariable(); checkOther.checkAccessOfMovedVariable();
} }
@ -169,7 +169,7 @@ public:
/** @brief %Check for free() operations on invalid memory locations */ /** @brief %Check for free() operations on invalid memory locations */
void checkInvalidFree(); void checkInvalidFree();
void invalidFreeError(const Token *tok, bool inconclusive); void invalidFreeError(const Token *tok, const std::string &allocation, bool inconclusive);
/** @brief %Check for code creating redundant copies */ /** @brief %Check for code creating redundant copies */
void checkRedundantCopy(); void checkRedundantCopy();
@ -285,7 +285,7 @@ private:
c.negativeBitwiseShiftError(nullptr, 2); c.negativeBitwiseShiftError(nullptr, 2);
c.checkPipeParameterSizeError(nullptr, "varname", "dimension"); c.checkPipeParameterSizeError(nullptr, "varname", "dimension");
c.raceAfterInterlockedDecrementError(nullptr); c.raceAfterInterlockedDecrementError(nullptr);
c.invalidFreeError(nullptr, false); c.invalidFreeError(nullptr, "malloc", false);
//performance //performance
c.redundantCopyError(nullptr, "varname"); c.redundantCopyError(nullptr, "varname");

18
test/testother.cpp
View File

@ -5287,31 +5287,31 @@ private:
" char *a; a = malloc(1024);\n" " char *a; a = malloc(1024);\n"
" free(a + 10);\n" " free(a + 10);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching address is freed. The address you get from malloc() must be freed without offset.\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = malloc(1024);\n" " char *a; a = malloc(1024);\n"
" free(a - 10);\n" " free(a - 10);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching address is freed. The address you get from malloc() must be freed without offset.\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = malloc(1024);\n" " char *a; a = malloc(1024);\n"
" free(10 + a);\n" " free(10 + a);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching address is freed. The address you get from malloc() must be freed without offset.\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = new char[1024];\n" " char *a; a = new char[1024];\n"
" delete[] (a + 10);\n" " delete[] (a + 10);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching address is deleted. The address you get from new must be deleted without offset..\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = new char;\n" " char *a; a = new char;\n"
" delete a + 10;\n" " delete a + 10;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching address is deleted. The address you get from new must be deleted without offset.\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = new char;\n" " char *a; a = new char;\n"
@ -5327,7 +5327,7 @@ private:
" delete a + 10;\n" " delete a + 10;\n"
" delete b + 10;\n" " delete b + 10;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:6]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:6]: (error) Mismatching address is deleted. The address you get from new must be deleted without offset.\n", errout.str());
check("void foo(char *p) {\n" check("void foo(char *p) {\n"
" char *a; a = new char;\n" " char *a; a = new char;\n"
@ -5343,14 +5343,14 @@ private:
" bar()\n" " bar()\n"
" delete a + 10;\n" " delete a + 10;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:4]: (error) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:4]: (error) Mismatching address is deleted. The address you get from new must be deleted without offset.\n", errout.str());
check("void foo(size_t xx) {\n" check("void foo(size_t xx) {\n"
" char *ptr; ptr = malloc(42);\n" " char *ptr; ptr = malloc(42);\n"
" ptr += xx;\n" " ptr += xx;\n"
" free(ptr - xx - 1);\n" " free(ptr + 1 - xx);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:4]: (error, inconclusive) Invalid memory address freed.\n", errout.str()); ASSERT_EQUALS("[test.cpp:4]: (error) Mismatching address is freed. The address you get from malloc() must be freed without offset.\n", errout.str());
check("void foo(size_t xx) {\n" check("void foo(size_t xx) {\n"
" char *ptr; ptr = malloc(42);\n" " char *ptr; ptr = malloc(42);\n"