CheckStl: rewrite and refactor out of bounds checker

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)
 {

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);

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<int> 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<int> 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<int> 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<int> 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<int> 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)

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;
 }

test/testsuite.h

@@ -38,6 +38,8 @@ private:
     static std::size_t succeeded_todos_counter;
     static std::set<std::string> 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;