Fixed danmar#1693 (false negative: std::vector, negative index)

danmar · danmar · commit 2679b576c2fe · 2017-08-22T11:04:02.000+02:00
diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp
@@ -44,6 +44,7 @@ static const struct CWE CWE628(628U);   // Function Call with Incorrectly Specif
 static const struct CWE CWE664(664U);   // Improper Control of a Resource Through its Lifetime
 static const struct CWE CWE704(704U);   // Incorrect Type Conversion or Cast
 static const struct CWE CWE762(762U);   // Mismatched Memory Management Routines
+static const struct CWE CWE786(786U);   // Access of Memory Location Before Start of Buffer
 static const struct CWE CWE788(788U);   // Access of Memory Location After End of Buffer
 static const struct CWE CWE825(825U);   // Expired Pointer Dereference
 static const struct CWE CWE834(834U);   // Excessive Iteration
@@ -437,6 +438,42 @@ void CheckStl::stlOutOfBoundsError(const Token *tok, const std::string &num, con
         reportError(tok, Severity::error, "stlOutOfBounds", "When " + num + "==" + var + ".size(), " + var + "[" + num + "] is out of bounds.", CWE788, false);
 }
 
+void CheckStl::negativeIndex()
+{
+    // Negative index is out of bounds..
+    const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase();
+    const std::size_t functions = symbolDatabase->functionScopes.size();
+    for (std::size_t ii = 0; ii < functions; ++ii) {
+        const Scope * scope = symbolDatabase->functionScopes[ii];
+        for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
+            if (!Token::Match(tok, "%var% ["))
+                continue;
+            const Variable * const var = tok->variable();
+            if (!var || tok == var->nameToken())
+                continue;
+            const Library::Container * const container = _settings->library.detectContainer(var->typeStartToken());
+            if (!container || !container->arrayLike_indexOp)
+                continue;
+            const ValueFlow::Value *index = tok->next()->astOperand2()->getValueLE(-1, _settings);
+            if (!index)
+                continue;
+            negativeIndexError(tok, *index);
+        }
+    }
+}
+
+void CheckStl::negativeIndexError(const Token *tok, const ValueFlow::Value &index)
+{
+    const ErrorPath errorPath = getErrorPath(tok, &index, "Negative array index");
+    std::ostringstream errmsg;
+    if (index.condition)
+        errmsg << ValueFlow::eitherTheConditionIsRedundant(index.condition)
+               << ", otherwise there is negative array index " << index.intvalue << ".";
+    else
+        errmsg << "Array index " << index.intvalue << " is out of bounds.";
+    reportError(errorPath, index.errorSeverity() ? Severity::error : Severity::warning, "negativeContainerIndex", errmsg.str(), CWE786, index.inconclusive);
+}
+
 void CheckStl::erase()
 {
     const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase();
diff --git a/lib/checkstl.h b/lib/checkstl.h
@@ -61,6 +61,7 @@ class CPPCHECKLIB CheckStl : public Check {
         CheckStl checkStl(tokenizer, settings, errorLogger);
 
         checkStl.stlOutOfBounds();
+        checkStl.negativeIndex();
         checkStl.iterators();
         checkStl.mismatchingContainers();
         checkStl.erase();
@@ -86,6 +87,11 @@ class CPPCHECKLIB CheckStl : public Check {
      */
     void stlOutOfBounds();
 
+    /**
+     * negative index for array like containers
+     */
+    void negativeIndex();
+
     /**
      * Finds errors like this:
      * for (it = foo.begin(); it != bar.end(); ++it)
@@ -171,6 +177,7 @@ class CPPCHECKLIB CheckStl : public Check {
     void string_c_strParam(const Token *tok, unsigned int number);
 
     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);
     void iteratorsError(const Token *tok, const std::string &container1, const std::string &container2);
     void mismatchingContainersError(const Token *tok);
@@ -203,6 +210,7 @@ class CPPCHECKLIB CheckStl : public Check {
         c.mismatchingContainersError(nullptr);
         c.dereferenceErasedError(nullptr, nullptr, "iter");
         c.stlOutOfBoundsError(nullptr, "i", "foo", false);
+        c.negativeIndexError(nullptr, ValueFlow::Value(-1));
         c.invalidIteratorError(nullptr, "push_back|push_front|insert", "iterator");
         c.invalidPointerError(nullptr, "push_back", "pointer");
         c.stlBoundariesError(nullptr);
diff --git a/test/teststl.cpp b/test/teststl.cpp
@@ -60,6 +60,7 @@ class TestStl : public TestFixture {
 
         TEST_CASE(STLSize);
         TEST_CASE(STLSizeNoErr);
+        TEST_CASE(negativeIndex);
         TEST_CASE(erase1);
         TEST_CASE(erase2);
         TEST_CASE(erase3);
@@ -720,7 +721,12 @@ class TestStl : public TestFixture {
         }
     }
 
-
+    void negativeIndex() {
+        check("void f(const std::vector<int> &v) {\n"
+              "  v[-11] = 123;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (error) Array index -11 is out of bounds.\n", errout.str());
+    }
 
 
 
