From 81f54f70944dc79dcb76bb3dfaff21746020746a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 10 Aug 2018 11:29:16 +0200 Subject: [PATCH] Fixed #8681 (ValueFlow: Container size) --- lib/checkstl.cpp | 38 ++++++++++++++++++++++++++++++++++-- lib/checkstl.h | 6 +++++- lib/token.cpp | 44 ++++++++++++++++++++++++------------------ lib/token.h | 10 ++++++++++ lib/valueflow.cpp | 36 ++++++++++++++++++++++++++++++++++ lib/valueflow.h | 8 +++++++- test/teststl.cpp | 10 ++++++++++ test/testvalueflow.cpp | 27 ++++++++++++++++++++++++++ 8 files changed, 156 insertions(+), 23 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 6174ca421..206dbb8ed 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -1718,8 +1718,42 @@ void CheckStl::readingEmptyStlContainer() } } -void CheckStl::readingEmptyStlContainerError(const Token *tok) + +void CheckStl::readingEmptyStlContainer2() +{ + for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { + for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + if (!tok->isName() || !tok->valueType()) + continue; + const Library::Container *container = tok->valueType()->container; + if (!container) + continue; + const ValueFlow::Value *value = tok->getContainerSizeValue(0); + if (!value) + continue; + if (value->isInconclusive() && !mSettings->inconclusive) + continue; + if (!value->errorSeverity() && !mSettings->isEnabled(Settings::WARNING)) + continue; + if (Token::Match(tok, "%name% . %name% (")) { + if (container->getYield(tok->strAt(2)) == Library::Container::Yield::ITEM) + readingEmptyStlContainerError(tok,value); + } + } + } +} + +void CheckStl::readingEmptyStlContainerError(const Token *tok, const ValueFlow::Value *value) { const std::string varname = tok ? tok->str() : std::string("var"); - reportError(tok, Severity::style, "reademptycontainer", "$symbol:" + varname +"\nReading from empty STL container '$symbol'", CWE398, true); + + std::string errmsg; + if (value && value->condition) + errmsg = "Reading from container '$symbol'. " + ValueFlow::eitherTheConditionIsRedundant(value->condition) + " or '$symbol' can be empty."; + else + errmsg = "Reading from empty STL container '$symbol'"; + + const ErrorPath errorPath = getErrorPath(tok, value, "Reading from empty container"); + + reportError(errorPath, value ? (value->errorSeverity() ? Severity::error : Severity::warning) : Severity::style, "reademptycontainer", "$symbol:" + varname +"\n" + errmsg, CWE398, !value); } diff --git a/lib/checkstl.h b/lib/checkstl.h index b867a767e..11f89f27d 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -79,6 +79,7 @@ public: checkStl.redundantCondition(); checkStl.missingComparison(); checkStl.readingEmptyStlContainer(); + checkStl.readingEmptyStlContainer2(); } @@ -169,6 +170,9 @@ public: /** @brief Reading from empty stl container */ void readingEmptyStlContainer(); + + /** @brief Reading from empty stl container (using valueflow) */ + void readingEmptyStlContainer2(); private: void readingEmptyStlContainer_parseUsage(const Token* tok, const Library::Container* container, std::map& empty, bool noerror); @@ -205,7 +209,7 @@ private: void dereferenceInvalidIteratorError(const Token* deref, const std::string& iterName); - void readingEmptyStlContainerError(const Token* tok); + void readingEmptyStlContainerError(const Token* tok, const ValueFlow::Value *value=nullptr); void getErrorMessages(ErrorLogger* errorLogger, const Settings* settings) const override { CheckStl c(nullptr, settings, errorLogger); diff --git a/lib/token.cpp b/lib/token.cpp index 69636b155..b12888252 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1374,62 +1374,68 @@ void Token::printValueFlow(bool xml, std::ostream &out) const if (tok->mValues->size() > 1U) out << '{'; } - for (std::list::const_iterator it=tok->mValues->begin(); it!=tok->mValues->end(); ++it) { + for (const ValueFlow::Value &value : *tok->mValues) { if (xml) { out << " valueType) { + switch (value.valueType) { case ValueFlow::Value::INT: if (tok->valueType() && tok->valueType()->sign == ValueType::UNSIGNED) - out << "intvalue=\"" << (MathLib::biguint)it->intvalue << '\"'; + out << "intvalue=\"" << (MathLib::biguint)value.intvalue << '\"'; else - out << "intvalue=\"" << it->intvalue << '\"'; + out << "intvalue=\"" << value.intvalue << '\"'; break; case ValueFlow::Value::TOK: - out << "tokvalue=\"" << it->tokvalue << '\"'; + out << "tokvalue=\"" << value.tokvalue << '\"'; break; case ValueFlow::Value::FLOAT: - out << "floatvalue=\"" << it->floatValue << '\"'; + out << "floatvalue=\"" << value.floatValue << '\"'; break; case ValueFlow::Value::MOVED: - out << "movedvalue=\"" << ValueFlow::Value::toString(it->moveKind) << '\"'; + out << "movedvalue=\"" << ValueFlow::Value::toString(value.moveKind) << '\"'; break; case ValueFlow::Value::UNINIT: out << "uninit=\"1\""; break; + case ValueFlow::Value::CONTAINER_SIZE: + out << "container-size=\"" << value.intvalue << '\"'; + break; } - if (it->condition) - out << " condition-line=\"" << it->condition->linenr() << '\"'; - if (it->isKnown()) + if (value.condition) + out << " condition-line=\"" << value.condition->linenr() << '\"'; + if (value.isKnown()) out << " known=\"true\""; - else if (it->isPossible()) + else if (value.isPossible()) out << " possible=\"true\""; - else if (it->isInconclusive()) + else if (value.isInconclusive()) out << " inconclusive=\"true\""; out << "/>" << std::endl; } else { - if (it != tok->mValues->begin()) + if (&value != &tok->mValues->front()) out << ","; - switch (it->valueType) { + switch (value.valueType) { case ValueFlow::Value::INT: if (tok->valueType() && tok->valueType()->sign == ValueType::UNSIGNED) - out << (MathLib::biguint)it->intvalue; + out << (MathLib::biguint)value.intvalue; else - out << it->intvalue; + out << value.intvalue; break; case ValueFlow::Value::TOK: - out << it->tokvalue->str(); + out << value.tokvalue->str(); break; case ValueFlow::Value::FLOAT: - out << it->floatValue; + out << value.floatValue; break; case ValueFlow::Value::MOVED: - out << ValueFlow::Value::toString(it->moveKind); + out << ValueFlow::Value::toString(value.moveKind); break; case ValueFlow::Value::UNINIT: out << "Uninit"; break; + case ValueFlow::Value::CONTAINER_SIZE: + out << "size=" << value.intvalue; + break; } } } diff --git a/lib/token.h b/lib/token.h index 6134827ec..a2a974253 100644 --- a/lib/token.h +++ b/lib/token.h @@ -875,6 +875,16 @@ public: const ValueFlow::Value * getInvalidValue(const Token *ftok, unsigned int argnr, const Settings *settings) const; + const ValueFlow::Value * getContainerSizeValue(const MathLib::bigint val) const { + if (!mValues) + return nullptr; + for (const ValueFlow::Value &value : *mValues) { + if (value.isContainerSizeValue() && value.intvalue == val) + return &value; + } + return nullptr; + } + const Token *getValueTokenMaxStrLength() const; const Token *getValueTokenMinStrSize() const; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 66a77c488..34b7776d5 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3413,6 +3413,38 @@ static void valueFlowUninit(TokenList *tokenlist, SymbolDatabase * /*symbolDatab } } +static void valueFlowContainerReverse(const Token *tok, unsigned int containerId, const ValueFlow::Value &value, const Settings *settings) +{ + while (nullptr != (tok = tok->previous())) { + if (Token::Match(tok, "[{}]")) + break; + if (tok->varId() != containerId) + continue; + if (!tok->valueType() || !tok->valueType()->container) + continue; + setTokenValue(const_cast(tok), value, settings); + } +} + +static void valueFlowContainerSize(TokenList * /*tokenlist*/, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings) +{ + for (const Scope &scope : symboldatabase->scopeList) { + if (scope.type != Scope::ScopeType::eIf) // TODO: while + continue; + for (const Token *tok = scope.classDef; tok && tok->str() != "{"; tok = tok->next()) { + if (!tok->isName() || !tok->valueType() || tok->valueType()->type != ValueType::CONTAINER) + continue; + if (!Token::Match(tok, "%name% . %name% (")) + continue; + if (tok->valueType()->container->getYield(tok->strAt(2)) != Library::Container::Yield::EMPTY) + continue; + ValueFlow::Value value(tok->tokAt(3), 0LL); + value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + valueFlowContainerReverse(scope.classDef, tok->varId(), value, settings); + } + } +} + ValueFlow::Value::Value(const Token *c, long long val) : valueType(INT), intvalue(val), @@ -3442,6 +3474,8 @@ std::string ValueFlow::Value::infoString() const return ""; case UNINIT: return ""; + case CONTAINER_SIZE: + return "size=" + MathLib::toString(intvalue); }; throw InternalError(nullptr, "Invalid ValueFlow Value type"); } @@ -3479,6 +3513,8 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowSubFunction(tokenlist, errorLogger, settings); valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings); valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings); + if (tokenlist->isCPP()) + valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings); } diff --git a/lib/valueflow.h b/lib/valueflow.h index 22f5a82cb..905dad7cc 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -65,6 +65,9 @@ namespace ValueFlow { break; case UNINIT: break; + case CONTAINER_SIZE: + if (intvalue != rhs.intvalue) + return false; }; return varvalue == rhs.varvalue && @@ -77,7 +80,7 @@ namespace ValueFlow { std::string infoString() const; - enum ValueType { INT, TOK, FLOAT, MOVED, UNINIT } valueType; + enum ValueType { INT, TOK, FLOAT, MOVED, UNINIT, CONTAINER_SIZE } valueType; bool isIntValue() const { return valueType == INT; } @@ -93,6 +96,9 @@ namespace ValueFlow { bool isUninitValue() const { return valueType == UNINIT; } + bool isContainerSizeValue() const { + return valueType == CONTAINER_SIZE; + } /** int value */ long long intvalue; diff --git a/test/teststl.cpp b/test/teststl.cpp index 71d180ac6..3b86ab609 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -143,6 +143,7 @@ private: TEST_CASE(dereference_auto); TEST_CASE(readingEmptyStlContainer); + TEST_CASE(readingEmptyStlContainer2); } void check(const char code[], const bool inconclusive=false, const Standards::cppstd_t cppstandard=Standards::CPP11) { @@ -3254,6 +3255,15 @@ private: "}", true); ASSERT_EQUALS("", errout.str()); } + + void readingEmptyStlContainer2() { + check("void f(std::vector v) {\n" + " v.front();\n" + " if (v.empty());\n" + "}\n",true); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Reading from container 'v'. Either the condition 'v.empty()' is redundant or 'v' can be empty.\n", errout.str()); + + } }; REGISTER_TEST(TestStl) diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 1532e3942..c322ecb1f 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -103,6 +103,8 @@ private: TEST_CASE(valueFlowInlineAssembly); TEST_CASE(valueFlowUninit); + + TEST_CASE(valueFlowContainerSize); } bool testValueOfX(const char code[], unsigned int linenr, int value) { @@ -3202,6 +3204,31 @@ private: ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible()); ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0); } + + void valueFlowContainerSize() { + const char *code; + std::list values; + + LOAD_LIB_2(settings.library, "std.cfg"); + + // valueFlowContainerReverse + code = "void f(const std::list &ints) {\n" + " ints.front();\n" + " if (ints.empty()) {}\n" + "}"; + values = tokenValues(code, "ints . front"); + ASSERT_EQUALS(1, values.size()); + ASSERT_EQUALS(true, values.empty() ? true : values.front().isContainerSizeValue()); + ASSERT_EQUALS(0, values.empty() ? 0 : values.front().intvalue); + + code = "void f(std::list ints) {\n" + " ints.front();\n" + " ints.pop_back();\n" + " if (ints.empty()) {}\n" + "}"; + values = tokenValues(code, "ints . front"); + ASSERT_EQUALS(true, values.empty()); + } }; REGISTER_TEST(TestValueFlow)