diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp index e1bfb4606..73eb9e739 100644 --- a/lib/checkexceptionsafety.cpp +++ b/lib/checkexceptionsafety.cpp @@ -97,8 +97,8 @@ void CheckExceptionSafety::unsafeNew() if (tok->str() != ":") continue; - // count "new" and check that it's an initializer list.. - unsigned int countNew = 0; + // multiple "new" in an initializer list.. + 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 (countNew > 1) - unsafeNewError(tok); - break; - } - else if (tok->str() != ",") + 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; - if (countNew > 1) - { - unsafeNewError(tok); - break; - } + unsafeNewError(tok, varname); + break; } + if (!autodealloc(tok->tokAt(3), _tokenizer->tokens())) + varname = tok->str(); } } } // allocating multiple local variables.. std::set 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; - if (countNew >= 2) - unsafeNewError(tok->next()); + varname = tok->strAt(1); } } } @@ -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; + } + } + } +} + + diff --git a/lib/checkexceptionsafety.h b/lib/checkexceptionsafety.h index 0bd73faa4..cd1f7c092 100644 --- a/lib/checkexceptionsafety.h +++ b/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"; } }; /// @} diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 468945f2f..3fa5e4a12 100644 --- a/lib/checkother.cpp +++ b/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; diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp index f2624ce2c..cb532ba6c 100644 --- a/test/testexceptionsafety.cpp +++ b/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.unsafeNew(); - checkExceptionSafety.realloc(); + checkExceptionSafety.runSimplifiedChecks(&tokenizer, &settings, this); } 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)