Fixed #832 (Exception Safety: deallocating member pointer and then throwing exception)

lib/checkexceptionsafety.cpp

@@ -97,8 +97,8 @@ void CheckExceptionSafety::unsafeNew()
         if (tok->str() != ":")
             continue;
 
-        // count "new" and check that it's an initializer list..
+        // multiple "new" in an initializer list..
-        unsigned int countNew = 0;
+        std::string varname;
         for (tok = tok->next(); tok; tok = tok->next())
         {
             if (!Token::Match(tok, "%var% ("))
@@ -106,22 +106,21 @@ void CheckExceptionSafety::unsafeNew()
             tok = tok->next();
             if (Token::Match(tok->next(), "new %type%"))
             {
-                if (countNew > 0 || !autodealloc(tok->tokAt(2), _tokenizer->tokens()))
+                if (!varname.empty())
                 {
-                    ++countNew;
+                    unsafeNewError(tok->previous(), varname);
+                    break;
+                }
+                if (!autodealloc(tok->tokAt(2), _tokenizer->tokens()))
+                {
+                    varname = tok->strAt(-1);
                 }
             }
             tok = tok->link();
             tok = tok ? tok->next() : 0;
             if (!tok)
                 break;
-            if (tok->str() == "{")
+            if (tok->str() != ",")
-            {
-                if (countNew > 1)
-                    unsafeNewError(tok);
-                break;
-            }
-            else if (tok->str() != ",")
                 break;
         }
         if (!tok)
@@ -143,7 +142,7 @@ void CheckExceptionSafety::unsafeNew()
             continue;
 
         // inspect the constructor..
-        unsigned int countNew = 0;
+        std::string varname;
         for (tok = tok->tokAt(3)->link()->tokAt(2); tok; tok = tok->next())
         {
             if (tok->str() == "{" || tok->str() == "}")
@@ -154,28 +153,26 @@ void CheckExceptionSafety::unsafeNew()
             // allocating with new..
             if (Token::Match(tok, "%var% = new %type%"))
             {
-                if (countNew > 0 || !autodealloc(tok->tokAt(3), _tokenizer->tokens()))
+                if (!varname.empty())
                 {
-                    ++countNew;
+                    unsafeNewError(tok, varname);
-                    if (countNew > 1)
+                    break;
-                    {
-                        unsafeNewError(tok);
-                        break;
-                    }
                 }
+                if (!autodealloc(tok->tokAt(3), _tokenizer->tokens()))
+                    varname = tok->str();
             }
         }
     }
 
     // allocating multiple local variables..
     std::set<unsigned int> localVars;
-    unsigned int countNew = 0;
+    std::string varname;
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
     {
         if (tok->str() == "{" || tok->str() == "}")
         {
             localVars.clear();
-            countNew = 0;
+            varname = "";
         }
 
         if (Token::Match(tok, "[;{}] %type% * %var% ;"))
@@ -187,10 +184,13 @@ void CheckExceptionSafety::unsafeNew()
 
         if (Token::Match(tok, "; %var% = new"))
         {
+            if (!varname.empty())
+            {
+                unsafeNewError(tok->next(), varname);
+                break;
+            }
             if (tok->next()->varId() && localVars.find(tok->next()->varId()) != localVars.end())
-                ++countNew;
+                varname = tok->strAt(1);
-            if (countNew >= 2)
-                unsafeNewError(tok->next());
         }
     }
 }
@@ -259,3 +259,83 @@ void CheckExceptionSafety::realloc()
         }
     }
 }
+
+
+void CheckExceptionSafety::deallocThrow()
+{
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
+    {
+        if (tok->str() != "delete")
+            continue;
+
+        // Check if this is something similar with: "delete p;"
+        tok = tok->next();
+        if (Token::simpleMatch(tok, "[ ]"))
+            tok = tok->tokAt(2);
+        if (!tok)
+            break;
+        if (!Token::Match(tok, "%var% ;"))
+            continue;
+        const unsigned int varid(tok->varId());
+        if (varid == 0)
+            continue;
+
+        // is this variable a global variable?
+        {
+            bool globalVar = false;
+            for (const Token *tok2 = _tokenizer->tokens(); tok2; tok2 = tok2->next())
+            {
+                if (tok->varId() == varid)
+                {
+                    globalVar = true;
+                    break;
+                }
+
+                if (tok2->str() == "class")
+                {
+                    while (tok2 && tok2->str() != ";" && tok2->str() != "{")
+                        tok2 = tok2->next();
+                    tok2 = tok2 ? tok2->next() : 0;
+                    if (!tok2)
+                        break;
+                }
+
+                if (tok2->str() == "{")
+                {
+                    tok2 = tok2->link();
+                    if (!tok2)
+                        break;
+                }
+            }
+            if (!globalVar)
+                continue;
+        }
+
+        // is there a throw after the deallocation?
+        unsigned int indentlevel = 0;
+        const Token *ThrowToken = 0;
+        for (const Token *tok2 = tok; tok2; tok2 = tok2->next())
+        {
+            if (tok2->str() == "{")
+                ++indentlevel;
+            else if (tok2->str() == "}")
+            {
+                if (indentlevel == 0)
+                    break;
+                --indentlevel;
+            }
+
+            if (tok2->str() == "throw")
+                ThrowToken = tok2;
+
+            else if (Token::Match(tok2, "%varid% =", varid))
+            {
+                if (ThrowToken)
+                    deallocThrowError(ThrowToken, tok->str());
+                break;
+            }
+        }
+    }
+}
+
+

lib/checkexceptionsafety.h

@@ -48,6 +48,7 @@ public:
         checkExceptionSafety.destructors();
         checkExceptionSafety.unsafeNew();
         checkExceptionSafety.realloc();
+        checkExceptionSafety.deallocThrow();
     }
 
 
@@ -60,17 +61,20 @@ public:
     /** Unsafe realloc */
     void realloc();
 
+    /** deallocating memory and then throw */
+    void deallocThrow();
+
 private:
     /** Don't throw exceptions in destructors */
     void destructorsError(const Token * const tok)
     {
-        reportError(tok, Severity::style, "exceptThrowInDestructor", "Throwing exception in destructor is not recommended");
+        reportError(tok, Severity::style, "exceptThrowInDestructor", "Throwing exception in destructor");
     }
 
     /** Unsafe use of new */
-    void unsafeNewError(const Token * const tok)
+    void unsafeNewError(const Token * const tok, const std::string &varname)
     {
-        reportError(tok, Severity::style, "exceptNew", "Upon exception there are memory leaks");
+        reportError(tok, Severity::style, "exceptNew", "Upon exception there is memory leak: " + varname);
     }
 
     /** Unsafe reallocation */
@@ -79,11 +83,17 @@ private:
         reportError(tok, Severity::style, "exceptRealloc", "Upon exception " + varname + " becomes a dead pointer");
     }
 
+    void deallocThrowError(const Token * const tok, const std::string &varname)
+    {
+        reportError(tok, Severity::error, "exceptDeallocThrow", "Throwing exception in invalid state, " + varname + " points at deallocated memory");
+    }
+
     void getErrorMessages()
     {
         destructorsError(0);
-        unsafeNewError(0);
+        unsafeNewError(0, "p");
         reallocError(0, "p");
+        deallocThrowError(0, "p");
     }
 
     std::string name() const
@@ -94,9 +104,10 @@ private:
     std::string classInfo() const
     {
         return "Checking exception safety\n"
-               " * Don't throw exceptions in destructors\n"
+               " * Throwing exceptions in destructors\n"
                " * Unsafe use of 'new'\n"
-               " * Unsafe reallocation\n";
+               " * Unsafe reallocation\n"
+               " * Throwing exception during invalid state";
     }
 };
 /// @}

lib/checkother.cpp

@@ -1363,7 +1363,7 @@ static const Token *uninitvar_checkscope(const Token * const tokens, const Token
         {
             if (array && !Token::simpleMatch(tok->next(), "["))
                 continue;
-            
+
             if (Token::simpleMatch(tok->previous(), "return"))
                 return tok;
 

test/testexceptionsafety.cpp

@@ -37,6 +37,7 @@ private:
         TEST_CASE(destructors);
         TEST_CASE(newnew);
         TEST_CASE(realloc);
+        TEST_CASE(deallocThrow);
     }
 
     void check(const std::string &code)
@@ -55,9 +56,7 @@ private:
         settings._checkCodingStyle = true;
         settings._exceptionSafety = true;
         CheckExceptionSafety checkExceptionSafety(&tokenizer, &settings, this);
-        checkExceptionSafety.destructors();
+        checkExceptionSafety.runSimplifiedChecks(&tokenizer, &settings, this);
-        checkExceptionSafety.unsafeNew();
-        checkExceptionSafety.realloc();
     }
 
     void destructors()
@@ -66,7 +65,7 @@ private:
               "{\n"
               "    throw e;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:3]: (style) Throwing exception in destructor is not recommended\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Throwing exception in destructor\n", errout.str());
     }
 
     void newnew()
@@ -74,7 +73,7 @@ private:
         const std::string AB("class A { }; class B { }; ");
 
         check(AB + "C::C() : a(new A), b(new B) { }");
-        ASSERT_EQUALS("[test.cpp:1]: (style) Upon exception there are memory leaks\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (style) Upon exception there is memory leak: a\n", errout.str());
 
         check("C::C() : a(new A), b(new B) { }");
         ASSERT_EQUALS("", errout.str());
@@ -84,14 +83,14 @@ private:
               "    a = new A;\n"
               "    b = new B;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there are memory leaks\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there is memory leak: a\n", errout.str());
 
         check("void a()\n"
               "{\n"
               "    A *a1 = new A;\n"
               "    A *a2 = new A;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there are memory leaks\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there is memory leak: a1\n", errout.str());
     }
 
     void realloc()
@@ -120,6 +119,19 @@ private:
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void deallocThrow()
+    {
+        check("int * p;\n"
+              "void f(int x)\n"
+              "{\n"
+              "    delete p;\n"
+              "    if (x)\n"
+              "        throw 123;\n"
+              "    p = 0;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:6]: (error) Throwing exception in invalid state, p points at deallocated memory\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestExceptionSafety)