Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #3372 (New check: dereference iterator and then checking it)

  • Loading branch information...
commit ecfe4eb5be569fd78050d4782015817a77668e28 1 parent fd7c90f
@zblair zblair authored
Showing with 185 additions and 1 deletion.
  1. +69 −0 lib/checkstl.cpp
  2. +8 −1 lib/checkstl.h
  3. +108 −0 test/teststl.cpp
View
69 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<Scope>& scopeList = _tokenizer->getSymbolDatabase()->scopeList;
+ for (std::list<Scope>::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);
@XhmikosR Collaborator
XhmikosR added a note

Not sure if GCC warns, but with MSVC 2012 I get:

..\lib\checkstl.cpp(1493): warning C4800: 'const Token *' : forcing value to bool 'true' or 'false' (performance warning) [C:\Users\xmr\Desktop\cppcheck\gui\cppcheck-gui-static.vcxproj]
..\lib\checkstl.cpp(1495): warning C4800: 'const Token *' : forcing value to bool 'true' or 'false' (performance warning) [C:\Users\xmr\Desktop\cppcheck\gui\cppcheck-gui-static.vcxproj]
@danmar Owner
danmar added a note

I don't understand how it can be performance. Is: "a=b;" slower than "a=(b!=NULL);"? Or are they suggesting to rewrite "b" completely then?

Please disable/ignore that warning for this code. The code looks ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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.");
+}
View
9 lib/checkstl.h
@@ -60,6 +60,7 @@ class CPPCHECKLIB CheckStl : public Check {
checkStl.string_c_str();
checkStl.checkAutoPointer();
checkStl.uselessCalls();
+ checkStl.checkDereferenceInvalidIterator();
// Style check
checkStl.size();
@@ -134,6 +135,8 @@ class CPPCHECKLIB CheckStl : public Check {
/** @brief %Check calls that using them is useless */
void uselessCalls();
+ /** @brief %Check for derefencing an iterator that is invalid */
@orbitcowboy Collaborator

typo: derefencing ==> dereferencing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ void checkDereferenceInvalidIterator();
/**
* Dereferencing an erased iterator
@@ -179,6 +182,8 @@ class CPPCHECKLIB CheckStl : public Check {
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 @@ class CPPCHECKLIB CheckStl : public Check {
c.uselessCallsSubstrError(0, false);
c.uselessCallsEmptyError(0);
c.uselessCallsRemoveError(0, "remove");
+ c.dereferenceInvalidIteratorError(0, "i");
}
static std::string myName() {
@@ -223,7 +229,8 @@ class CPPCHECKLIB CheckStl : public Check {
"* 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";
}
};
/// @}
View
108 test/teststl.cpp
@@ -125,6 +125,8 @@ class TestStl : public TestFixture {
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 @@ class TestStl : public TestFixture {
"};\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)
@XhmikosR
Collaborator

Not sure if GCC warns, but with MSVC 2012 I get:

..\lib\checkstl.cpp(1493): warning C4800: 'const Token *' : forcing value to bool 'true' or 'false' (performance warning) [C:\Users\xmr\Desktop\cppcheck\gui\cppcheck-gui-static.vcxproj]
..\lib\checkstl.cpp(1495): warning C4800: 'const Token *' : forcing value to bool 'true' or 'false' (performance warning) [C:\Users\xmr\Desktop\cppcheck\gui\cppcheck-gui-static.vcxproj]
@orbitcowboy
Collaborator

typo: derefencing ==> dereferencing

@danmar

I don't understand how it can be performance. Is: "a=b;" slower than "a=(b!=NULL);"? Or are they suggesting to rewrite "b" completely then?

Please disable/ignore that warning for this code. The code looks ok.

Please sign in to comment.
Something went wrong with that request. Please try again.