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

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);
+            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.");
+}

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";
     }
 };
 /// @}

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)