Skip to content

Commit 1b00a0f

Browse files
chrchr-githubchrchr-github
authored
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
1 parent e22a740 commit 1b00a0f

3 files changed

Lines changed: 85 additions & 39 deletions

File tree

‎lib/checkleakautovar.cpp‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,9 @@ void VarInfo::print()
9696
std::cout << "size=" << alloctype.size() << std::endl;
9797
for (std::map<int, AllocInfo>::const_iterator it = alloctype.cbegin(); it != alloctype.cend(); ++it) {
9898
std::string strusage;
99-
const std::map<int, std::string>::const_iterator use =
100-
possibleUsage.find(it->first);
99+
const auto use = possibleUsage.find(it->first);
101100
if (use != possibleUsage.end())
102-
strusage = use->second;
101+
strusage = use->second.first;
103102

104103
std::string status;
105104
switch (it->second.status) {
@@ -133,11 +132,11 @@ void VarInfo::print()
133132
}
134133
}
135134

136-
void VarInfo::possibleUsageAll(const std::string &functionName)
135+
void VarInfo::possibleUsageAll(const std::pair<std::string, Usage>& functionUsage)
137136
{
138137
possibleUsage.clear();
139138
for (std::map<int, AllocInfo>::const_iterator it = alloctype.cbegin(); it != alloctype.cend(); ++it)
140-
possibleUsage[it->first] = functionName;
139+
possibleUsage[it->first] = functionUsage;
141140
}
142141

143142

@@ -169,13 +168,13 @@ void CheckLeakAutoVar::deallocReturnError(const Token *tok, const Token *dealloc
169168
reportError(locations, Severity::error, "deallocret", "$symbol:" + varname + "\nReturning/dereferencing '$symbol' after it is deallocated / released", CWE672, Certainty::normal);
170169
}
171170

172-
void CheckLeakAutoVar::configurationInfo(const Token* tok, const std::string &functionName)
171+
void CheckLeakAutoVar::configurationInfo(const Token* tok, const std::pair<std::string, VarInfo::Usage>& functionUsage)
173172
{
174-
if (mSettings->checkLibrary) {
173+
if (mSettings->checkLibrary && functionUsage.second == VarInfo::USED) {
175174
reportError(tok,
176175
Severity::information,
177176
"checkLibraryUseIgnore",
178-
"--check-library: Function " + functionName + "() should have <use>/<leak-ignore> configuration");
177+
"--check-library: Function " + functionUsage.first + "() should have <use>/<leak-ignore> configuration");
179178
}
180179
}
181180

@@ -300,7 +299,7 @@ bool CheckLeakAutoVar::checkScope(const Token * const startToken,
300299
throw InternalError(startToken, "Internal limit: CheckLeakAutoVar::checkScope() Maximum recursive count of 1000 reached.", InternalError::LIMIT);
301300

302301
std::map<int, VarInfo::AllocInfo> &alloctype = varInfo.alloctype;
303-
std::map<int, std::string> &possibleUsage = varInfo.possibleUsage;
302+
auto& possibleUsage = varInfo.possibleUsage;
304303
const std::set<int> conditionalAlloc(varInfo.conditionalAlloc);
305304

306305
// Parse all tokens
@@ -671,8 +670,15 @@ bool CheckLeakAutoVar::checkScope(const Token * const startToken,
671670
if (mTokenizer->isScopeNoReturn(tok->tokAt(2), &unknown)) {
672671
if (!unknown)
673672
varInfo.clear();
674-
else if (!mSettings->library.isLeakIgnore(functionName) && !mSettings->library.isUse(functionName))
675-
varInfo.possibleUsageAll(functionName);
673+
else {
674+
if (ftok->function() && !ftok->function()->isAttributeNoreturn() &&
675+
!(ftok->function()->functionScope && mTokenizer->isScopeNoReturn(ftok->function()->functionScope->bodyEnd))) // check function scope
676+
continue;
677+
if (!mSettings->library.isLeakIgnore(functionName) && !mSettings->library.isUse(functionName)) {
678+
const VarInfo::Usage usage = Token::simpleMatch(openingPar, "( )") ? VarInfo::NORET : VarInfo::USED; // TODO: check parameters passed to function
679+
varInfo.possibleUsageAll({ functionName, usage });
680+
}
681+
}
676682
}
677683
}
678684

@@ -870,7 +876,7 @@ void CheckLeakAutoVar::changeAllocStatus(VarInfo &varInfo, const VarInfo::AllocI
870876
if (var != alloctype.end()) {
871877
if (allocation.status == VarInfo::NOALLOC) {
872878
// possible usage
873-
varInfo.possibleUsage[arg->varId()] = tok->str();
879+
varInfo.possibleUsage[arg->varId()] = { tok->str(), VarInfo::USED };
874880
if (var->second.status == VarInfo::DEALLOC && arg->previous()->str() == "&")
875881
varInfo.erase(arg->varId());
876882
} else if (var->second.managed()) {
@@ -1006,11 +1012,11 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok,
10061012
const VarInfo &varInfo)
10071013
{
10081014
const std::map<int, VarInfo::AllocInfo> &alloctype = varInfo.alloctype;
1009-
const std::map<int, std::string> &possibleUsage = varInfo.possibleUsage;
1015+
const auto& possibleUsage = varInfo.possibleUsage;
10101016

10111017
const std::map<int, VarInfo::AllocInfo>::const_iterator var = alloctype.find(vartok->varId());
10121018
if (var != alloctype.cend() && var->second.status == VarInfo::ALLOC) {
1013-
const std::map<int, std::string>::const_iterator use = possibleUsage.find(vartok->varId());
1019+
const auto use = possibleUsage.find(vartok->varId());
10141020
if (use == possibleUsage.end()) {
10151021
leakError(vartok, vartok->str(), var->second.type);
10161022
} else {
@@ -1022,7 +1028,7 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok,
10221028
void CheckLeakAutoVar::ret(const Token *tok, VarInfo &varInfo, const bool isEndOfScope)
10231029
{
10241030
const std::map<int, VarInfo::AllocInfo> &alloctype = varInfo.alloctype;
1025-
const std::map<int, std::string> &possibleUsage = varInfo.possibleUsage;
1031+
const auto& possibleUsage = varInfo.possibleUsage;
10261032
std::vector<int> toRemove;
10271033

10281034
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
@@ -1071,7 +1077,7 @@ void CheckLeakAutoVar::ret(const Token *tok, VarInfo &varInfo, const bool isEndO
10711077
deallocReturnError(tok, it->second.allocTok, var->name());
10721078

10731079
else if (!used && !it->second.managed()) {
1074-
const std::map<int, std::string>::const_iterator use = possibleUsage.find(varid);
1080+
const auto use = possibleUsage.find(varid);
10751081
if (use == possibleUsage.end()) {
10761082
leakError(tok, var->name(), it->second.type);
10771083
} else {

‎lib/checkleakautovar.h‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ class CPPCHECKLIB VarInfo {
5555
return status < 0;
5656
}
5757
};
58+
enum Usage { USED, NORET };
5859
std::map<int, AllocInfo> alloctype;
59-
std::map<int, std::string> possibleUsage;
60+
std::map<int, std::pair<std::string, Usage>> possibleUsage;
6061
std::set<int> conditionalAlloc;
6162
std::set<int> referenced;
6263

@@ -92,7 +93,7 @@ class CPPCHECKLIB VarInfo {
9293
}
9394

9495
/** set possible usage for all variables */
95-
void possibleUsageAll(const std::string &functionName);
96+
void possibleUsageAll(const std::pair<std::string, Usage>& functionUsage);
9697

9798
void print();
9899
};
@@ -159,12 +160,12 @@ class CPPCHECKLIB CheckLeakAutoVar : public Check {
159160
void doubleFreeError(const Token *tok, const Token *prevFreeTok, const std::string &varname, int type);
160161

161162
/** message: user configuration is needed to complete analysis */
162-
void configurationInfo(const Token* tok, const std::string &functionName);
163+
void configurationInfo(const Token* tok, const std::pair<std::string, VarInfo::Usage>& functionUsage);
163164

164165
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override {
165166
CheckLeakAutoVar c(nullptr, settings, errorLogger);
166167
c.deallocReturnError(nullptr, nullptr, "p");
167-
c.configurationInfo(nullptr, "f"); // user configuration is needed to complete analysis
168+
c.configurationInfo(nullptr, { "f", VarInfo::USED }); // user configuration is needed to complete analysis
168169
c.doubleFreeError(nullptr, nullptr, "varname", 0);
169170
}
170171

‎test/testleakautovar.cpp‎

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@ class TestLeakAutoVar : public TestFixture {
104104
TEST_CASE(freopen2);
105105

106106
TEST_CASE(deallocuse1);
107-
TEST_CASE(deallocuse2);
108107
TEST_CASE(deallocuse3);
109108
TEST_CASE(deallocuse4);
110109
TEST_CASE(deallocuse5); // #4018: FP. free(p), p = 0;
@@ -508,8 +507,8 @@ class TestLeakAutoVar : public TestFixture {
508507
settings = s;
509508
}
510509

511-
void assign24() { // #7440
512-
check("void f() {\n"
510+
void assign24() {
511+
check("void f() {\n" // #7440
513512
" char* data = new char[100];\n"
514513
" char** dataPtr = &data;\n"
515514
" delete[] *dataPtr;\n"
@@ -523,6 +522,46 @@ class TestLeakAutoVar : public TestFixture {
523522
" delete[] *dataPtr;\n"
524523
"}\n", true);
525524
ASSERT_EQUALS("", errout.str());
525+
526+
check("void f() {\n" // #9279
527+
" int* p = new int;\n"
528+
" *p = 42;\n"
529+
" g();\n"
530+
"}\n", /*cpp*/ true);
531+
ASSERT_EQUALS("[test.cpp:4]: (information) --check-library: Function g() should have <noreturn> configuration\n",
532+
errout.str());
533+
534+
check("void g();\n"
535+
"void f() {\n"
536+
" int* p = new int;\n"
537+
" *p = 42;\n"
538+
" g();\n"
539+
"}\n", /*cpp*/ true);
540+
ASSERT_EQUALS("[test.cpp:6]: (error) Memory leak: p\n", errout.str());
541+
542+
check("void g() {}\n"
543+
"void f() {\n"
544+
" int* p = new int;\n"
545+
" *p = 42;\n"
546+
" g();\n"
547+
"}\n", /*cpp*/ true);
548+
ASSERT_EQUALS("[test.cpp:6]: (error) Memory leak: p\n", errout.str());
549+
550+
check("[[noreturn]] void g();\n"
551+
"void f() {\n"
552+
" int* p = new int;\n"
553+
" *p = 42;\n"
554+
" g();\n"
555+
"}\n", /*cpp*/ true);
556+
ASSERT_EQUALS("", errout.str());
557+
558+
check("void g() { exit(1); }\n"
559+
"void f() {\n"
560+
" int* p = new int;\n"
561+
" *p = 42;\n"
562+
" g();\n"
563+
"}\n", /*cpp*/ true);
564+
ASSERT_EQUALS("", errout.str());
526565
}
527566

528567
void isAutoDealloc() {
@@ -647,20 +686,6 @@ class TestLeakAutoVar : public TestFixture {
647686
ASSERT_EQUALS("[test.c:3]: (error) Dereferencing 'p' after it is deallocated / released\n", errout.str());
648687
}
649688

650-
void deallocuse2() {
651-
check("void f(char *p) {\n"
652-
" free(p);\n"
653-
" strcpy(a, p);\n"
654-
"}");
655-
TODO_ASSERT_EQUALS("error (free,use)", "[test.c:3]: (information) --check-library: Function strcpy() should have <noreturn> configuration\n", errout.str());
656-
657-
check("void f(char *p) {\n" // #3041 - assigning pointer when it's used
658-
" free(p);\n"
659-
" strcpy(a, p=b());\n"
660-
"}");
661-
TODO_ASSERT_EQUALS("", "[test.c:3]: (information) --check-library: Function strcpy() should have <noreturn> configuration\n", errout.str());
662-
}
663-
664689
void deallocuse3() {
665690
check("void f(struct str *p) {\n"
666691
" free(p);\n"
@@ -1437,8 +1462,7 @@ class TestLeakAutoVar : public TestFixture {
14371462
" char *p = malloc(10);\n"
14381463
" fatal_error();\n"
14391464
"}");
1440-
ASSERT_EQUALS("[test.c:3]: (information) --check-library: Function fatal_error() should have <noreturn> configuration\n"
1441-
"[test.c:4]: (information) --check-library: Function fatal_error() should have <use>/<leak-ignore> configuration\n",
1465+
ASSERT_EQUALS("[test.c:3]: (information) --check-library: Function fatal_error() should have <noreturn> configuration\n",
14421466
errout.str());
14431467
}
14441468

@@ -2711,6 +2735,7 @@ class TestLeakAutoVarStrcpy : public TestFixture {
27112735
LOAD_LIB_2(settings.library, "std.cfg");
27122736

27132737
TEST_CASE(returnedValue); // #9298
2738+
TEST_CASE(deallocuse2);
27142739
TEST_CASE(fclose_false_positive); // #9575
27152740
}
27162741

@@ -2724,6 +2749,20 @@ class TestLeakAutoVarStrcpy : public TestFixture {
27242749
ASSERT_EQUALS("", errout.str());
27252750
}
27262751

2752+
void deallocuse2() {
2753+
check("void f(char *p) {\n"
2754+
" free(p);\n"
2755+
" strcpy(a, p);\n"
2756+
"}");
2757+
TODO_ASSERT_EQUALS("error (free,use)", "", errout.str());
2758+
2759+
check("void f(char *p) {\n" // #3041 - assigning pointer when it's used
2760+
" free(p);\n"
2761+
" strcpy(a, p=b());\n"
2762+
"}");
2763+
ASSERT_EQUALS("", errout.str());
2764+
}
2765+
27272766
void fclose_false_positive() { // #9575
27282767
check("int f(FILE *fp) { return fclose(fp); }");
27292768
ASSERT_EQUALS("", errout.str());

0 commit comments

Comments
 (0)