From 2a657cfd085531c1bd05ea60571531c888b6ca2e Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 16 Apr 2018 04:11:13 -0500 Subject: [PATCH] Check for double frees when using smart pointers (#1172) * Check for double frees when using smart pointers * Some updates from feedback * Add test for mismatch allocation * Constants * Check smart pointer deleter * Switch order * Use next * Add owned state * Fix handling of leaks * Use ast for checking addressof operator * Remove stray character * Add a test for mismatch allocator * Add another test for deallocating with custom function --- lib/checkleakautovar.cpp | 64 ++++++++++++++++++++--- lib/checkleakautovar.h | 7 ++- lib/library.cpp | 2 +- test/testleakautovar.cpp | 110 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 174 insertions(+), 9 deletions(-) diff --git a/lib/checkleakautovar.cpp b/lib/checkleakautovar.cpp index 3ba6ac5be..b8665d69b 100644 --- a/lib/checkleakautovar.cpp +++ b/lib/checkleakautovar.cpp @@ -67,6 +67,9 @@ void VarInfo::print() std::string status; switch (it->second.status) { + case OWNED: + status = "owned"; + break; case DEALLOC: status = "dealloc"; break; @@ -456,13 +459,13 @@ void CheckLeakAutoVar::checkScope(const Token * const startToken, // Conditional allocation/deallocation for (it = varInfo1.alloctype.begin(); it != varInfo1.alloctype.end(); ++it) { - if (it->second.status == VarInfo::DEALLOC && conditionalAlloc.find(it->first) != conditionalAlloc.end()) { + if (it->second.managed() && conditionalAlloc.find(it->first) != conditionalAlloc.end()) { varInfo->conditionalAlloc.erase(it->first); varInfo2.erase(it->first); } } for (it = varInfo2.alloctype.begin(); it != varInfo2.alloctype.end(); ++it) { - if (it->second.status == VarInfo::DEALLOC && conditionalAlloc.find(it->first) != conditionalAlloc.end()) { + if (it->second.managed() && conditionalAlloc.find(it->first) != conditionalAlloc.end()) { varInfo->conditionalAlloc.erase(it->first); varInfo1.erase(it->first); } @@ -553,6 +556,53 @@ void CheckLeakAutoVar::checkScope(const Token * const startToken, else if (Token::Match(tok, "continue|break ;")) { varInfo->clear(); } + + // Check smart pointer + else if (Token::Match(ftok, "auto_ptr|unique_ptr|shared_ptr < %type%")) { + + + const Token * endToken = ftok->linkAt(1); + if(!Token::Match(endToken, "> %var% {|( %var%")) + continue; + + bool arrayDelete = false; + if(Token::findsimplematch(ftok->next(), "[ ]", endToken)) + arrayDelete = true; + + // Check deleter + const Token * deleterToken = nullptr; + const Token * endDeleterToken = nullptr; + const Library::AllocFunc* af = nullptr; + if(Token::Match(ftok, "unique_ptr < %type% ,")) { + deleterToken = ftok->tokAt(4); + endDeleterToken = endToken; + } else if(Token::Match(endToken, "> %var% {|( %var% ,")) { + deleterToken = endToken->tokAt(5); + endDeleterToken = endToken->linkAt(2); + } + if(deleterToken) { + // Check if its a pointer to a function + const Token * dtok = Token::findmatch(deleterToken, "& %name%", endDeleterToken); + if (dtok) { + af = _settings->library.dealloc(dtok->tokAt(1)); + } else { + // If the deleter is a class, check if class calls the dealloc function + dtok = Token::findmatch(deleterToken, "%type%", endDeleterToken); + if (dtok && dtok->type()) { + const Scope * tscope = dtok->type()->classScope; + for (const Token *tok2 = tscope->classStart; tok2 != tscope->classEnd; tok2 = tok2->next()) { + af = _settings->library.dealloc(tok2); + if (af) + break; + } + } + } + } + + const Token * vtok = endToken->tokAt(3); + const VarInfo::AllocInfo allocation(af ? af->groupId : (arrayDelete ? NEW_ARRAY : NEW), VarInfo::OWNED); + changeAllocStatus(varInfo, allocation, vtok, vtok); + } } } @@ -567,7 +617,7 @@ void CheckLeakAutoVar::changeAllocStatus(VarInfo *varInfo, const VarInfo::AllocI possibleUsage[arg->varId()] = tok->str(); if (var->second.status == VarInfo::DEALLOC && arg->previous()->str() == "&") varInfo->erase(arg->varId()); - } else if (var->second.status == VarInfo::DEALLOC) { + } else if (var->second.managed()) { doubleFreeError(tok, arg->str(), allocation.type); } else if (var->second.type != allocation.type) { // mismatching allocation and deallocation @@ -575,7 +625,7 @@ void CheckLeakAutoVar::changeAllocStatus(VarInfo *varInfo, const VarInfo::AllocI varInfo->erase(arg->varId()); } else { // deallocation - var->second.status = VarInfo::DEALLOC; + var->second.status = allocation.status; var->second.type = allocation.type; } } else if (allocation.status != VarInfo::NOALLOC) { @@ -629,7 +679,7 @@ void CheckLeakAutoVar::leakIfAllocated(const Token *vartok, const std::map &possibleUsage = varInfo.possibleUsage; const std::map::const_iterator var = alloctype.find(vartok->varId()); - if (var != alloctype.end() && var->second.status != VarInfo::DEALLOC) { + if (var != alloctype.end() && var->second.status == VarInfo::ALLOC) { const std::map::const_iterator use = possibleUsage.find(vartok->varId()); if (use == possibleUsage.end()) { leakError(vartok, vartok->str(), var->second.type); @@ -647,7 +697,7 @@ void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo) const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); for (std::map::const_iterator it = alloctype.begin(); it != alloctype.end(); ++it) { // don't warn if variable is conditionally allocated - if (it->second.status != VarInfo::DEALLOC && varInfo.conditionalAlloc.find(it->first) != varInfo.conditionalAlloc.end()) + if (!it->second.managed() && varInfo.conditionalAlloc.find(it->first) != varInfo.conditionalAlloc.end()) continue; // don't warn if there is a reference of the variable @@ -675,7 +725,7 @@ void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo) if (used && it->second.status == VarInfo::DEALLOC) deallocReturnError(tok, var->name()); - else if (!used && it->second.status != VarInfo::DEALLOC) { + else if (!used && !it->second.managed()) { const std::map::const_iterator use = possibleUsage.find(varid); if (use == possibleUsage.end()) { leakError(tok, var->name(), it->second.type); diff --git a/lib/checkleakautovar.h b/lib/checkleakautovar.h index d84cd3d7e..eef35b8a0 100644 --- a/lib/checkleakautovar.h +++ b/lib/checkleakautovar.h @@ -38,7 +38,7 @@ class Tokenizer; class CPPCHECKLIB VarInfo { public: - enum AllocStatus { DEALLOC = -1, NOALLOC = 0, ALLOC = 1 }; + enum AllocStatus { OWNED = -2, DEALLOC = -1, NOALLOC = 0, ALLOC = 1 }; struct AllocInfo { AllocStatus status; /** Allocation type. If it is a positive value then it corresponds to @@ -47,6 +47,11 @@ public: */ int type; AllocInfo(int type_ = 0, AllocStatus status_ = NOALLOC) : status(status_), type(type_) {} + + bool managed() const + { + return status < 0; + } }; std::map alloctype; std::map possibleUsage; diff --git a/lib/library.cpp b/lib/library.cpp index 045b6fffd..959e61b70 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -770,7 +770,7 @@ std::string Library::getFunctionName(const Token *ftok, bool *error) const std::string Library::getFunctionName(const Token *ftok) const { - if (!Token::Match(ftok, "%name% (")) + if (!Token::Match(ftok, "%name% (") && (ftok->strAt(-1) != "&" || ftok->previous()->astOperand2())) return ""; // Lookup function name using AST.. diff --git a/test/testleakautovar.cpp b/test/testleakautovar.cpp index 1a9086c74..6ea18ced4 100644 --- a/test/testleakautovar.cpp +++ b/test/testleakautovar.cpp @@ -72,6 +72,7 @@ private: TEST_CASE(doublefree5); // #5522 TEST_CASE(doublefree6); // #7685 TEST_CASE(doublefree7); + TEST_CASE(doublefree8); // exit TEST_CASE(exit1); @@ -104,6 +105,8 @@ private: // mismatching allocation/deallocation TEST_CASE(mismatchAllocDealloc); + + TEST_CASE(smartPointerDeleter); // Execution reaches a 'return' TEST_CASE(return1); @@ -889,6 +892,58 @@ private: ASSERT_EQUALS("", errout.str()); } + void doublefree8() { + check("void f() {\n" + " int * i = new int;\n" + " std::unique_ptr x(i);\n" + " delete i;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + + check("void f() {\n" + " int * i = new int;\n" + " delete i;\n" + " std::unique_ptr x(i);\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + + check("void f() {\n" + " int * i = new int;\n" + " std::unique_ptr x{i};\n" + " delete i;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + + check("void f() {\n" + " int * i = new int;\n" + " std::shared_ptr x(i);\n" + " delete i;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + + check("void f() {\n" + " int * i = new int;\n" + " std::shared_ptr x{i};\n" + " delete i;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + + // Check for use-after-free FP + check("void f() {\n" + " int * i = new int;\n" + " std::shared_ptr x{i};\n" + " *i = 123;\n" + "}\n", true); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int * i = new int[1];\n" + " std::unique_ptr x(i);\n" + " delete i;\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory pointed to by 'i' is freed twice.\n", errout.str()); + } + void exit1() { check("void f() {\n" " char *p = malloc(10);\n" @@ -1129,6 +1184,61 @@ private: " char *cPtr = new (buf) char[100];\n" "}", true); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " int * i = new int[1];\n" + " std::unique_ptr x(i);\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching allocation and deallocation: i\n", errout.str()); + + check("void f() {\n" + " int * i = new int;\n" + " std::unique_ptr x(i);\n" + "}\n", true); + ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching allocation and deallocation: i\n", errout.str()); + } + + void smartPointerDeleter() { + check("void f() {\n" + " FILE*f=fopen(fname,a);\n" + " std::unique_ptr fp{f};\n" + "}", true); + ASSERT_EQUALS("[test.cpp:3]: (error) Mismatching allocation and deallocation: f\n", errout.str()); + + check("void f() {\n" + " FILE*f=fopen(fname,a);\n" + " std::unique_ptr fp{f, &fclose};\n" + "}", true); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " FILE*f=fopen(fname,a);\n" + " std::shared_ptr fp{f, &fclose};\n" + "}", true); + ASSERT_EQUALS("", errout.str()); + + check("struct deleter { void operator()(FILE* f) { fclose(f); }};\n" + "void f() {\n" + " FILE*f=fopen(fname,a);\n" + " std::unique_ptr fp{f};\n" + "}", true); + ASSERT_EQUALS("", errout.str()); + + check("int * create();\n" + "void destroy(int * x);\n" + "void f() {\n" + " int x * = create()\n" + " std::unique_ptr xp{x, &destroy()};\n" + "}\n", true); + ASSERT_EQUALS("", errout.str()); + + check("int * create();\n" + "void destroy(int * x);\n" + "void f() {\n" + " int x * = create()\n" + " std::unique_ptr xp(x, &destroy());\n" + "}\n", true); + ASSERT_EQUALS("", errout.str()); } void return1() {