From db8c7deb30ab79c6fb89008c76f2911481763c51 Mon Sep 17 00:00:00 2001 From: Thomas Jarosch Date: Sun, 23 Oct 2011 13:07:43 +0200 Subject: [PATCH] Fixed #3232 (Check if container is modified inside BOOST_FOREACH) --- Makefile | 8 ++++ lib/checkboost.cpp | 58 ++++++++++++++++++++++++++ lib/checkboost.h | 75 +++++++++++++++++++++++++++++++++ lib/lib.pri | 2 + lib/preprocessor.cpp | 7 +++- lib/tokenize.cpp | 2 +- test/testboost.cpp | 99 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 248 insertions(+), 3 deletions(-) create mode 100644 lib/checkboost.cpp create mode 100644 lib/checkboost.h create mode 100644 test/testboost.cpp diff --git a/Makefile b/Makefile index c1d35a74f..e04b0acdb 100644 --- a/Makefile +++ b/Makefile @@ -51,6 +51,7 @@ MAN_SOURCE=man/cppcheck.1.xml LIBOBJ = lib/check64bit.o \ lib/checkassignif.o \ lib/checkautovariables.o \ + lib/checkboost.o \ lib/checkbufferoverrun.o \ lib/checkclass.o \ lib/checkexceptionsafety.o \ @@ -88,6 +89,7 @@ TESTOBJ = test/options.o \ test/test64bit.o \ test/testassignif.o \ test/testautovariables.o \ + test/testboost.o \ test/testbufferoverrun.o \ test/testcharvar.o \ test/testclass.o \ @@ -181,6 +183,9 @@ lib/checkassignif.o: lib/checkassignif.cpp lib/checkassignif.h lib/check.h lib/t lib/checkautovariables.o: lib/checkautovariables.cpp lib/checkautovariables.h lib/check.h lib/token.h lib/tokenize.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h lib/symboldatabase.h lib/mathlib.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o lib/checkautovariables.o lib/checkautovariables.cpp +lib/checkboost.o: lib/checkboost.cpp lib/checkboost.h lib/check.h lib/token.h lib/tokenize.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h lib/symboldatabase.h lib/mathlib.h + $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o lib/checkboost.o lib/checkboost.cpp + lib/checkbufferoverrun.o: lib/checkbufferoverrun.cpp lib/checkbufferoverrun.h lib/check.h lib/token.h lib/tokenize.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h lib/mathlib.h lib/symboldatabase.h lib/executionpath.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_LIB} -c -o lib/checkbufferoverrun.o lib/checkbufferoverrun.cpp @@ -286,6 +291,9 @@ test/testassignif.o: test/testassignif.cpp lib/tokenize.h lib/checkassignif.h li test/testautovariables.o: test/testautovariables.cpp lib/tokenize.h lib/checkautovariables.h lib/check.h lib/token.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h test/testsuite.h test/redirect.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testautovariables.o test/testautovariables.cpp +test/testboost.o: test/testboost.cpp lib/tokenize.h lib/checkboost.h lib/check.h lib/token.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h test/testsuite.h test/redirect.h + $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testboost.o test/testboost.cpp + test/testbufferoverrun.o: test/testbufferoverrun.cpp lib/tokenize.h lib/checkbufferoverrun.h lib/check.h lib/token.h lib/settings.h lib/suppressions.h lib/standards.h lib/errorlogger.h lib/mathlib.h test/testsuite.h test/redirect.h $(CXX) $(CPPFLAGS) $(CXXFLAGS) ${INCLUDE_FOR_TEST} -c -o test/testbufferoverrun.o test/testbufferoverrun.cpp diff --git a/lib/checkboost.cpp b/lib/checkboost.cpp new file mode 100644 index 000000000..2d3d24257 --- /dev/null +++ b/lib/checkboost.cpp @@ -0,0 +1,58 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2011 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "checkboost.h" +#include "symboldatabase.h" +#include + +// Register this check class (by creating a static instance of it) +namespace { + CheckBoost instance; +} + +void CheckBoost::checkBoostForeachModification() +{ + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (!Token::simpleMatch(tok, "BOOST_FOREACH (")) + continue; + + const Token *container_tok = tok->next()->link()->tokAt(-1); + if (!Token::Match(container_tok, "%var% ) {")) + continue; + + const unsigned int container_id = container_tok->varId(); + if (container_id == 0) + continue; + + const Token *tok2 = container_tok->tokAt(2); + const Token *end = tok2->link(); + for (; tok2 != end; tok2 = tok2->next()) { + if (Token::Match(tok2, "%varid% . insert|erase|push_back|push_front|pop_front|pop_back|clear|swap|resize|assign|merge|remove|remove_if|reverse|sort|splice|unique|pop|push", container_id)) { + boostForeachError(tok2); + break; + } + } + } +} + +void CheckBoost::boostForeachError(const Token *tok) +{ + reportError(tok, Severity::error, "boostForeachError", + "BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container." + ); +} diff --git a/lib/checkboost.h b/lib/checkboost.h new file mode 100644 index 000000000..0c2f89109 --- /dev/null +++ b/lib/checkboost.h @@ -0,0 +1,75 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2011 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + + +//--------------------------------------------------------------------------- +#ifndef CHECKBOOST_H +#define CHECKBOOST_H +//--------------------------------------------------------------------------- + +#include "check.h" + +class Token; + +/// @addtogroup Checks +/// @{ + + +/** @brief %Check Boost usage */ +class CheckBoost : public Check { +public: + /** This constructor is used when registering the CheckClass */ + CheckBoost() : Check(myName()) + { } + + /** This constructor is used when running checks. */ + CheckBoost(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + : Check(myName(), tokenizer, settings, errorLogger) + { } + + /** Simplified checks. The token list is simplified. */ + void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + CheckBoost checkBoost(tokenizer, settings, errorLogger); + + checkBoost.checkBoostForeachModification(); + } + + /** @brief %Check for container modification while using the BOOST_FOREACH macro */ + void checkBoostForeachModification(); + +private: + void boostForeachError(const Token *tok); + + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { + CheckBoost c(0, settings, errorLogger); + c.boostForeachError(0); + } + + std::string myName() const { + return "Boost usage"; + } + + std::string classInfo() const { + return "Check for invalid usage of Boost:\n" + "* container modification during BOOST_FOREACH\n"; + } +}; +/// @} +//--------------------------------------------------------------------------- +#endif + diff --git a/lib/lib.pri b/lib/lib.pri index 6c899fe32..d0997df45 100644 --- a/lib/lib.pri +++ b/lib/lib.pri @@ -6,6 +6,7 @@ HEADERS += $${BASEPATH}check.h \ $${BASEPATH}check64bit.h \ $${BASEPATH}checkassignif.h \ $${BASEPATH}checkautovariables.h \ + $${BASEPATH}checkboost.h \ $${BASEPATH}checkbufferoverrun.h \ $${BASEPATH}checkclass.h \ $${BASEPATH}checkexceptionsafety.h \ @@ -35,6 +36,7 @@ HEADERS += $${BASEPATH}check.h \ SOURCES += $${BASEPATH}check64bit.cpp \ $${BASEPATH}checkassignif.cpp \ $${BASEPATH}checkautovariables.cpp \ + $${BASEPATH}checkboost.cpp \ $${BASEPATH}checkbufferoverrun.cpp \ $${BASEPATH}checkclass.cpp \ $${BASEPATH}checkexceptionsafety.cpp \ diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 9fb6cb2be..7e4717564 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -2374,9 +2374,12 @@ std::string Preprocessor::expandMacros(const std::string &code, std::string file // defining a macro.. if (line.compare(0, 8, "#define ") == 0) { PreprocessorMacro *macro = new PreprocessorMacro(line.substr(8)); - if (macro->name().empty()) + if (macro->name().empty()) { delete macro; - else { + } else if (macro->name() == "BOOST_FOREACH") { + // BOOST_FOREACH is currently too complex to parse, so skip it. + delete macro; + } else { std::map::iterator it; it = macros.find(macro->name()); if (it != macros.end()) diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 337e1cdb0..92cfca2ae 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -4680,7 +4680,7 @@ bool Tokenizer::simplifyIfAddBraces() continue; } - if (Token::Match(tok, "if|for|while (")) { + if (Token::Match(tok, "if|for|while|BOOST_FOREACH (")) { // don't add "{}" around ";" in "do {} while();" (#609) const Token *prev = tok->previous(); if (Token::simpleMatch(prev, "} while") && diff --git a/test/testboost.cpp b/test/testboost.cpp new file mode 100644 index 000000000..e27f3b4cf --- /dev/null +++ b/test/testboost.cpp @@ -0,0 +1,99 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2011 Daniel Marjamäki and Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + + + +#include "tokenize.h" +#include "checkboost.h" +#include "testsuite.h" +#include + +extern std::ostringstream errout; + +class TestBoost : public TestFixture { +public: + TestBoost() : TestFixture("TestBoost") + { } + +private: + void run() { + TEST_CASE(BoostForeachContainerModification) + } + + void check(const std::string &code) { + // Clear the error buffer.. + errout.str(""); + + Settings settings; + settings.addEnabled("style"); + settings.addEnabled("performance"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code.c_str()); + tokenizer.tokenize(istr, "test.cpp"); + tokenizer.simplifyTokenList(); + + // Check.. + CheckBoost checkBoost; + checkBoost.runSimplifiedChecks(&tokenizer, &settings, this); + } + + void BoostForeachContainerModification() { + check("void f() {\n" + " vector data;\n" + " BOOST_FOREACH(int i, data) {\n" + " data.push_back(123);\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container.\n", errout.str()); + + check("void f() {\n" + " set data;\n" + " BOOST_FOREACH(int i, data) {\n" + " data.insert(123);\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container.\n", errout.str()); + + check("void f() {\n" + " set data;\n" + " BOOST_FOREACH(const int &i, data) {\n" + " data.erase(123);\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container.\n", errout.str()); + + // Check single line usage + check("void f() {\n" + " set data;\n" + " BOOST_FOREACH(const int &i, data)\n" + " data.clear();\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container.\n", errout.str()); + + // Container returned as result of a function -> Be quiet + check("void f() {\n" + " BOOST_FOREACH(const int &i, get_data())\n" + " data.insert(i);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } +}; + +REGISTER_TEST(TestBoost)