From ecfe4eb5be569fd78050d4782015817a77668e28 Mon Sep 17 00:00:00 2001 From: zblair Date: Thu, 4 Apr 2013 21:14:59 -0700 Subject: [PATCH] Fixed #3372 (New check: dereference iterator and then checking it) --- lib/checkstl.cpp | 69 ++++++++++++++++++++++++++++++ lib/checkstl.h | 9 +++- test/teststl.cpp | 108 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+), 1 deletion(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 3d4eaeda0..d792d4aac 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -1457,3 +1457,72 @@ void CheckStl::uselessCallsRemoveError(const Token *tok, const std::string& func "The return value of std::" + function + "() is ignored. This function returns an iterator to the end of the range containing those elements that should be kept. " "Elements past new end remain valid but with unspecified values. Use the erase method of the container to delete them."); } + +// Check for iterators being dereferenced before being checked for validity. +// E.g. if (*i && i != str.end()) { } +void CheckStl::checkDereferenceInvalidIterator() +{ + if (!_settings->isEnabled("warning")) + return; + + // Iterate over "if", "while", and "for" conditions where there may + // be an iterator that is dereferenced before being checked for validity. + const std::list& scopeList = _tokenizer->getSymbolDatabase()->scopeList; + for (std::list::const_iterator i = scopeList.begin(); i != scopeList.end(); ++i) { + if (i->type == Scope::eIf || i->type == Scope::eWhile || i->type == Scope::eFor) { + + const Token* const tok = i->classDef; + const Token* startOfCondition = tok->next(); + const Token* endOfCondition = startOfCondition->link(); + if (!endOfCondition) + continue; + + // For "for" loops, only search between the two semicolons + if (i->type == Scope::eFor) { + startOfCondition = Token::findmatch(tok->tokAt(2), ";", endOfCondition); + if (!startOfCondition) + continue; + endOfCondition = Token::findmatch(startOfCondition->next(), ";", endOfCondition); + if (!endOfCondition) + continue; + } + + // Only consider conditions composed of all "&&" terms and + // conditions composed of all "||" terms + const bool isOrExpression = + Token::findsimplematch(startOfCondition, "||", endOfCondition); + const bool isAndExpression = + Token::findsimplematch(startOfCondition, "&&", endOfCondition); + + // Look for a check of the validity of an iterator + const Token* validityCheckTok = 0; + if (!isOrExpression && isAndExpression) { + validityCheckTok = + Token::findmatch(startOfCondition, "&& %var% != %var% . end|rend|cend|crend ( )", endOfCondition); + } else if (isOrExpression && !isAndExpression) { + validityCheckTok = + Token::findmatch(startOfCondition, "%oror% %var% == %var% . end|rend|cend|crend ( )", endOfCondition); + } + + if (!validityCheckTok) + continue; + const unsigned int iteratorVarId = validityCheckTok->next()->varId(); + if (!iteratorVarId) + continue; + + // If the iterator dereference is to the left of the check for + // the iterator's validity, report an error. + const Token* const dereferenceTok = + Token::findmatch(startOfCondition, "* %varid%", validityCheckTok, iteratorVarId); + if (dereferenceTok) + dereferenceInvalidIteratorError(dereferenceTok, dereferenceTok->strAt(1)); + } + } +} + +void CheckStl::dereferenceInvalidIteratorError(const Token* deref, const std::string &iterName) +{ + reportError(deref, Severity::warning, + "derefInvalidIterator", "Possible dereference of an invalid iterator: " + iterName + "\n" + + "Make sure to check that the iterator is valid before dereferencing it - not after."); +} diff --git a/lib/checkstl.h b/lib/checkstl.h index 396832afc..46d151b8f 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -60,6 +60,7 @@ public: checkStl.string_c_str(); checkStl.checkAutoPointer(); checkStl.uselessCalls(); + checkStl.checkDereferenceInvalidIterator(); // Style check checkStl.size(); @@ -134,6 +135,8 @@ public: /** @brief %Check calls that using them is useless */ void uselessCalls(); + /** @brief %Check for derefencing an iterator that is invalid */ + void checkDereferenceInvalidIterator(); /** * Dereferencing an erased iterator @@ -179,6 +182,8 @@ private: void uselessCallsEmptyError(const Token *tok); void uselessCallsRemoveError(const Token *tok, const std::string& function); + void dereferenceInvalidIteratorError(const Token* deref, const std::string &itername); + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckStl c(0, settings, errorLogger); c.invalidIteratorError(0, "iterator"); @@ -205,6 +210,7 @@ private: c.uselessCallsSubstrError(0, false); c.uselessCallsEmptyError(0); c.uselessCallsRemoveError(0, "remove"); + c.dereferenceInvalidIteratorError(0, "i"); } static std::string myName() { @@ -223,7 +229,8 @@ private: "* redundant condition\n" "* common mistakes when using string::c_str()\n" "* using auto pointer (auto_ptr)\n" - "* useless calls of string and STL functions\n"; + "* useless calls of string and STL functions\n" + "* dereferencing an invalid iterator\n"; } }; /// @} diff --git a/test/teststl.cpp b/test/teststl.cpp index 9b602ab51..461af0938 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -125,6 +125,8 @@ private: TEST_CASE(uselessCalls); TEST_CASE(stabilityOfChecks); // #4684 cppcheck crash in template function call + TEST_CASE(dereferenceInvalidIterator); + } void check(const char code[], const bool inconclusive=false) { @@ -2246,6 +2248,112 @@ private: "};\n"); ASSERT_EQUALS("", errout.str()); } + + void dereferenceInvalidIterator() { + // Test simplest "if" with && case + check("void foo(std::string::iterator& i) {\n" + " if (std::isalpha(*i) && i != str.end()) {\n" + " std::cout << *i;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test suggested correction doesn't report an error + check("void foo(std::string::iterator& i) {\n" + " if (i != str.end() && std::isalpha(*i)) {\n" + " std::cout << *i;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Test "while" with "&&" case + check("void foo(std::string::iterator& i) {\n" + " while (std::isalpha(*i) && i != str.end()) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test "while" with "||" case + check("void foo(std::string::iterator& i) {\n" + " while (!(!std::isalpha(*i) || i == str.end())) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test fix for "while" with "||" case + check("void foo(std::string::iterator& i) {\n" + " while (!(i == str.end() || !std::isalpha(*i))) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Test "for" with "&&" case + check("void foo(std::string::iterator& i) {\n" + " for (; std::isalpha(*i) && i != str.end() ;) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test "for" with "||" case + check("void foo(std::string::iterator& i) {\n" + " for (; std::isalpha(*i) || i == str.end() ;) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test that a dereference outside the condition part of a "for" + // loop does not result in a false positive + check("void foo(std::string::iterator& i) {\n" + " for (char c = *i; isRunning && i != str.end() ;) {\n" + " std::cout << c;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Test that other "&&" terms in the condition don't invalidate the check + check("void foo(char* c, std::string::iterator& i) {\n" + " if (*c && std::isalpha(*i) && i != str.end()) {\n" + " std::cout << *i;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test that dereference of different variable doesn't trigger a false positive + check("void foo(const char* c, std::string::iterator& i) {\n" + " if (std::isalpha(*c) && i != str.end()) {\n" + " std::cout << *c;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // Test case involving "rend()" instead of "end()" + check("void foo(std::string::iterator& i) {\n" + " while (std::isalpha(*i) && i != str.rend()) {\n" + " std::cout << *i;\n" + " i ++;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + // Test that mixed "&&" and "||" don't result in a false positive + check("void foo(std::string::iterator& i) {\n" + " if ((i == str.end() || *i) || (isFoo() && i != str.end())) {\n" + " std::cout << \"foo\";\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestStl)