From c3eb37972d59fc2eb0f3d4ca8b5b7461640f5712 Mon Sep 17 00:00:00 2001 From: Rikard Falkeborn Date: Fri, 26 Feb 2021 12:58:52 +0100 Subject: [PATCH] Fix #10182 (FN memory leak with if-statement) (#3151) Improve leak detections in if-statements. This is done by checking for leaks every time a scope is left. This allows cppcheck to catch more memory leaks, as well as improve some error messages which now contain the line where the variable goes out of scope, instead of the end of the function. --- lib/checkleakautovar.cpp | 30 +++++++++++------------------- lib/checkleakautovar.h | 2 +- test/testleakautovar.cpp | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/lib/checkleakautovar.cpp b/lib/checkleakautovar.cpp index 6976eef68..3da2047b6 100644 --- a/lib/checkleakautovar.cpp +++ b/lib/checkleakautovar.cpp @@ -195,22 +195,6 @@ void CheckLeakAutoVar::check() VarInfo varInfo; checkScope(scope->bodyStart, &varInfo, notzero, 0); - - varInfo.conditionalAlloc.clear(); - - // Clear reference arguments from varInfo.. - std::map::iterator it = varInfo.alloctype.begin(); - while (it != varInfo.alloctype.end()) { - const Variable *var = symbolDatabase->getVariableFromVarId(it->first); - if (!var || - (var->isArgument() && var->isReference()) || - (!var->isArgument() && !var->isLocal())) - varInfo.alloctype.erase(it++); - else - ++it; - } - - ret(scope->bodyEnd, varInfo); } } @@ -741,6 +725,7 @@ void CheckLeakAutoVar::checkScope(const Token * const startToken, changeAllocStatus(varInfo, allocation, vtok, vtok); } } + ret(endToken, *varInfo, true); } @@ -973,15 +958,16 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok, } } -void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo) +void CheckLeakAutoVar::ret(const Token *tok, VarInfo &varInfo, const bool isEndOfScope) { const std::map &alloctype = varInfo.alloctype; const std::map &possibleUsage = varInfo.possibleUsage; + std::vector toRemove; const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (std::map::const_iterator it = alloctype.begin(); it != alloctype.end(); ++it) { - // don't warn if variable is conditionally allocated - if (!it->second.managed() && varInfo.conditionalAlloc.find(it->first) != varInfo.conditionalAlloc.end()) + // don't warn if variable is conditionally allocated, unless it leaves the scope + if (!isEndOfScope && !it->second.managed() && varInfo.conditionalAlloc.find(it->first) != varInfo.conditionalAlloc.end()) continue; // don't warn if there is a reference of the variable @@ -991,6 +977,9 @@ void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo) const int varid = it->first; const Variable *var = symbolDatabase->getVariableFromVarId(varid); if (var) { + // don't warn if we leave an inner scope + if (isEndOfScope && var->scope() && tok != var->scope()->bodyEnd) + continue; bool used = false; for (const Token *tok2 = tok; tok2; tok2 = tok2->next()) { if (tok2->str() == ";") @@ -1028,6 +1017,9 @@ void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo) configurationInfo(tok, use->second); } } + toRemove.push_back(varid); } } + for (int varId : toRemove) + varInfo.erase(varId); } diff --git a/lib/checkleakautovar.h b/lib/checkleakautovar.h index 83f75853f..205b2ab70 100644 --- a/lib/checkleakautovar.h +++ b/lib/checkleakautovar.h @@ -149,7 +149,7 @@ private: void changeAllocStatusIfRealloc(std::map &alloctype, const Token *fTok, const Token *retTok); /** return. either "return" or end of variable scope is seen */ - void ret(const Token *tok, const VarInfo &varInfo); + void ret(const Token *tok, VarInfo &varInfo, const bool isEndOfScope = false); /** if variable is allocated then there is a leak */ void leakIfAllocated(const Token *vartok, const VarInfo &varInfo); diff --git a/test/testleakautovar.cpp b/test/testleakautovar.cpp index 181876c19..421bd664b 100644 --- a/test/testleakautovar.cpp +++ b/test/testleakautovar.cpp @@ -145,6 +145,8 @@ private: TEST_CASE(ifelse17); // if (!!(!p)) TEST_CASE(ifelse18); TEST_CASE(ifelse19); + TEST_CASE(ifelse20); // #10182 + TEST_CASE(ifelse21); // switch TEST_CASE(switch1); @@ -1611,6 +1613,39 @@ private: ASSERT_EQUALS("", errout.str()); } + void ifelse20() { + check("void f() {\n" + " if (x > 0)\n" + " void * p1 = malloc(5);\n" + " else\n" + " void * p2 = malloc(2);\n" + " return;\n" + "}"); + ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p1\n" + "[test.c:5]: (error) Memory leak: p2\n", errout.str()); + + check("void f() {\n" + " if (x > 0)\n" + " void * p1 = malloc(5);\n" + " else\n" + " void * p2 = malloc(2);\n" + "}"); + ASSERT_EQUALS("[test.c:3]: (error) Memory leak: p1\n" + "[test.c:5]: (error) Memory leak: p2\n", errout.str()); + } + + void ifelse21() { + check("void f() {\n" + " if (y) {\n" + " void * p;\n" + " if (x > 0)\n" + " p = malloc(5);\n" + " }\n" + " return;\n" + "}"); + ASSERT_EQUALS("[test.c:6]: (error) Memory leak: p\n", errout.str()); + } + void switch1() { check("void f() {\n" " char *p = 0;\n"