From faaabb1a05f831d69378172f7e127f44383e111d Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sun, 4 Feb 2024 22:16:28 +0100 Subject: [PATCH] New check: eraseIteratorOutOfBounds (#5913) --- lib/astutils.cpp | 4 ++- lib/checkstl.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++ lib/checkstl.h | 7 +++++ releasenotes.txt | 2 +- test/teststl.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 152 insertions(+), 4 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 2c8efaa31f3..c8eebebb0f3 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -3027,8 +3027,10 @@ const Token* findExpressionChangedSkipDeadCode(const Token* expr, const Token* getArgumentStart(const Token* ftok) { const Token* tok = ftok; - if (Token::Match(tok, "%name% (|{")) + if (Token::Match(tok, "%name% (|{|)")) tok = ftok->next(); + while (Token::simpleMatch(tok, ")")) + tok = tok->next(); if (!Token::Match(tok, "(|{|[")) return nullptr; const Token* startTok = tok->astOperand2(); diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 26fb07c5945..708b2bf41ae 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -3119,6 +3119,77 @@ void CheckStl::knownEmptyContainer() } } +void CheckStl::eraseIteratorOutOfBoundsError(const Token *ftok, const Token* itertok, const ValueFlow::Value* val) +{ + if (!ftok || !itertok || !val) { + reportError(ftok, Severity::error, "eraseIteratorOutOfBounds", + "Calling function 'erase()' on the iterator 'iter' which is out of bounds.", CWE628, Certainty::normal); + reportError(ftok, Severity::warning, "eraseIteratorOutOfBoundsCond", + "Either the condition 'x' is redundant or function 'erase()' is called on the iterator 'iter' which is out of bounds.", CWE628, Certainty::normal); + return; + } + const std::string& func = ftok->str(); + const std::string iter = itertok->expressionString(); + + const bool isConditional = val->isPossible(); + std::string msg; + if (isConditional) { + msg = ValueFlow::eitherTheConditionIsRedundant(val->condition) + " or function '" + func + "()' is called on the iterator '" + iter + "' which is out of bounds."; + } else { + msg = "Calling function '" + func + "()' on the iterator '" + iter + "' which is out of bounds."; + } + + const Severity severity = isConditional ? Severity::warning : Severity::error; + const std::string id = isConditional ? "eraseIteratorOutOfBoundsCond" : "eraseIteratorOutOfBounds"; + reportError(ftok, severity, + id, + msg, CWE628, Certainty::normal); +} + +static const ValueFlow::Value* getOOBIterValue(const Token* tok, const ValueFlow::Value* sizeVal) +{ + auto it = std::find_if(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& v) { + if (v.isPossible() || v.isKnown()) { + switch (v.valueType) { + case ValueFlow::Value::ValueType::ITERATOR_END: + return v.intvalue >= 0; + case ValueFlow::Value::ValueType::ITERATOR_START: + return (v.intvalue < 0) || (sizeVal && v.intvalue >= sizeVal->intvalue); + default: + break; + } + } + return false; + }); + return it != tok->values().end() ? &*it : nullptr; +} + +void CheckStl::eraseIteratorOutOfBounds() +{ + logChecker("CheckStl::eraseIteratorOutOfBounds"); + for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { + for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + + if (!tok->valueType()) + continue; + const Library::Container* container = tok->valueType()->container; + if (!container || !astIsLHS(tok) || !Token::simpleMatch(tok->astParent(), ".")) + continue; + const Token* const ftok = tok->astParent()->astOperand2(); + const Library::Container::Action action = container->getAction(ftok->str()); + if (action != Library::Container::Action::ERASE) + continue; + const std::vector args = getArguments(ftok); + if (args.size() != 1) // TODO: check range overload + continue; + + const ValueFlow::Value* sizeVal = tok->getKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE); + if (const ValueFlow::Value* errVal = getOOBIterValue(args[0], sizeVal)) + eraseIteratorOutOfBoundsError(ftok, args[0], errVal); + } + } +} + static bool isMutex(const Variable* var) { const Token* tok = Token::typeDecl(var->nameToken()).first; diff --git a/lib/checkstl.h b/lib/checkstl.h index 6694c3cdd16..ec476bb5156 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -78,6 +78,7 @@ class CPPCHECKLIB CheckStl : public Check { checkStl.mismatchingContainers(); checkStl.mismatchingContainerIterator(); checkStl.knownEmptyContainer(); + checkStl.eraseIteratorOutOfBounds(); checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -183,6 +184,8 @@ class CPPCHECKLIB CheckStl : public Check { void knownEmptyContainer(); + void eraseIteratorOutOfBounds(); + void checkMutexes(); bool isContainerSize(const Token *containerToken, const Token *expr) const; @@ -234,6 +237,8 @@ class CPPCHECKLIB CheckStl : public Check { void knownEmptyContainerError(const Token *tok, const std::string& algo); + void eraseIteratorOutOfBoundsError(const Token* ftok, const Token* itertok, const ValueFlow::Value* val = nullptr); + void globalLockGuardError(const Token *tok); void localMutexError(const Token *tok); @@ -271,6 +276,7 @@ class CPPCHECKLIB CheckStl : public Check { c.uselessCallsEmptyError(nullptr); c.uselessCallsRemoveError(nullptr, "remove"); c.dereferenceInvalidIteratorError(nullptr, "i"); + c.eraseIteratorOutOfBoundsError(nullptr, nullptr); c.useStlAlgorithmError(nullptr, emptyString); c.knownEmptyContainerError(nullptr, emptyString); c.globalLockGuardError(nullptr); @@ -296,6 +302,7 @@ class CPPCHECKLIB CheckStl : public Check { "- common mistakes when using string::c_str()\n" "- useless calls of string and STL functions\n" "- dereferencing an invalid iterator\n" + "- erasing an iterator that is out of bounds\n" "- reading from empty STL container\n" "- iterating over an empty STL container\n" "- consider using an STL algorithm instead of raw loop\n" diff --git a/releasenotes.txt b/releasenotes.txt index 6b313cce25d..3cf24efdb0f 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -1,7 +1,7 @@ Release Notes for Cppcheck 2.14 New checks: -- +eraseIteratorOutOfBounds: warns when erase() is called on an iterator that is out of bounds Improved checking: - diff --git a/test/teststl.cpp b/test/teststl.cpp index 67eb86f4a6a..6768e78d81a 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -74,6 +74,7 @@ class TestStl : public TestFixture { TEST_CASE(iteratorExpression); TEST_CASE(iteratorSameExpression); TEST_CASE(mismatchingContainerIterator); + TEST_CASE(eraseIteratorOutOfBounds); TEST_CASE(dereference); TEST_CASE(dereference_break); // #3644 - handle "break" @@ -2160,6 +2161,69 @@ class TestStl : public TestFixture { ASSERT_EQUALS("", errout.str()); } + void eraseIteratorOutOfBounds() { + check("void f() {\n" + " std::vector v;\n" + " v.erase(v.begin());\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()' which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " v.erase(v.end());\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.end()' which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " auto it = v.begin();\n" + " v.erase(it);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Calling function 'erase()' on the iterator 'it' which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " std::vector v{ 1, 2, 3 };\n" + " auto it = v.end();\n" + " v.erase(it);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Calling function 'erase()' on the iterator 'it' which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " std::vector v{ 1, 2, 3 };\n" + " auto it = v.begin();\n" + " v.erase(it);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " std::vector v{ 1, 2, 3 };\n" + " v.erase(v.end() - 1);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " std::vector v{ 1, 2, 3 };\n" + " v.erase(v.begin() - 1);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()-1' which is out of bounds.\n" + "[test.cpp:3]: (error) Dereference of an invalid iterator: v.begin()-1\n", + errout.str()); + + check("void f(std::vector& v, std::vector::iterator it) {\n" + " if (it == v.end()) {}\n" + " v.erase(it);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Either the condition 'it==v.end()' is redundant or function 'erase()' is called on the iterator 'it' which is out of bounds.\n", + errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " ((v).erase)(v.begin());\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Calling function 'erase()' on the iterator 'v.begin()' which is out of bounds.\n", + errout.str()); + } + // Dereferencing invalid pointer void dereference() { check("void f()\n" @@ -2219,7 +2283,9 @@ class TestStl : public TestFixture { " ints.erase(iter);\n" " std::cout << iter->first << std::endl;\n" "}"); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n" + "[test.cpp:6]: (error) Calling function 'erase()' on the iterator 'iter' which is out of bounds.\n", + errout.str()); // Reverse iterator check("void f()\n" @@ -2230,7 +2296,9 @@ class TestStl : public TestFixture { " ints.erase(iter);\n" " std::cout << iter->first << std::endl;\n" "}"); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (error) Iterator 'iter' used after element has been erased.\n" + "[test.cpp:6]: (error) Calling function 'erase()' on the iterator 'iter' which is out of bounds.\n", + errout.str()); } void dereference_auto() {