From 86ed860d26cd8e8fc47d5b2b6f3029c73d9149d9 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 6 Jun 2020 10:54:56 -0500 Subject: [PATCH] Fix issue 9548: False negative: Mismatching iterators when inserting into a vector (#2595) --- lib/checkstl.cpp | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ lib/checkstl.h | 5 +++++ test/teststl.cpp | 16 ++++++++++++++ 3 files changed, 77 insertions(+) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 0a04cb7b5..581f34db0 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -27,6 +27,7 @@ #include "utils.h" #include "astutils.h" #include "pathanalysis.h" +#include "valueflow.h" #include #include @@ -534,6 +535,18 @@ void CheckStl::iterators() } } +void CheckStl::mismatchingContainerIteratorError(const Token* tok, const Token* iterTok) +{ + const std::string container(tok ? tok->expressionString() : std::string("v1")); + const std::string iter(iterTok ? iterTok->expressionString() : std::string("it")); + reportError(tok, + Severity::error, + "mismatchingContainerIterator", + "Iterator '" + iter + "' from different container '" + container + "' are used together.", + CWE664, + false); +} + // Error message for bad iterator usage.. void CheckStl::mismatchingContainersError(const Token* tok1, const Token* tok2) { @@ -708,6 +721,49 @@ void CheckStl::mismatchingContainers() } } +void CheckStl::mismatchingContainerIterator() +{ + // Check if different containers are used in various calls of standard functions + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope * scope : symbolDatabase->functionScopes) { + for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { + if (!astIsContainer(tok)) + continue; + if (!Token::Match(tok, "%var% . %name% ( !!)")) + continue; + const Token * const ftok = tok->tokAt(2); + const std::vector args = getArguments(ftok); + + const Library::Container * c = tok->valueType()->container; + Library::Container::Action action = c->getAction(tok->strAt(2)); + const Token* iterTok = nullptr; + if (action == Library::Container::Action::INSERT && args.size() == 2) { + // Skip if iterator pair + if (astIsIterator(args.back())) + continue; + if (!astIsIterator(args.front())) + continue; + iterTok = args.front(); + } else if (action == Library::Container::Action::ERASE) { + if (!astIsIterator(args.front())) + continue; + iterTok = args.front(); + } else { + continue; + } + + ValueFlow::Value val = getLifetimeObjValue(iterTok); + if (!val.tokvalue) + continue; + if (val.lifetimeKind != ValueFlow::Value::LifetimeKind::Iterator) + continue; + if (isSameExpression(true, false, tok, val.tokvalue, mSettings->library, false, false)) + continue; + mismatchingContainerIteratorError(tok, iterTok); + } + } +} + static bool isInvalidMethod(const Token * tok) { if (Token::Match(tok->next(), ". assign|clear|swap")) diff --git a/lib/checkstl.h b/lib/checkstl.h index d332d0b61..23ea56242 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -80,6 +80,7 @@ public: checkStl.invalidContainer(); checkStl.invalidContainerLoop(); checkStl.mismatchingContainers(); + checkStl.mismatchingContainerIterator(); checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -122,6 +123,8 @@ public: * std::find(foo.begin(), bar.end(), x) */ void mismatchingContainers(); + + void mismatchingContainerIterator(); /** * Dangerous usage of erase. The iterator is invalidated by erase so @@ -201,6 +204,7 @@ private: void iteratorsError(const Token* tok, const std::string& containerName1, const std::string& containerName2); void iteratorsError(const Token* tok, const Token* containerTok, const std::string& containerName1, const std::string& containerName2); void iteratorsError(const Token* tok, const Token* containerTok, const std::string& containerName); + void mismatchingContainerIteratorError(const Token* tok, const Token* iterTok); void mismatchingContainersError(const Token* tok1, const Token* tok2); void mismatchingContainerExpressionError(const Token *tok1, const Token *tok2); void sameIteratorExpressionError(const Token *tok); @@ -235,6 +239,7 @@ private: c.iteratorsError(nullptr, nullptr, "container"); c.invalidContainerLoopError(nullptr, nullptr); c.invalidContainerError(nullptr, nullptr, nullptr, errorPath); + c.mismatchingContainerIteratorError(nullptr, nullptr); c.mismatchingContainersError(nullptr, nullptr); c.mismatchingContainerExpressionError(nullptr, nullptr); c.sameIteratorExpressionError(nullptr); diff --git a/test/teststl.cpp b/test/teststl.cpp index 4126296af..824274c8d 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -70,6 +70,7 @@ private: TEST_CASE(iterator25); // #9742 TEST_CASE(iteratorExpression); TEST_CASE(iteratorSameExpression); + TEST_CASE(mismatchingContainerIterator); TEST_CASE(dereference); TEST_CASE(dereference_break); // #3644 - handle "break" @@ -1277,6 +1278,21 @@ private: ASSERT_EQUALS("[test.cpp:2]: (style) Same iterators expression are used for algorithm.\n", errout.str()); } + void mismatchingContainerIterator() { + check("std::vector to_vector(int value) {\n" + " std::vector a, b;\n" + " a.insert(b.end(), value);\n" + " return a;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Iterator 'b.end()' from different container 'a' are used together.\n", errout.str()); + + check("std::vector f(std::vector a, std::vector b) {\n" + " a.erase(b.begin());\n" + " return a;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (error) Iterator 'b.begin()' from different container 'a' are used together.\n", errout.str()); + } + // Dereferencing invalid pointer void dereference() { check("void f()\n"