From 543d5cbf455a75ea93b44a85dd5613da90530800 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 31 Oct 2009 18:58:42 +0100 Subject: [PATCH] Fixed #831 (Exception safety: multiple new in a simple execution path) --- lib/checkexceptionsafety.cpp | 93 ++++++++++++++++++++++++++++++++++++ lib/checkexceptionsafety.h | 13 ++++- lib/cppcheck.cpp | 5 ++ lib/settings.cpp | 1 + lib/settings.h | 3 ++ test/testexceptionsafety.cpp | 24 +++++++++- 6 files changed, 136 insertions(+), 3 deletions(-) diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp index 2758221a7..946c99727 100644 --- a/lib/checkexceptionsafety.cpp +++ b/lib/checkexceptionsafety.cpp @@ -74,3 +74,96 @@ void CheckExceptionSafety::destructors() } } + +static bool autodealloc(const Token * const C, const Token * const tokens) +{ + if (C->isStandardType()) + return false; + return !Token::findmatch(tokens, ("class " + C->str() + " {").c_str()); +} + +void CheckExceptionSafety::unsafeNew() +{ + // Check that "--exception-safety" was given + if (!_settings->_exceptionSafety) + return; + + // Inspect initializer lists.. + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) + { + if (tok->str() != ")") + continue; + tok = tok->next(); + if (tok->str() != ":") + continue; + + // count "new" and check that it's an initializer list.. + unsigned int countNew = 0; + for (tok = tok->next(); tok; tok = tok->next()) + { + if (!Token::Match(tok, "%var% (")) + break; + tok = tok->next(); + if (Token::Match(tok->next(), "new %type%")) + { + if (countNew > 0 || !autodealloc(tok->tokAt(2), _tokenizer->tokens())) + { + ++countNew; + } + } + tok = tok->link(); + tok = tok ? tok->next() : 0; + if (!tok) + break; + if (tok->str() == "{") + { + if (countNew > 1) + unsafeNewError(tok); + break; + } + else if (tok->str() != ",") + break; + } + if (!tok) + break; + } + + + // Inspect constructors.. + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) + { + // match this pattern.. "C :: C ( .. ) {" + if (!tok->next() || tok->next()->str() != "::") + continue; + if (!Token::Match(tok, "%var% :: %var% (")) + continue; + if (tok->str() != tok->strAt(2)) + continue; + if (!Token::simpleMatch(tok->tokAt(3)->link(), ") {")) + continue; + + // inspect the constructor.. + unsigned int countNew = 0; + for (tok = tok->tokAt(3)->link()->tokAt(2); tok; tok = tok->next()) + { + if (tok->str() == "{" || tok->str() == "}") + break; + // some variable declaration.. + if (Token::Match(tok->previous(), "[{;] %type% * %var% ;")) + break; + // allocating with new.. + if (Token::Match(tok, "%var% = new %type%")) + { + if (countNew > 0 || !autodealloc(tok->tokAt(3), _tokenizer->tokens())) + { + ++countNew; + if (countNew > 1) + { + unsafeNewError(tok); + break; + } + } + } + } + } +} diff --git a/lib/checkexceptionsafety.h b/lib/checkexceptionsafety.h index 1a517ccd7..cbee704e4 100644 --- a/lib/checkexceptionsafety.h +++ b/lib/checkexceptionsafety.h @@ -52,6 +52,9 @@ public: /** Don't throw exceptions in destructors */ void destructors(); + /** unsafe use of "new" */ + void unsafeNew(); + private: /** Don't throw exceptions in destructors */ void destructorsError(const Token * const tok) @@ -59,9 +62,16 @@ private: reportError(tok, Severity::style, "throwInDestructor", "Throwing exception in destructor is not recommended"); } + /** Unsafe use of new */ + void unsafeNewError(const Token * const tok) + { + reportError(tok, Severity::style, "unsafeNew", "Exception safety: unsafe use of 'new'"); + } + void getErrorMessages() { destructorsError(0); + unsafeNewError(0); } std::string name() const @@ -72,7 +82,8 @@ private: std::string classInfo() const { return "Checking exception safety\n" - " * Don't throw exceptions in destructors"; + " * Don't throw exceptions in destructors" + " * Unsafe use of 'new'"; } }; /// @} diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index b77f1dc8c..c54b63821 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -109,6 +109,10 @@ void CppCheck::parseFromArgs(int argc, const char* const argv[]) else if (strcmp(argv[i], "-s") == 0 || strcmp(argv[i], "--style") == 0) _settings._checkCodingStyle = true; + // Checking exception safety + else if (strcmp(argv[i], "--exception-safety") == 0) + _settings._exceptionSafety = true; + // Filter errors else if (strcmp(argv[i], "--suppressions") == 0) { @@ -348,6 +352,7 @@ void CppCheck::parseFromArgs(int argc, const char* const argv[]) " if arguments are not valid or if no input files are\n" " provided. Note that your operating system can\n" " modify this value, e.g. 256 can become 0.\n" + " --exception-safety Extended checking for exception safety\n" " -f, --force Force checking on files that have \"too many\"\n" " configurations\n" " -h, --help Print this help\n" diff --git a/lib/settings.cpp b/lib/settings.cpp index 16bc139f6..f1758d54f 100644 --- a/lib/settings.cpp +++ b/lib/settings.cpp @@ -36,6 +36,7 @@ Settings::Settings() _exitCode = 0; _showtime = false; _append = ""; + _exceptionSafety = false; } Settings::~Settings() diff --git a/lib/settings.h b/lib/settings.h index 5ee008b03..9476af968 100644 --- a/lib/settings.h +++ b/lib/settings.h @@ -56,6 +56,9 @@ public: bool _errorsOnly; bool _verbose; + /** extra checks for exception safety */ + bool _exceptionSafety; + /** Force checking t he files with "too many" configurations. */ bool _force; diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp index 1908ac7b6..7d72d621d 100644 --- a/test/testexceptionsafety.cpp +++ b/test/testexceptionsafety.cpp @@ -35,13 +35,14 @@ private: void run() { TEST_CASE(destructors); + TEST_CASE(newnew); } - void check(const char code[]) + void check(const std::string &code) { // Tokenize.. Tokenizer tokenizer; - std::istringstream istr(code); + std::istringstream istr(code.c_str()); tokenizer.tokenize(istr, "test.cpp"); tokenizer.simplifyTokenList(); @@ -51,8 +52,10 @@ private: // Check char variable usage.. Settings settings; settings._checkCodingStyle = true; + settings._exceptionSafety = true; CheckExceptionSafety checkExceptionSafety(&tokenizer, &settings, this); checkExceptionSafety.destructors(); + checkExceptionSafety.unsafeNew(); } void destructors() @@ -64,6 +67,23 @@ private: ASSERT_EQUALS("[test.cpp:3]: (style) Throwing exception in destructor is not recommended\n", errout.str()); } + void newnew() + { + const std::string AB("class A { }; class B { }; "); + + check(AB + "C::C() : a(new A), b(new B) { }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Exception safety: unsafe use of 'new'\n", errout.str()); + + check("C::C() : a(new A), b(new B) { }"); + ASSERT_EQUALS("", errout.str()); + + check(AB + "C::C()\n" + "{\n" + " a = new A;\n" + " b = new B;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Exception safety: unsafe use of 'new'\n", errout.str()); + } }; REGISTER_TEST(TestExceptionSafety)