diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 206dbb8ed..4aafb0122 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -50,6 +50,103 @@ static const struct CWE CWE788(788U); // Access of Memory Location After End o static const struct CWE CWE825(825U); // Expired Pointer Dereference static const struct CWE CWE834(834U); // Excessive Iteration + + +void CheckStl::outOfBounds() +{ + 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; + for (const ValueFlow::Value &value : tok->values()) { + if (!value.isContainerSizeValue()) + continue; + if (value.isInconclusive() && !mSettings->inconclusive) + continue; + if (!value.errorSeverity() && !mSettings->isEnabled(Settings::WARNING)) + continue; + if (value.intvalue == 0 && Token::Match(tok, "%name% . %name% (") && container->getYield(tok->strAt(2)) == Library::Container::Yield::ITEM) { + outOfBoundsError(tok, &value, nullptr); + continue; + } + if (value.intvalue == 0 && Token::Match(tok, "%name% [")) { + outOfBoundsError(tok, &value, nullptr); + continue; + } + if (container->arrayLike_indexOp && Token::Match(tok, "%name% [")) { + const ValueFlow::Value *indexValue = tok->next()->astOperand2() ? tok->next()->astOperand2()->getMaxValue(false) : nullptr; + if (indexValue && indexValue->intvalue >= value.intvalue) { + outOfBoundsError(tok, &value, indexValue); + continue; + } + if (mSettings->isEnabled(Settings::WARNING)) { + indexValue = tok->next()->astOperand2() ? tok->next()->astOperand2()->getMaxValue(true) : nullptr; + if (indexValue && indexValue->intvalue >= value.intvalue) { + outOfBoundsError(tok, &value, indexValue); + continue; + } + } + } + } + } + } +} + +void CheckStl::outOfBoundsError(const Token *tok, const ValueFlow::Value *containerSize, const ValueFlow::Value *index) +{ + // Do not warn if both the container size and index are possible + if (containerSize && index && containerSize->isPossible() && index->isPossible()) + return; + + const std::string varname = tok ? tok->str() : std::string("var"); + + std::string errmsg; + if (!containerSize) + errmsg = "Out of bounds access of item in container '$symbol'"; + else if (containerSize->intvalue == 0) { + if (containerSize->condition) + errmsg = "Accessing an item in container '$symbol'. " + ValueFlow::eitherTheConditionIsRedundant(containerSize->condition) + " or '$symbol' can be empty."; + else + errmsg = "Accessing an item in container '$symbol' that is empty."; + } else { + if (containerSize->condition || index->condition) + errmsg = "Possible access out of bounds"; + else + errmsg = "Access out of bounds"; + + errmsg += " of container '$symbol'; size=" + + MathLib::toString(containerSize->intvalue) + ", index=" + + MathLib::toString(index->intvalue); + } + + ErrorPath errorPath; + if (!index) + errorPath = getErrorPath(tok, containerSize, "Access out of bounds"); + else { + ErrorPath errorPath1 = getErrorPath(tok, containerSize, "Access out of bounds"); + ErrorPath errorPath2 = getErrorPath(tok, index, "Access out of bounds"); + if (errorPath1.size() <= 1) + errorPath = errorPath2; + else if (errorPath2.size() <= 1) + errorPath = errorPath1; + else { + errorPath = errorPath1; + errorPath.splice(errorPath.end(), errorPath2); + } + } + + reportError(errorPath, + (containerSize && !containerSize->errorSeverity()) || (index && !index->errorSeverity()) ? Severity::warning : Severity::error, + "containerOutOfBounds", + "$symbol:" + varname +"\n" + errmsg, + CWE398, + (containerSize && containerSize->isInconclusive()) || (index && index->isInconclusive())); +} + + // Error message for bad iterator usage.. void CheckStl::invalidIteratorError(const Token *tok, const std::string &iteratorName) { diff --git a/lib/checkstl.h b/lib/checkstl.h index 11f89f27d..6c4da60a7 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -53,6 +53,16 @@ public: : Check(myName(), tokenizer, settings, errorLogger) { } + /** run checks, the token list is not simplified */ + virtual void runChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger) { + if (!tokenizer->isCPP()) { + return; + } + + CheckStl checkStl(tokenizer, settings, errorLogger); + checkStl.outOfBounds(); + } + /** Simplified checks. The token list is simplified. */ void runSimplifiedChecks(const Tokenizer* tokenizer, const Settings* settings, ErrorLogger* errorLogger) override { if (!tokenizer->isCPP()) { @@ -79,9 +89,10 @@ public: checkStl.redundantCondition(); checkStl.missingComparison(); checkStl.readingEmptyStlContainer(); - checkStl.readingEmptyStlContainer2(); } + /** Accessing container out of bounds using ValueFlow */ + void outOfBounds(); /** * Finds errors like this: @@ -182,6 +193,7 @@ private: void string_c_strReturn(const Token* tok); void string_c_strParam(const Token* tok, unsigned int number); + void outOfBoundsError(const Token *tok, const ValueFlow::Value *containerSize, const ValueFlow::Value *index); 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); @@ -213,6 +225,7 @@ private: void getErrorMessages(ErrorLogger* errorLogger, const Settings* settings) const override { CheckStl c(nullptr, settings, errorLogger); + c.outOfBoundsError(nullptr, nullptr, nullptr); c.invalidIteratorError(nullptr, "iterator"); c.iteratorsError(nullptr, "container1", "container2"); c.mismatchingContainersError(nullptr); diff --git a/test/teststl.cpp b/test/teststl.cpp index 3b86ab609..798512953 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -40,6 +40,8 @@ private: settings.addEnabled("performance"); LOAD_LIB_2(settings.library, "std.cfg"); + TEST_CASE(outOfBounds); + TEST_CASE(iterator1); TEST_CASE(iterator2); TEST_CASE(iterator3); @@ -143,7 +145,6 @@ 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) { @@ -167,6 +168,55 @@ private: check(code.c_str(), inconclusive); } + void checkNormal(const char code[]) { + // Clear the error buffer.. + errout.str(""); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Check.. + CheckStl checkStl(&tokenizer, &settings, this); + checkStl.runChecks(&tokenizer, &settings, this); + } + + void outOfBounds() { + setMultiline(); + + checkNormal("void f(std::vector v) {\n" + " v.front();\n" + " if (v.empty()) {}\n" + "}\n"); + ASSERT_EQUALS("test.cpp:2:warning:Accessing an item in container 'v'. Either the condition 'v.empty()' is redundant or 'v' can be empty.\n" + "test.cpp:3:note:condition 'v.empty()'\n" + "test.cpp:2:note:Access out of bounds\n", errout.str()); + + checkNormal("void f(std::vector v) {\n" + " if (v.size() == 3) {}\n" + " v[16] = 0;\n" + "}\n"); + ASSERT_EQUALS("test.cpp:3:warning:Possible access out of bounds of container 'v'; size=3, index=16\n" + "test.cpp:2:note:condition 'v.size()==3'\n" + "test.cpp:3:note:Access out of bounds\n", errout.str()); + + checkNormal("void f(std::vector v) {\n" + " int i = 16;\n" + " if (v.size() == 3) {\n" + " v[i] = 0;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("test.cpp:4:warning:Possible access out of bounds of container 'v'; size=3, index=16\n" + "test.cpp:3:note:condition 'v.size()==3'\n" + "test.cpp:4:note:Access out of bounds\n", errout.str()); + + checkNormal("void f(std::vector v, int i) {\n" + " if (v.size() == 3 || i == 16) {}\n" + " v[i] = 0;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } void iterator1() { check("void f()\n" @@ -3255,15 +3305,6 @@ 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/testsuite.cpp b/test/testsuite.cpp index e9939bfca..38851f64a 100644 --- a/test/testsuite.cpp +++ b/test/testsuite.cpp @@ -83,6 +83,10 @@ TestFixture::TestFixture(const char * const _name) bool TestFixture::prepareTest(const char testname[]) { + mVerbose = false; + mTemplateFormat.clear(); + mTemplateLocation.clear(); + // Check if tests should be executed if (testToRun.empty() || testToRun == testname) { // Tests will be executed - prepare them @@ -330,7 +334,7 @@ void TestFixture::reportOut(const std::string & outmsg) void TestFixture::reportErr(const ErrorLogger::ErrorMessage &msg) { - const std::string errormessage(msg.toString(mVerbose)); + const std::string errormessage(msg.toString(mVerbose, mTemplateFormat, mTemplateLocation)); if (errout.str().find(errormessage) == std::string::npos) errout << errormessage << std::endl; } diff --git a/test/testsuite.h b/test/testsuite.h index affa15b46..90569a7cc 100644 --- a/test/testsuite.h +++ b/test/testsuite.h @@ -38,6 +38,8 @@ private: static std::size_t succeeded_todos_counter; static std::set missingLibs; bool mVerbose; + std::string mTemplateFormat; + std::string mTemplateLocation; protected: std::string testToRun; @@ -71,6 +73,11 @@ protected: mVerbose = v; } + void setMultiline() { + mTemplateFormat = "{file}:{line}:{severity}:{message}"; + mTemplateLocation = "{file}:{line}:note:{info}"; + } + void processOptions(const options& args); public: virtual void reportOut(const std::string &outmsg) override;