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

@@ -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<unsigned int, std::string> &possibleUsage = varInfo.possibleUsage;
 
     const std::map<unsigned int, VarInfo::AllocInfo>::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<unsigned int, std::string>::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<unsigned int, VarInfo::AllocInfo>::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<unsigned int, std::string>::const_iterator use = possibleUsage.find(varid);
                 if (use == possibleUsage.end()) {
                     leakError(tok, var->name(), it->second.type);

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<unsigned int, AllocInfo> alloctype;
     std::map<unsigned int, std::string> possibleUsage;

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..

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);
@@ -105,6 +106,8 @@ private:
         // mismatching allocation/deallocation
         TEST_CASE(mismatchAllocDealloc);
         
+        TEST_CASE(smartPointerDeleter);
+
         // Execution reaches a 'return'
         TEST_CASE(return1);
         TEST_CASE(return2);
@@ -889,6 +892,58 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void doublefree8() {
+        check("void f() {\n"
+              "    int * i = new int;\n"
+              "    std::unique_ptr<int> 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<int> 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<int> 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<int> 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<int> 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<int> 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<int[]> 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<int> 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<int[]> 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<FILE> 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<FILE, decltype(&fclose)> fp{f, &fclose};\n"
+              "}", true);
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    FILE*f=fopen(fname,a);\n"
+              "    std::shared_ptr<FILE> 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<FILE, deleter> 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<int, decltype(&destroy)> 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<int, decltype(&destroy)> xp(x, &destroy());\n"
+              "}\n", true);
+        ASSERT_EQUALS("", errout.str());
     }
 
     void return1() {