From b17807c5684d87e88154d8fd0e92117f0fb3de41 Mon Sep 17 00:00:00 2001 From: amai2012 Date: Wed, 10 Jan 2018 09:37:21 +0100 Subject: [PATCH] #6572 False positive eraseDereference - in iterator class - flag error inconclusive if iterator is not STL type --- lib/checkstl.cpp | 30 ++++++++++++--------- lib/checkstl.h | 70 +++++++++++++++++++++++++----------------------- test/teststl.cpp | 28 ++++++++++++++++++- 3 files changed, 81 insertions(+), 47 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 75fabdc72..8dad38190 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -61,7 +61,7 @@ void CheckStl::iteratorsError(const Token *tok, const std::string &container1, c } // Error message used when dereferencing an iterator that has been erased.. -void CheckStl::dereferenceErasedError(const Token *erased, const Token* deref, const std::string &itername) +void CheckStl::dereferenceErasedError(const Token *erased, const Token* deref, const std::string &itername, bool inconclusive) { if (erased) { std::list callstack; @@ -70,12 +70,12 @@ void CheckStl::dereferenceErasedError(const Token *erased, const Token* deref, c reportError(callstack, Severity::error, "eraseDereference", "Iterator '" + itername + "' used after element has been erased.\n" "The iterator '" + itername + "' is invalid after the element it pointed to has been erased. " - "Dereferencing or comparing it with another iterator is invalid operation.", CWE664, false); + "Dereferencing or comparing it with another iterator is invalid operation.", CWE664, inconclusive); } else { reportError(deref, Severity::error, "eraseDereference", "Invalid iterator '" + itername + "' used.\n" "The iterator '" + itername + "' is invalid before being assigned. " - "Dereferencing or comparing it with another iterator is invalid operation.", CWE664, false); + "Dereferencing or comparing it with another iterator is invalid operation.", CWE664, inconclusive); } } @@ -86,7 +86,7 @@ static const Token *skipMembers(const Token *tok) return tok; } -static bool isIterator(const Variable *var) +static bool isIterator(const Variable *var, bool& inconclusiveType) { // Check that its an iterator if (!var || !var->isLocal() || !Token::Match(var->typeEndToken(), "iterator|const_iterator|reverse_iterator|const_reverse_iterator|auto")) @@ -99,8 +99,11 @@ static bool isIterator(const Variable *var) // look for operator* and operator++ const Function* end = var->type()->getFunction("operator*"); const Function* incOperator = var->type()->getFunction("operator++"); - if (!end || end->argCount() > 0 || !incOperator) + if (!end || end->argCount() > 0 || !incOperator) { return false; + } else { + inconclusiveType = true; // heuristics only + } } return true; @@ -126,7 +129,8 @@ void CheckStl::iterators() for (unsigned int iteratorId = 1; iteratorId < symbolDatabase->getVariableListSize(); iteratorId++) { const Variable* var = symbolDatabase->getVariableFromVarId(iteratorId); - if (!isIterator(var)) + bool inconclusiveType=false; + if (!isIterator(var, inconclusiveType)) continue; // the validIterator flag says if the iterator has a valid value or not @@ -202,7 +206,8 @@ void CheckStl::iterators() while (par2->str() != ")") { if (par2->varId() == containerToken->varId()) break; - if (isIterator(par2->variable())) + bool inconclusiveType2=false; + if (isIterator(par2->variable(), inconclusiveType2)) break; // TODO: check if iterator points at same container if (par2->str() == "(") par2 = par2->link(); @@ -266,10 +271,10 @@ void CheckStl::iterators() // Dereferencing invalid iterator? else if (!validIterator && Token::Match(tok2, "* %varid%", iteratorId)) { - dereferenceErasedError(eraseToken, tok2, tok2->strAt(1)); + dereferenceErasedError(eraseToken, tok2, tok2->strAt(1), inconclusiveType); tok2 = tok2->next(); } else if (!validIterator && Token::Match(tok2, "%varid% . %name%", iteratorId)) { - dereferenceErasedError(eraseToken, tok2, tok2->str()); + dereferenceErasedError(eraseToken, tok2, tok2->str(), inconclusiveType); tok2 = tok2->tokAt(2); } @@ -514,7 +519,8 @@ void CheckStl::erase() void CheckStl::eraseCheckLoopVar(const Scope &scope, const Variable *var) { - if (!var || !Token::simpleMatch(var->typeEndToken(), "iterator")) + bool inconclusiveType=false; + if (!isIterator(var, inconclusiveType)) return; for (const Token *tok = scope.classStart; tok != scope.classEnd; tok = tok->next()) { if (tok->str() != "(") @@ -541,14 +547,14 @@ void CheckStl::eraseCheckLoopVar(const Scope &scope, const Variable *var) if (tok2->varId() == var->declarationId()) { if (Token::simpleMatch(tok2->next(), "=")) break; - dereferenceErasedError(tok, tok2, tok2->str()); + dereferenceErasedError(tok, tok2, tok2->str(), inconclusiveType); break; } if (indentlevel == 0U && Token::Match(tok2, "break|return|goto")) break; } if (tok2 == scope.classEnd) - dereferenceErasedError(tok, scope.classDef, var->nameToken()->str()); + dereferenceErasedError(tok, scope.classDef, var->nameToken()->str(), inconclusiveType); } } diff --git a/lib/checkstl.h b/lib/checkstl.h index 48d3b5083..7b615bd22 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -49,14 +49,15 @@ public: } /** This constructor is used when running checks. */ - CheckStl(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) + CheckStl(const Tokenizer* tokenizer, const Settings* settings, ErrorLogger* errorLogger) : Check(myName(), tokenizer, settings, errorLogger) { } /** Simplified checks. The token list is simplified. */ - void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { - if (!tokenizer->isCPP()) + void runSimplifiedChecks(const Tokenizer* tokenizer, const Settings* settings, ErrorLogger* errorLogger) { + if (!tokenizer->isCPP()) { return; + } CheckStl checkStl(tokenizer, settings, errorLogger); @@ -109,7 +110,7 @@ public: * it is bad to dereference it after the erase. */ void erase(); - void eraseCheckLoopVar(const Scope &scope, const Variable *var); + void eraseCheckLoopVar(const Scope& scope, const Variable* var); /** @@ -161,8 +162,9 @@ public: * @param erased token where the erase occurs * @param deref token where the dereference occurs * @param itername iterator name + * @param inconclusive inconclusive flag */ - void dereferenceErasedError(const Token* erased, const Token* deref, const std::string &itername); + void dereferenceErasedError(const Token* erased, const Token* deref, const std::string& itername, bool inconclusive); /** @brief Reading from empty stl container */ void readingEmptyStlContainer(); @@ -170,45 +172,45 @@ public: private: void readingEmptyStlContainer_parseUsage(const Token* tok, const Library::Container* container, std::map& empty, bool noerror); - void missingComparisonError(const Token *incrementToken1, const Token *incrementToken2); - void string_c_strThrowError(const Token *tok); - void string_c_strError(const Token *tok); - void string_c_strReturn(const Token *tok); - void string_c_strParam(const Token *tok, unsigned int number); + void missingComparisonError(const Token* incrementToken1, const Token* incrementToken2); + void string_c_strThrowError(const Token* tok); + void string_c_strError(const Token* tok); + void string_c_strReturn(const Token* tok); + void string_c_strParam(const Token* tok, unsigned int number); - void stlOutOfBoundsError(const Token *tok, const std::string &num, const std::string &var, bool at); - void negativeIndexError(const Token *tok, const ValueFlow::Value &index); - void invalidIteratorError(const Token *tok, const std::string &iteratorName); - void iteratorsError(const Token *tok, const std::string &container1, const std::string &container2); - void mismatchingContainersError(const Token *tok); - void invalidIteratorError(const Token *tok, const std::string &func, const std::string &iterator_name); - void invalidPointerError(const Token *tok, const std::string &func, const std::string &pointer_name); - void stlBoundariesError(const Token *tok); - void if_findError(const Token *tok, bool str); - void sizeError(const Token *tok); - void redundantIfRemoveError(const Token *tok); + void stlOutOfBoundsError(const Token* tok, const std::string& num, const std::string& var, bool at); + void negativeIndexError(const Token* tok, const ValueFlow::Value& index); + void invalidIteratorError(const Token* tok, const std::string& iteratorName); + void iteratorsError(const Token* tok, const std::string& container1, const std::string& container2); + void mismatchingContainersError(const Token* tok); + void invalidIteratorError(const Token* tok, const std::string& func, const std::string& iterator_name); + void invalidPointerError(const Token* tok, const std::string& func, const std::string& pointer_name); + void stlBoundariesError(const Token* tok); + void if_findError(const Token* tok, bool str); + void sizeError(const Token* tok); + void redundantIfRemoveError(const Token* tok); - void autoPointerError(const Token *tok); - void autoPointerContainerError(const Token *tok); - void autoPointerArrayError(const Token *tok); - void autoPointerMallocError(const Token *tok, const std::string& allocFunction); + void autoPointerError(const Token* tok); + void autoPointerContainerError(const Token* tok); + void autoPointerArrayError(const Token* tok); + void autoPointerMallocError(const Token* tok, const std::string& allocFunction); - void uselessCallsReturnValueError(const Token *tok, const std::string &varname, const std::string &function); - void uselessCallsSwapError(const Token *tok, const std::string &varname); - void uselessCallsSubstrError(const Token *tok, bool empty); - void uselessCallsEmptyError(const Token *tok); - void uselessCallsRemoveError(const Token *tok, const std::string& function); + void uselessCallsReturnValueError(const Token* tok, const std::string& varname, const std::string& function); + void uselessCallsSwapError(const Token* tok, const std::string& varname); + void uselessCallsSubstrError(const Token* tok, bool empty); + void uselessCallsEmptyError(const Token* tok); + void uselessCallsRemoveError(const Token* tok, const std::string& function); - void dereferenceInvalidIteratorError(const Token* deref, const std::string &iterName); + void dereferenceInvalidIteratorError(const Token* deref, const std::string& iterName); - void readingEmptyStlContainerError(const Token *tok); + void readingEmptyStlContainerError(const Token* tok); - void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { + void getErrorMessages(ErrorLogger* errorLogger, const Settings* settings) const { CheckStl c(nullptr, settings, errorLogger); c.invalidIteratorError(nullptr, "iterator"); c.iteratorsError(nullptr, "container1", "container2"); c.mismatchingContainersError(nullptr); - c.dereferenceErasedError(nullptr, nullptr, "iter"); + c.dereferenceErasedError(nullptr, nullptr, "iter", false); c.stlOutOfBoundsError(nullptr, "i", "foo", false); c.negativeIndexError(nullptr, ValueFlow::Value(-1)); c.invalidIteratorError(nullptr, "push_back|push_front|insert", "iterator"); diff --git a/test/teststl.cpp b/test/teststl.cpp index b9e9db7c1..f0b1ef5b8 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -137,6 +137,7 @@ private: TEST_CASE(stabilityOfChecks); // #4684 cppcheck crash in template function call TEST_CASE(dereferenceInvalidIterator); + TEST_CASE(dereferenceInvalidIterator2); // #6572 TEST_CASE(dereference_auto); TEST_CASE(readingEmptyStlContainer); @@ -1600,7 +1601,7 @@ private: " iterator i;\n" " return i.foo();;\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) Invalid iterator 'i' used.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (error, inconclusive) Invalid iterator 'i' used.\n", errout.str()); } void stlBoundaries6() { // #7106 @@ -2895,6 +2896,31 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + void dereferenceInvalidIterator2() { + // Self-implemented iterator class + check("class iterator {\n" + "public:\n" + " CCommitPointer m_ptr;\n" + " iterator() {}\n" + " CCommitPointer& operator*() {\n" + " return m_ptr;\n" + " }\n" + " CCommitPointer* operator->() {\n" + " return &m_ptr;\n" + " }\n" + " iterator& operator++() {\n" + " ++m_ptr.m_place;\n" + " return *this;\n" + " }\n" + " }; \n" + " iterator begin() {\n" + " iterator it; \n" + " it->m_place = 0;\n" + " return it; \n" + "}\n"); + ASSERT_EQUALS("[test.cpp:18]: (error, inconclusive) Invalid iterator 'it' used.\n", errout.str()); + } void readingEmptyStlContainer() { check("void f() {\n"