ValueFlow: Improved valueflow of for loop 'for (i=a; i<10; i++)' => unknown start value but end value is known

lib/checkbufferoverrun.cpp

@@ -961,6 +961,16 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
             return;
     }
 
+    // ValueFlow array index..
+    if ((declarationId > 0 && Token::Match(tok, "%varid% [", declarationId)) ||
+        (declarationId == 0 && Token::Match(tok, (varnames + " [").c_str()))) {
+
+        const Token *tok2 = tok;
+        while (tok2->str() != "[")
+            tok2 = tok2->next();
+        valueFlowCheckArrayIndex(tok2, arrayInfo);
+    }
+
     // Array index..
     if ((declarationId > 0 && Token::Match(tok, "%varid% [ %num% ]", declarationId)) ||
         (declarationId == 0 && Token::Match(tok, (varnames + " [ %num% ]").c_str()))) {
@@ -1165,6 +1175,114 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
     }
 }
 
+void CheckBufferOverrun::valueFlowCheckArrayIndex(const Token * const tok, const ArrayInfo &arrayInfo)
+{
+    // Taking address?
+    bool addressOf = false;
+    {
+        const Token *tok2 = tok->astParent();
+        while (Token::Match(tok2, "%var%|.|::"))
+            tok2 = tok2->astParent();
+        addressOf = Token::Match(tok2, "&") && !(tok2->astOperand1() && tok2->astOperand2());
+    }
+
+    // Look for errors first
+    for (int warn = 0; warn == 0 || warn == 1; ++warn) {
+        // Negative index..
+        for (const Token *tok2 = tok; tok2 && tok2->str() == "["; tok2 = tok2->link()->next()) {
+            const Token *index = tok2->astOperand2();
+            if (!index)
+                continue;
+            std::list<ValueFlow::Value>::const_iterator it;
+            const ValueFlow::Value *val = nullptr;
+            for (it = index->values.begin(); it != index->values.end(); ++it) {
+                if (it->intvalue < 0) {
+                    val = &*it;
+                    if (val->condition == nullptr)
+                        break;
+                }
+            }
+            if (val && !val->condition)
+                negativeIndexError(index, val->intvalue);
+        }
+
+        // Index out of bounds..
+        std::vector<ValueFlow::Value> indexes;
+        unsigned int valuevarid = 0;
+        for (const Token *tok2 = tok; indexes.size() < arrayInfo.num().size() && Token::Match(tok2, "["); tok2 = tok2->link()->next()) {
+            if (!tok2->astOperand2()) {
+                indexes.clear();
+                break;
+            }
+            const ValueFlow::Value *value = tok2->astOperand2()->getMaxValue(warn == 1);
+            if (!value) {
+                indexes.clear();
+                break;
+            }
+            if (valuevarid == 0U)
+                valuevarid = value->varId;
+            if (value->varId > 0 && valuevarid != value->varId) {
+                indexes.clear();
+                break;
+            }
+            if (value->intvalue < 0) {
+                indexes.clear();
+                break;
+            }
+            indexes.push_back(*value);
+        }
+        if (indexes.size() == arrayInfo.num().size()) {
+            // Check if the indexes point outside the whole array..
+            // char a[10][10];
+            // a[0][20]  <-- ok.
+            // a[9][20]  <-- error.
+
+            // total number of elements of array..
+            MathLib::bigint totalElements = 1;
+
+            // total index..
+            MathLib::bigint totalIndex = 0;
+
+            // calculate the totalElements and totalIndex..
+            for (unsigned int i = 0; i < indexes.size(); ++i) {
+                std::size_t ri = indexes.size() - 1 - i;
+                totalIndex += indexes[ri].intvalue * totalElements;
+                totalElements *= arrayInfo.num(ri);
+            }
+
+            // totalElements == 0 => Unknown size
+            if (totalElements == 0)
+                continue;
+
+            // taking address of 1 past end?
+            if (addressOf && totalIndex == totalElements)
+                continue;
+
+            // Is totalIndex in bounds?
+            if (totalIndex >= totalElements) {
+                arrayIndexOutOfBoundsError(tok, arrayInfo, indexes);
+                break;
+            }
+
+            // Is any array index out of bounds?
+            else {
+                // check each index for overflow
+                for (unsigned int i = 0; i < indexes.size(); ++i) {
+                    if (indexes[i].intvalue >= arrayInfo.num(i)) {
+                        // The access is still within the memory range for the array
+                        // so it may be intentional.
+                        if (_settings->inconclusive) {
+                            arrayIndexOutOfBoundsError(tok, arrayInfo, indexes);
+                            break; // only warn about the first one
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+
 
 void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo)
 {
@@ -1181,108 +1299,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
         }
 
         else if (Token::Match(tok, "%varid% [", arrayInfo.declarationId())) {
-            // Look for errors first
+            valueFlowCheckArrayIndex(tok->next(), arrayInfo);
-            for (int warn = 0; warn == 0 || warn == 1; ++warn) {
-                // Negative index..
-                for (const Token *tok2 = tok->next(); tok2 && tok2->str() == "["; tok2 = tok2->link()->next()) {
-                    const Token *index = tok2->astOperand2();
-                    if (!index)
-                        continue;
-                    std::list<ValueFlow::Value>::const_iterator it;
-                    const ValueFlow::Value *val = nullptr;
-                    for (it = index->values.begin(); it != index->values.end(); ++it) {
-                        if (it->intvalue < 0) {
-                            val = &*it;
-                            if (val->condition == nullptr)
-                                break;
-                        }
-                    }
-                    if (val && !val->condition)
-                        negativeIndexError(index, val->intvalue);
-                }
-
-                // Index out of bounds..
-                std::vector<ValueFlow::Value> indexes;
-                unsigned int valuevarid = 0;
-                for (const Token *tok2 = tok->next(); indexes.size() < arrayInfo.num().size() && Token::Match(tok2, "["); tok2 = tok2->link()->next()) {
-                    if (!tok2->astOperand2()) {
-                        indexes.clear();
-                        break;
-                    }
-                    const ValueFlow::Value *value = tok2->astOperand2()->getMaxValue(warn == 1);
-                    if (!value) {
-                        indexes.clear();
-                        break;
-                    }
-                    if (valuevarid == 0U)
-                        valuevarid = value->varId;
-                    if (value->varId > 0 && valuevarid != value->varId) {
-                        indexes.clear();
-                        break;
-                    }
-                    if (value->intvalue < 0) {
-                        indexes.clear();
-                        break;
-                    }
-                    indexes.push_back(*value);
-                }
-                if (indexes.size() == arrayInfo.num().size()) {
-                    // Check if the indexes point outside the whole array..
-                    // char a[10][10];
-                    // a[0][20]  <-- ok.
-                    // a[9][20]  <-- error.
-
-                    // total number of elements of array..
-                    MathLib::bigint totalElements = 1;
-
-                    // total index..
-                    MathLib::bigint totalIndex = 0;
-
-                    // calculate the totalElements and totalIndex..
-                    for (unsigned int i = 0; i < indexes.size(); ++i) {
-                        std::size_t ri = indexes.size() - 1 - i;
-                        totalIndex += indexes[ri].intvalue * totalElements;
-                        totalElements *= arrayInfo.num(ri);
-                    }
-
-                    // totalElements == 0 => Unknown size
-                    if (totalElements == 0)
-                        continue;
-
-                    const Token *tok2 = tok->previous();
-                    while (tok2 && Token::Match(tok2->previous(), "%var% ."))
-                        tok2 = tok2->tokAt(-2);
-
-                    // just taking the address?
-                    const bool addr(tok2 && (tok2->str() == "&" ||
-                                             Token::simpleMatch(tok2->previous(), "& (")));
-
-                    // taking address of 1 past end?
-                    if (addr && totalIndex == totalElements)
-                        continue;
-
-                    // Is totalIndex in bounds?
-                    if (totalIndex >= totalElements) {
-                        arrayIndexOutOfBoundsError(tok, arrayInfo, indexes);
-                        break;
-                    }
-
-                    // Is any array index out of bounds?
-                    else {
-                        // check each index for overflow
-                        for (unsigned int i = 0; i < indexes.size(); ++i) {
-                            if (indexes[i].intvalue >= arrayInfo.num(i)) {
-                                // The access is still within the memory range for the array
-                                // so it may be intentional.
-                                if (_settings->inconclusive) {
-                                    arrayIndexOutOfBoundsError(tok, arrayInfo, indexes);
-                                    break; // only warn about the first one
-                                }
-                            }
-                        }
-                    }
-                }
-            }
         }
 
         // Loop..

lib/checkbufferoverrun.h

@@ -231,6 +231,8 @@ private:
     void argumentSizeError(const Token *tok, const std::string &functionName, const std::string &varname);
     void writeOutsideBufferSizeError(const Token *tok, const std::size_t stringLength, const MathLib::bigint writeLength, const std::string& functionName);
 
+    void valueFlowCheckArrayIndex(const Token * const tok, const ArrayInfo &arrayInfo);
+
 public:
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckBufferOverrun c(0, settings, errorLogger);

lib/valueflow.cpp

@@ -722,14 +722,21 @@ static void execute(const Token *expr,
 static bool valueFlowForLoop1(const Token *tok, unsigned int * const varid, MathLib::bigint * const num1, MathLib::bigint * const num2)
 {
     tok = tok->tokAt(2);
-    if (!Token::Match(tok,"%type%| %var% = %num% ;"))
+    if (!Token::Match(tok,"%type%| %var% ="))
         return false;
     const Token * const vartok = tok->tokAt(Token::Match(tok, "%var% =") ? 0 : 1);
     if (vartok->varId() == 0U)
         return false;
     *varid = vartok->varId();
-    *num1 = MathLib::toLongNumber(vartok->strAt(2));
+    const Token * const num1tok = Token::Match(vartok->tokAt(2), "%num% ;") ? vartok->tokAt(2) : nullptr;
-    tok = vartok->tokAt(4);
+    if (num1tok)
+        *num1 = MathLib::toLongNumber(num1tok->str());
+    tok = vartok->tokAt(2);
+    while (Token::Match(tok, "%var%|%num%|%or%|+|-|*|/|&|[|]|("))
+        tok = (tok->str() == "(") ? tok->link()->next() : tok->next();
+    if (!tok || tok->str() != ";")
+        return false;
+    tok = tok->next();
     const Token *num2tok = nullptr;
     if (Token::Match(tok, "%varid% <|<=|!=", vartok->varId())) {
         tok = tok->next();
@@ -742,6 +749,8 @@ static bool valueFlowForLoop1(const Token *tok, unsigned int * const varid, Math
     if (!num2tok)
         return false;
     *num2 = MathLib::toLongNumber(num2tok ? num2tok->str() : "0") - ((tok->str()=="<=") ? 0 : 1);
+    if (!num1tok)
+        *num1 = *num2;
     while (tok && tok->str() != ";")
         tok = tok->next();
     if (!num2tok || !Token::Match(tok, "; %varid% ++ ) {", vartok->varId()))

test/testbufferoverrun.cpp

@@ -425,7 +425,8 @@ private:
                   "    for (i = a; i < 100; i++)\n"
                   "        sum += val[i];\n"
                   "}");
-            ASSERT_EQUALS("[test.cpp:6]: (error) Buffer is accessed out of bounds: val\n", errout.str());
+            ASSERT_EQUALS("[test.cpp:6]: (error) Buffer is accessed out of bounds: val\n"
+                          "[test.cpp:6]: (error) Array 'val[50]' accessed at index 99, which is out of bounds.\n", errout.str());
         }
 
         {

test/testvalueflow.cpp

@@ -633,6 +633,12 @@ private:
         ASSERT_EQUALS(true, testValueOfX(code, 3U, 9));
         ASSERT_EQUALS(false, testValueOfX(code, 3U, 10));
 
+        code = "void f(int a) {\n"
+               "    for (int x = a; x < 10; x++)\n"
+               "        a[x] = 0;\n"
+               "}";
+        ASSERT_EQUALS(true, testValueOfX(code, 3U, 9));
+
         code = "void f() {\n"
                "    for (int x = 0; x < 10; x = x + 2)\n"
                "        a[x] = 0;\n"