From 1b00a0f06ab751ce0601399a841393c23900fb4c Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Thu, 6 Apr 2023 18:44:03 +0200 Subject: [PATCH] Fix #9279 Missing --check-library warning when memory leaks check assumes function is noreturn (#4937) * Fix #9279 Missing --check-library warning when memory leaks check assumes function is noreturn * Format * Fix check, add tests * Format --- lib/checkleakautovar.cpp | 38 +++++++++++--------- lib/checkleakautovar.h | 9 ++--- test/testleakautovar.cpp | 77 ++++++++++++++++++++++++++++++---------- 3 files changed, 85 insertions(+), 39 deletions(-) diff --git a/lib/checkleakautovar.cpp b/lib/checkleakautovar.cpp index 0fe3daf8c..743e82c75 100644 --- a/lib/checkleakautovar.cpp +++ b/lib/checkleakautovar.cpp @@ -96,10 +96,9 @@ void VarInfo::print() std::cout << "size=" << alloctype.size() << std::endl; for (std::map::const_iterator it = alloctype.cbegin(); it != alloctype.cend(); ++it) { std::string strusage; - const std::map::const_iterator use = - possibleUsage.find(it->first); + const auto use = possibleUsage.find(it->first); if (use != possibleUsage.end()) - strusage = use->second; + strusage = use->second.first; std::string status; switch (it->second.status) { @@ -133,11 +132,11 @@ void VarInfo::print() } } -void VarInfo::possibleUsageAll(const std::string &functionName) +void VarInfo::possibleUsageAll(const std::pair& functionUsage) { possibleUsage.clear(); for (std::map::const_iterator it = alloctype.cbegin(); it != alloctype.cend(); ++it) - possibleUsage[it->first] = functionName; + possibleUsage[it->first] = functionUsage; } @@ -169,13 +168,13 @@ void CheckLeakAutoVar::deallocReturnError(const Token *tok, const Token *dealloc reportError(locations, Severity::error, "deallocret", "$symbol:" + varname + "\nReturning/dereferencing '$symbol' after it is deallocated / released", CWE672, Certainty::normal); } -void CheckLeakAutoVar::configurationInfo(const Token* tok, const std::string &functionName) +void CheckLeakAutoVar::configurationInfo(const Token* tok, const std::pair& functionUsage) { - if (mSettings->checkLibrary) { + if (mSettings->checkLibrary && functionUsage.second == VarInfo::USED) { reportError(tok, Severity::information, "checkLibraryUseIgnore", - "--check-library: Function " + functionName + "() should have / configuration"); + "--check-library: Function " + functionUsage.first + "() should have / configuration"); } } @@ -300,7 +299,7 @@ bool CheckLeakAutoVar::checkScope(const Token * const startToken, throw InternalError(startToken, "Internal limit: CheckLeakAutoVar::checkScope() Maximum recursive count of 1000 reached.", InternalError::LIMIT); std::map &alloctype = varInfo.alloctype; - std::map &possibleUsage = varInfo.possibleUsage; + auto& possibleUsage = varInfo.possibleUsage; const std::set conditionalAlloc(varInfo.conditionalAlloc); // Parse all tokens @@ -671,8 +670,15 @@ bool CheckLeakAutoVar::checkScope(const Token * const startToken, if (mTokenizer->isScopeNoReturn(tok->tokAt(2), &unknown)) { if (!unknown) varInfo.clear(); - else if (!mSettings->library.isLeakIgnore(functionName) && !mSettings->library.isUse(functionName)) - varInfo.possibleUsageAll(functionName); + else { + if (ftok->function() && !ftok->function()->isAttributeNoreturn() && + !(ftok->function()->functionScope && mTokenizer->isScopeNoReturn(ftok->function()->functionScope->bodyEnd))) // check function scope + continue; + if (!mSettings->library.isLeakIgnore(functionName) && !mSettings->library.isUse(functionName)) { + const VarInfo::Usage usage = Token::simpleMatch(openingPar, "( )") ? VarInfo::NORET : VarInfo::USED; // TODO: check parameters passed to function + varInfo.possibleUsageAll({ functionName, usage }); + } + } } } @@ -870,7 +876,7 @@ void CheckLeakAutoVar::changeAllocStatus(VarInfo &varInfo, const VarInfo::AllocI if (var != alloctype.end()) { if (allocation.status == VarInfo::NOALLOC) { // possible usage - varInfo.possibleUsage[arg->varId()] = tok->str(); + varInfo.possibleUsage[arg->varId()] = { tok->str(), VarInfo::USED }; if (var->second.status == VarInfo::DEALLOC && arg->previous()->str() == "&") varInfo.erase(arg->varId()); } else if (var->second.managed()) { @@ -1006,11 +1012,11 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok, const VarInfo &varInfo) { const std::map &alloctype = varInfo.alloctype; - const std::map &possibleUsage = varInfo.possibleUsage; + const auto& possibleUsage = varInfo.possibleUsage; const std::map::const_iterator var = alloctype.find(vartok->varId()); if (var != alloctype.cend() && var->second.status == VarInfo::ALLOC) { - const std::map::const_iterator use = possibleUsage.find(vartok->varId()); + const auto use = possibleUsage.find(vartok->varId()); if (use == possibleUsage.end()) { leakError(vartok, vartok->str(), var->second.type); } else { @@ -1022,7 +1028,7 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok, void CheckLeakAutoVar::ret(const Token *tok, VarInfo &varInfo, const bool isEndOfScope) { const std::map &alloctype = varInfo.alloctype; - const std::map &possibleUsage = varInfo.possibleUsage; + const auto& possibleUsage = varInfo.possibleUsage; std::vector toRemove; const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); @@ -1071,7 +1077,7 @@ void CheckLeakAutoVar::ret(const Token *tok, VarInfo &varInfo, const bool isEndO deallocReturnError(tok, it->second.allocTok, var->name()); else if (!used && !it->second.managed()) { - const std::map::const_iterator use = possibleUsage.find(varid); + const auto use = possibleUsage.find(varid); if (use == possibleUsage.end()) { leakError(tok, var->name(), it->second.type); } else { diff --git a/lib/checkleakautovar.h b/lib/checkleakautovar.h index 217e6a331..1d7cb5fb3 100644 --- a/lib/checkleakautovar.h +++ b/lib/checkleakautovar.h @@ -55,8 +55,9 @@ public: return status < 0; } }; + enum Usage { USED, NORET }; std::map alloctype; - std::map possibleUsage; + std::map> possibleUsage; std::set conditionalAlloc; std::set referenced; @@ -92,7 +93,7 @@ public: } /** set possible usage for all variables */ - void possibleUsageAll(const std::string &functionName); + void possibleUsageAll(const std::pair& functionUsage); void print(); }; @@ -159,12 +160,12 @@ private: void doubleFreeError(const Token *tok, const Token *prevFreeTok, const std::string &varname, int type); /** message: user configuration is needed to complete analysis */ - void configurationInfo(const Token* tok, const std::string &functionName); + void configurationInfo(const Token* tok, const std::pair& functionUsage); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { CheckLeakAutoVar c(nullptr, settings, errorLogger); c.deallocReturnError(nullptr, nullptr, "p"); - c.configurationInfo(nullptr, "f"); // user configuration is needed to complete analysis + c.configurationInfo(nullptr, { "f", VarInfo::USED }); // user configuration is needed to complete analysis c.doubleFreeError(nullptr, nullptr, "varname", 0); } diff --git a/test/testleakautovar.cpp b/test/testleakautovar.cpp index 8fbc36ffb..c7296539b 100644 --- a/test/testleakautovar.cpp +++ b/test/testleakautovar.cpp @@ -104,7 +104,6 @@ private: TEST_CASE(freopen2); TEST_CASE(deallocuse1); - TEST_CASE(deallocuse2); TEST_CASE(deallocuse3); TEST_CASE(deallocuse4); TEST_CASE(deallocuse5); // #4018: FP. free(p), p = 0; @@ -508,8 +507,8 @@ private: settings = s; } - void assign24() { // #7440 - check("void f() {\n" + void assign24() { + check("void f() {\n" // #7440 " char* data = new char[100];\n" " char** dataPtr = &data;\n" " delete[] *dataPtr;\n" @@ -523,6 +522,46 @@ private: " delete[] *dataPtr;\n" "}\n", true); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" // #9279 + " int* p = new int;\n" + " *p = 42;\n" + " g();\n" + "}\n", /*cpp*/ true); + ASSERT_EQUALS("[test.cpp:4]: (information) --check-library: Function g() should have configuration\n", + errout.str()); + + check("void g();\n" + "void f() {\n" + " int* p = new int;\n" + " *p = 42;\n" + " g();\n" + "}\n", /*cpp*/ true); + ASSERT_EQUALS("[test.cpp:6]: (error) Memory leak: p\n", errout.str()); + + check("void g() {}\n" + "void f() {\n" + " int* p = new int;\n" + " *p = 42;\n" + " g();\n" + "}\n", /*cpp*/ true); + ASSERT_EQUALS("[test.cpp:6]: (error) Memory leak: p\n", errout.str()); + + check("[[noreturn]] void g();\n" + "void f() {\n" + " int* p = new int;\n" + " *p = 42;\n" + " g();\n" + "}\n", /*cpp*/ true); + ASSERT_EQUALS("", errout.str()); + + check("void g() { exit(1); }\n" + "void f() {\n" + " int* p = new int;\n" + " *p = 42;\n" + " g();\n" + "}\n", /*cpp*/ true); + ASSERT_EQUALS("", errout.str()); } void isAutoDealloc() { @@ -647,20 +686,6 @@ private: ASSERT_EQUALS("[test.c:3]: (error) Dereferencing 'p' after it is deallocated / released\n", errout.str()); } - void deallocuse2() { - check("void f(char *p) {\n" - " free(p);\n" - " strcpy(a, p);\n" - "}"); - TODO_ASSERT_EQUALS("error (free,use)", "[test.c:3]: (information) --check-library: Function strcpy() should have configuration\n", errout.str()); - - check("void f(char *p) {\n" // #3041 - assigning pointer when it's used - " free(p);\n" - " strcpy(a, p=b());\n" - "}"); - TODO_ASSERT_EQUALS("", "[test.c:3]: (information) --check-library: Function strcpy() should have configuration\n", errout.str()); - } - void deallocuse3() { check("void f(struct str *p) {\n" " free(p);\n" @@ -1437,8 +1462,7 @@ private: " char *p = malloc(10);\n" " fatal_error();\n" "}"); - ASSERT_EQUALS("[test.c:3]: (information) --check-library: Function fatal_error() should have configuration\n" - "[test.c:4]: (information) --check-library: Function fatal_error() should have / configuration\n", + ASSERT_EQUALS("[test.c:3]: (information) --check-library: Function fatal_error() should have configuration\n", errout.str()); } @@ -2711,6 +2735,7 @@ private: LOAD_LIB_2(settings.library, "std.cfg"); TEST_CASE(returnedValue); // #9298 + TEST_CASE(deallocuse2); TEST_CASE(fclose_false_positive); // #9575 } @@ -2724,6 +2749,20 @@ private: ASSERT_EQUALS("", errout.str()); } + void deallocuse2() { + check("void f(char *p) {\n" + " free(p);\n" + " strcpy(a, p);\n" + "}"); + TODO_ASSERT_EQUALS("error (free,use)", "", errout.str()); + + check("void f(char *p) {\n" // #3041 - assigning pointer when it's used + " free(p);\n" + " strcpy(a, p=b());\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + void fclose_false_positive() { // #9575 check("int f(FILE *fp) { return fclose(fp); }"); ASSERT_EQUALS("", errout.str());