From 7776fb82a2bad3ec5c82343ada7ebfef146c14cc Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 17 Aug 2020 16:36:45 -0500 Subject: [PATCH] Fix issue 737: new check: Dereference end iterator --- lib/checknullpointer.cpp | 8 ++--- lib/checkstl.cpp | 63 ++++++++++++++++++++++++++++++++++++++++ lib/checkstl.h | 3 ++ lib/token.cpp | 14 +++++++++ lib/valueflow.cpp | 59 +++++++++++++++++++++++++++++++------ lib/valueflow.h | 27 ++++++++++------- test/teststl.cpp | 55 +++++++++++++++++++++++++++++++++++ 7 files changed, 205 insertions(+), 24 deletions(-) diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 324d7a6ce..11cebd655 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -172,11 +172,9 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set if (parent->str() == "." && parent->astOperand2() == tok) return isPointerDeRef(parent, unknown, settings); const bool firstOperand = parent->astOperand1() == tok; - while (parent->str() == "(" && (parent->astOperand2() == nullptr && parent->strAt(1) != ")")) { // Skip over casts - parent = parent->astParent(); - if (!parent) - return false; - } + parent = astParentSkipParens(tok); + if (!parent) + return false; // Dereferencing pointer.. if (parent->isUnaryOp("*") && !Token::Match(parent->tokAt(-2), "sizeof|decltype|typeof")) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index cf7ec70c1..856682b98 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -18,6 +18,8 @@ #include "checkstl.h" +#include "check.h" +#include "checknullpointer.h" #include "library.h" #include "mathlib.h" #include "settings.h" @@ -2011,6 +2013,67 @@ void CheckStl::checkDereferenceInvalidIterator() } } +void CheckStl::checkDereferenceInvalidIterator2() +{ + const bool printInconclusive = (mSettings->inconclusive); + + for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "sizeof|decltype|typeid|typeof (")) { + tok = tok->next()->link(); + continue; + } + + // Can iterator point to END or before START? + for(const ValueFlow::Value& value:tok->values()) { + if (!printInconclusive && value.isInconclusive()) + continue; + if (!value.isIteratorValue()) + continue; + if (value.isIteratorEndValue() && value.intvalue < 0) + continue; + if (value.isIteratorStartValue() && value.intvalue >= 0) + continue; + bool unknown = false; + if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings)) { + if (unknown) + dereferenceInvalidIteratorError(tok, &value, true); + continue; + } + dereferenceInvalidIteratorError(tok, &value, false); + } + } +} + +void CheckStl::dereferenceInvalidIteratorError(const Token* tok, const ValueFlow::Value *value, bool inconclusive) +{ + const std::string& varname = tok ? tok->expressionString() : "var"; + const std::string errmsgcond("$symbol:" + varname + '\n' + ValueFlow::eitherTheConditionIsRedundant(value ? value->condition : nullptr) + " or there is possible dereference of an invalid iterator: $symbol."); + if (!tok || !value) { + reportError(tok, Severity::error, "derefInvalidIterator", "Dereference of an invalid iterator", CWE825, false); + reportError(tok, Severity::warning, "derefInvalidIteratorRedundantCheck", errmsgcond, CWE825, false); + return; + } + if (!mSettings->isEnabled(value, inconclusive)) + return; + + const ErrorPath errorPath = getErrorPath(tok, value, "Dereference of an invalid iterator"); + + if (value->condition) { + reportError(errorPath, Severity::warning, "derefInvalidIteratorRedundantCheck", errmsgcond, CWE825, inconclusive || value->isInconclusive()); + } else { + std::string errmsg; + errmsg = std::string(value->isKnown() ? "Dereference" : "Possible dereference") + " of an invalid iterator"; + if (!varname.empty()) + errmsg = "$symbol:" + varname + '\n' + errmsg + ": $symbol"; + + reportError(errorPath, + value->isKnown() ? Severity::error : Severity::warning, + "derefInvalidIterator", + errmsg, + CWE825, inconclusive || value->isInconclusive()); + } +} + void CheckStl::dereferenceInvalidIteratorError(const Token* deref, const std::string &iterName) { reportError(deref, Severity::warning, diff --git a/lib/checkstl.h b/lib/checkstl.h index 9dd5e07d5..55256a39f 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -84,6 +84,7 @@ public: checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); + checkStl.checkDereferenceInvalidIterator2(); checkStl.checkMutexes(); // Style check @@ -171,6 +172,7 @@ public: /** @brief %Check for dereferencing an iterator that is invalid */ void checkDereferenceInvalidIterator(); + void checkDereferenceInvalidIterator2(); /** * Dereferencing an erased iterator @@ -227,6 +229,7 @@ private: void uselessCallsRemoveError(const Token* tok, const std::string& function); void dereferenceInvalidIteratorError(const Token* deref, const std::string& iterName); + void dereferenceInvalidIteratorError(const Token* tok, const ValueFlow::Value *value, bool inconclusive); void readingEmptyStlContainerError(const Token* tok, const ValueFlow::Value *value=nullptr); diff --git a/lib/token.cpp b/lib/token.cpp index 6a3f7f9b8..0d3dafccb 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1631,6 +1631,12 @@ void Token::printValueFlow(bool xml, std::ostream &out) const case ValueFlow::Value::CONTAINER_SIZE: out << "container-size=\"" << value.intvalue << '\"'; break; + case ValueFlow::Value::ITERATOR_START: + out << "iterator-start=\"" << value.intvalue << '\"'; + break; + case ValueFlow::Value::ITERATOR_END: + out << "iterator-end=\"" << value.intvalue << '\"'; + break; case ValueFlow::Value::LIFETIME: out << "lifetime=\"" << value.tokvalue << '\"'; break; @@ -1680,6 +1686,12 @@ void Token::printValueFlow(bool xml, std::ostream &out) const case ValueFlow::Value::CONTAINER_SIZE: out << "size=" << value.intvalue; break; + case ValueFlow::Value::ITERATOR_START: + out << "start=" << value.intvalue; + break; + case ValueFlow::Value::ITERATOR_END: + out << "end=" << value.intvalue; + break; case ValueFlow::Value::LIFETIME: out << "lifetime=" << value.tokvalue->str(); break; @@ -1958,6 +1970,8 @@ bool Token::addValue(const ValueFlow::Value &value) case ValueFlow::Value::ValueType::INT: case ValueFlow::Value::ValueType::CONTAINER_SIZE: case ValueFlow::Value::ValueType::BUFFER_SIZE: + case ValueFlow::Value::ValueType::ITERATOR_START: + case ValueFlow::Value::ValueType::ITERATOR_END: differentValue = (it->intvalue != value.intvalue); break; case ValueFlow::Value::ValueType::TOK: diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 09a424174..00e173375 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -347,6 +347,10 @@ static void combineValueProperties(const ValueFlow::Value &value1, const ValueFl result->setInconclusive(); else result->setPossible(); + if (value1.isIteratorValue()) + result->valueType = value1.valueType; + if (value2.isIteratorValue()) + result->valueType = value2.valueType; result->condition = value1.condition ? value1.condition : value2.condition; result->varId = (value1.varId != 0U) ? value1.varId : value2.varId; result->varvalue = (result->varId == value1.varId) ? value1.varvalue : value2.varvalue; @@ -376,6 +380,20 @@ static const Token *getCastTypeStartToken(const Token *parent) return nullptr; } +static bool isComputableValue(const Token* parent, const ValueFlow::Value& value) +{ + const bool noninvertible = parent->isComparisonOp() || Token::Match(parent, "%|/|&|%or%"); + if (noninvertible && value.isImpossible()) + return false; + if (!value.isIntValue() && !value.isFloatValue() && !value.isTokValue() && !value.isIteratorValue()) + return false; + if (value.isIteratorValue() && !Token::Match(parent, "+|-")) + return false; + if (value.isTokValue() && (!parent->isComparisonOp() || value.tokvalue->tokType() != Token::eString)) + return false; + return true; +} + /** Set token value for cast */ static void setTokenValueCast(Token *parent, const ValueType &valueType, const ValueFlow::Value &value, const Settings *settings); @@ -564,20 +582,14 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti } for (const ValueFlow::Value &value1 : parent->astOperand1()->values()) { - if (noninvertible && value1.isImpossible()) - continue; - if (!value1.isIntValue() && !value1.isFloatValue() && !value1.isTokValue()) - continue; - if (value1.isTokValue() && (!parent->isComparisonOp() || value1.tokvalue->tokType() != Token::eString)) + if (!isComputableValue(parent, value1)) continue; for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) { if (value1.path != value2.path) continue; - if (noninvertible && value2.isImpossible()) + if (!isComputableValue(parent, value2)) continue; - if (!value2.isIntValue() && !value2.isFloatValue() && !value2.isTokValue()) - continue; - if (value2.isTokValue() && (!parent->isComparisonOp() || value2.tokvalue->tokType() != Token::eString || value1.isTokValue())) + if (value1.isIteratorValue() && value2.isIteratorValue()) continue; if (value1.isKnown() || value2.isKnown() || value1.varId == 0U || value2.varId == 0U || (value1.varId == value2.varId && value1.varvalue == value2.varvalue && value1.isIntValue() && @@ -5803,6 +5815,30 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge } } +static void valueFlowIterators(TokenList *tokenlist, ErrorLogger * errorLogger, const Settings *settings) +{ + for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { + if (!tok->scope()) + continue; + if (!tok->scope()->isExecutable()) + continue; + if (!astIsContainer(tok)) + continue; + if (Token::Match(tok->astParent(), ". %name% (")) { + Library::Container::Yield yield = tok->valueType()->container->getYield(tok->astParent()->strAt(1)); + ValueFlow::Value v(0); + v.setKnown(); + if (yield == Library::Container::Yield::START_ITERATOR) { + v.valueType = ValueFlow::Value::ITERATOR_START; + setTokenValue(tok->astParent()->tokAt(2), v, settings); + } else if (yield == Library::Container::Yield::END_ITERATOR) { + v.valueType = ValueFlow::Value::ITERATOR_END; + setTokenValue(tok->astParent()->tokAt(2), v, settings); + } + } + } +} + static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings) { // declaration @@ -6314,6 +6350,10 @@ std::string ValueFlow::Value::infoString() const case BUFFER_SIZE: case CONTAINER_SIZE: return "size=" + MathLib::toString(intvalue); + case ITERATOR_START: + return "start=" + MathLib::toString(intvalue); + case ITERATOR_END: + return "end=" + MathLib::toString(intvalue); case LIFETIME: return "lifetime=" + tokvalue->str(); } @@ -6391,6 +6431,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings); if (tokenlist->isCPP()) { valueFlowSmartPointer(tokenlist, errorLogger, settings); + valueFlowIterators(tokenlist, errorLogger, settings); valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings); valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings); } diff --git a/lib/valueflow.h b/lib/valueflow.h index 7f68cd953..139c25af2 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -82,6 +82,10 @@ namespace ValueFlow { return false; switch (valueType) { case ValueType::INT: + case ValueType::CONTAINER_SIZE: + case ValueType::BUFFER_SIZE: + case ValueType::ITERATOR_START: + case ValueType::ITERATOR_END: if (intvalue != rhs.intvalue) return false; break; @@ -100,14 +104,6 @@ namespace ValueFlow { break; case ValueType::UNINIT: break; - case ValueType::BUFFER_SIZE: - if (intvalue != rhs.intvalue) - return false; - break; - case ValueType::CONTAINER_SIZE: - if (intvalue != rhs.intvalue) - return false; - break; case ValueType::LIFETIME: if (tokvalue != rhs.tokvalue) return false; @@ -120,7 +116,9 @@ namespace ValueFlow { switch (valueType) { case ValueType::INT: case ValueType::BUFFER_SIZE: - case ValueType::CONTAINER_SIZE: { + case ValueType::CONTAINER_SIZE: + case ValueType::ITERATOR_START: + case ValueType::ITERATOR_END: { f(intvalue); break; } @@ -174,7 +172,7 @@ namespace ValueFlow { std::string infoString() const; - enum ValueType { INT, TOK, FLOAT, MOVED, UNINIT, CONTAINER_SIZE, LIFETIME, BUFFER_SIZE } valueType; + enum ValueType { INT, TOK, FLOAT, MOVED, UNINIT, CONTAINER_SIZE, LIFETIME, BUFFER_SIZE, ITERATOR_START, ITERATOR_END } valueType; bool isIntValue() const { return valueType == ValueType::INT; } @@ -199,6 +197,15 @@ namespace ValueFlow { bool isBufferSizeValue() const { return valueType == ValueType::BUFFER_SIZE; } + bool isIteratorValue() const { + return valueType == ValueType::ITERATOR_START || valueType == ValueType::ITERATOR_END; + } + bool isIteratorStartValue() const { + return valueType == ValueType::ITERATOR_START; + } + bool isIteratorEndValue() const { + return valueType == ValueType::ITERATOR_END; + } bool isLocalLifetimeValue() const { return valueType == ValueType::LIFETIME && lifetimeScope == LifetimeScope::Local; diff --git a/test/teststl.cpp b/test/teststl.cpp index ae812f3bf..e0f1bb01b 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -3515,6 +3515,61 @@ private: " }\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " std::vector ::iterator i = v.end();\n" + " *i=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Dereference of an invalid iterator: i\n", errout.str()); + + check("void f(std::vector v) {\n" + " std::vector ::iterator i = v.end();\n" + " *i=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Dereference of an invalid iterator: i\n", errout.str()); + + check("void f(std::vector v) {\n" + " std::vector ::iterator i = v.end();\n" + " *(i+1)=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Dereference of an invalid iterator: i+1\n", errout.str()); + + check("void f(std::vector v) {\n" + " std::vector ::iterator i = v.end();\n" + " *(i-1)=0;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(std::vector v) {\n" + " std::vector ::iterator i = v.begin();\n" + " *(i-1)=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (error) Dereference of an invalid iterator: i-1\n", errout.str()); + + check("void f(std::vector v, bool b) {\n" + " std::vector ::iterator i = v.begin();\n" + " if (b)\n" + " i = v.end();\n" + " *i=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Possible dereference of an invalid iterator: i\n", errout.str()); + + check("void f(std::vector v, bool b) {\n" + " std::vector ::iterator i = v.begin();\n" + " if (b)\n" + " i = v.end();\n" + " *(i+1)=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Possible dereference of an invalid iterator: i+1\n", errout.str()); + + check("void f(std::vector v, bool b) {\n" + " std::vector ::iterator i = v.begin();\n" + " if (b)\n" + " i = v.end();\n" + " *(i-1)=0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Possible dereference of an invalid iterator: i-1\n", errout.str()); } void dereferenceInvalidIterator2() {