From 6ab3c93fb1852ee78fc2b7b84c0ebea0a7e0f396 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 28 Aug 2020 12:29:09 -0500 Subject: [PATCH] Fix issue 9756: false negative: invalid iterator from std::find_if (#2760) --- lib/checkstl.h | 2 +- lib/valueflow.cpp | 107 +++++++++++++++++++++++++++++++++++++++++----- lib/valueflow.h | 2 + test/teststl.cpp | 21 +++++++++ 4 files changed, 121 insertions(+), 11 deletions(-) diff --git a/lib/checkstl.h b/lib/checkstl.h index 6b9e5606c..19fe5725d 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -84,7 +84,7 @@ public: checkStl.knownEmptyContainerLoop(); checkStl.stlBoundaries(); - checkStl.checkDereferenceInvalidIterator(); + checkStl.checkDereferenceInvalidIterator(); checkStl.checkDereferenceInvalidIterator2(); checkStl.checkMutexes(); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 0dcd77418..84ce35938 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1703,6 +1703,14 @@ static void valueFlowGlobalStaticVar(TokenList *tokenList, const Settings *setti } } +static void valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + TokenList* const tokenlist, + const Settings* const settings); + +// Old deprecated version static void valueFlowForward(Token* startToken, const Token* endToken, const Token* exprTok, @@ -2696,6 +2704,17 @@ static void valueFlowForwardVariable(Token* const startToken, } } +static void valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + TokenList* const tokenlist, + const Settings* const settings) +{ + valueFlowForwardVariable(startToken, endToken, var, std::move(values), getAliasesFromValues(values), tokenlist, settings); +} + +// Old deprecated version static bool valueFlowForwardVariable(Token* const startToken, const Token* const endToken, const Variable* const var, @@ -2707,7 +2726,7 @@ static bool valueFlowForwardVariable(Token* const startToken, ErrorLogger* const, const Settings* const settings) { - valueFlowForwardVariable(startToken, endToken, var, std::move(values), getAliasesFromValues(values), tokenlist, settings); + valueFlowForwardVariable(startToken, endToken, var, std::move(values), tokenlist, settings); return true; } @@ -2857,10 +2876,7 @@ static void valueFlowForward(Token* startToken, const Token* endToken, const Token* exprTok, std::list values, - const bool constValue, - const bool subFunction, TokenList* const tokenlist, - ErrorLogger* const errorLogger, const Settings* settings) { const Token* expr = solveExprValues(exprTok, values); @@ -2868,18 +2884,28 @@ static void valueFlowForward(Token* startToken, valueFlowForwardVariable(startToken, endToken, expr->variable(), - expr->varId(), values, - constValue, - subFunction, tokenlist, - errorLogger, settings); } else { valueFlowForwardExpression(startToken, endToken, expr, values, tokenlist, settings); } } +// Old deprecated version +static void valueFlowForward(Token* startToken, + const Token* endToken, + const Token* exprTok, + std::list values, + const bool, + const bool, + TokenList* const tokenlist, + ErrorLogger* const, + const Settings* settings) +{ + valueFlowForward(startToken, endToken, exprTok, std::move(values), tokenlist, settings); +} + static int getArgumentPos(const Variable *var, const Function *f) { auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable &v) { @@ -4441,8 +4467,8 @@ static void valueFlowAfterCondition(TokenList *tokenlist, const Token* stop, const Token* vartok, const std::list& values, - bool constValue) { - valueFlowForward(start->next(), stop, vartok, values, constValue, false, tokenlist, errorLogger, settings); + bool) { + valueFlowForward(start->next(), stop, vartok, values, tokenlist, settings); std::vector vars = getExprVariables(vartok, tokenlist, symboldatabase, settings); return isVariablesChanged(start, stop, 0, vars, settings, tokenlist->isCPP()); }; @@ -5913,6 +5939,60 @@ static void valueFlowIterators(TokenList *tokenlist, const Settings *settings) } } +static std::list getIteratorValues(std::list values) +{ + values.remove_if([&](const ValueFlow::Value& v) { + return !v.isIteratorEndValue() && !v.isIteratorStartValue(); + }); + return values; +} + +static void valueFlowIteratorAfterCondition(TokenList *tokenlist, + SymbolDatabase *symboldatabase, + ErrorLogger *errorLogger, + const Settings *settings) +{ + ValueFlowConditionHandler handler; + handler.forward = [&](Token* start, + const Token* stop, + const Token* vartok, + const std::list& values, + bool) { + valueFlowForward(start->next(), stop, vartok, values, tokenlist, settings); + std::vector vars = getExprVariables(vartok, tokenlist, symboldatabase, settings); + return isVariablesChanged(start, stop, 0, vars, settings, tokenlist->isCPP()); + }; + handler.parse = [&](const Token *tok) { + ValueFlowConditionHandler::Condition cond; + + ValueFlow::Value true_value; + ValueFlow::Value false_value; + + if (Token::Match(tok, "==|!=")) { + if (!tok->astOperand1() || !tok->astOperand2()) + return cond; + + std::list values = getIteratorValues(tok->astOperand1()->values()); + if (!values.empty()) { + cond.vartok = tok->astOperand2(); + } else { + values = getIteratorValues(tok->astOperand2()->values()); + if (!values.empty()) + cond.vartok = tok->astOperand1(); + } + for(ValueFlow::Value& v:values) { + v.setPossible(); + v.assumeCondition(tok); + } + cond.true_values = values; + cond.false_values = values; + } + + return cond; + }; + handler.afterCondition(tokenlist, symboldatabase, errorLogger, settings); +} + static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings) { // declaration @@ -6411,6 +6491,12 @@ ValueFlow::Value::Value(const Token* c, long long val) errorPath.emplace_back(c, "Assuming that condition '" + c->expressionString() + "' is not redundant"); } +void ValueFlow::Value::assumeCondition(const Token* tok) +{ + condition = tok; + errorPath.emplace_back(tok, "Assuming that condition '" + tok->expressionString() + "' is not redundant"); +} + std::string ValueFlow::Value::infoString() const { switch (valueType) { @@ -6509,6 +6595,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, if (tokenlist->isCPP()) { valueFlowSmartPointer(tokenlist, errorLogger, settings); valueFlowIterators(tokenlist, settings); + valueFlowIteratorAfterCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings); valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings); } diff --git a/lib/valueflow.h b/lib/valueflow.h index d9df2a79e..669d00c27 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -170,6 +170,8 @@ namespace ValueFlow { decreaseRange(); } + void assumeCondition(const Token* tok); + std::string infoString() const; enum ValueType { INT, TOK, FLOAT, MOVED, UNINIT, CONTAINER_SIZE, LIFETIME, BUFFER_SIZE, ITERATOR_START, ITERATOR_END } valueType; diff --git a/test/teststl.cpp b/test/teststl.cpp index 5e4b92df4..40d09db56 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -3619,6 +3619,27 @@ private: " return *(v.begin() + pos);\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("int f(std::vector v, int i) {\n" + " auto it = std::find(v.begin(), v.end(), i);\n" + " if (it != v.end()) {}\n" + " return *it;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'it!=v.end()' is redundant or there is possible dereference of an invalid iterator: it.\n", errout.str()); + + check("void f(std::vector & v) {\n" + " std::vector::iterator i= v.begin();\n" + " if(i == v.end() && *(i+1) == *i) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n" + "[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i.\n", errout.str()); + + check("void f(std::vector & v) {\n" + " std::vector::iterator i= v.begin();\n" + " if(i == v.end() && *i == *(i+1)) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i.\n" + "[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n", errout.str()); } void dereferenceInvalidIterator2() {