Fixed false negatives in CheckBufferOverrun::arrayIndexThenCheck()

PKEuS · PKEuS · commit 171e1b8244df · 2017-03-27T11:07:49.000+02:00
diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp
@@ -1864,35 +1864,28 @@ void CheckBufferOverrun::arrayIndexThenCheck()
             }
 
             if (Token::Match(tok, "%name% [ %var% ]")) {
-                tok = tok->tokAt(2);
+                tok = tok->next();
 
-                const unsigned int indexID = tok->varId();
-                const std::string& indexName(tok->str());
+                const unsigned int indexID = tok->next()->varId();
+                const std::string& indexName(tok->strAt(1));
 
-                // skip array index..
-                tok = tok->tokAt(2);
-                while (tok && tok->str() == "[")
-                    tok = tok->link()->next();
-
-                // syntax error
-                if (!tok)
-                    return;
-
-                // skip comparison
-                if (tok->tokType() == Token::eComparisonOp)
-                    tok = tok->tokAt(2);
+                // Iterate AST upwards
+                const Token* tok2 = tok;
+                const Token* tok3 = tok2;
+                while (tok2->astParent() && tok2->tokType() != Token::eLogicalOp) {
+                    tok3 = tok2;
+                    tok2 = tok2->astParent();
+                }
 
-                if (!tok)
-                    break;
-                // skip close parentheses
-                if (tok->str() == ")")
-                    tok = tok->next();
+                // Ensure that we ended at a logical operator and that we came from its left side
+                if (tok2->tokType() != Token::eLogicalOp || tok2->astOperand1() != tok3)
+                    continue;
 
                 // check if array index is ok
                 // statement can be closed in parentheses, so "(| " is using
-                if (Token::Match(tok, "&& (| %varid% <|<=", indexID))
+                if (Token::Match(tok2, "&& (| %varid% <|<=", indexID))
                     arrayIndexThenCheckError(tok, indexName);
-                else if (Token::Match(tok, "&& (| %any% >|>= %varid% !!+", indexID))
+                else if (Token::Match(tok2, "&& (| %any% >|>= %varid% !!+", indexID))
                     arrayIndexThenCheckError(tok, indexName);
             }
         }
diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp
@@ -3876,12 +3876,23 @@ class TestBufferOverrun : public TestFixture {
               "}");
         ASSERT_EQUALS("", errout.str());
 
-        // this one doesn't work for now, hopefully in the future
         check("void f(const int a[], unsigned i) {\n"
               "    if(a[i] < func(i) && i <= 42) {\n"
               "    }\n"
               "}");
-        TODO_ASSERT_EQUALS("[test.cpp:2]: (style) Array index 'i' is used before limits check.\n", "", errout.str());
+        ASSERT_EQUALS("[test.cpp:2]: (style) Array index 'i' is used before limits check.\n", errout.str());
+
+        check("void f(const int a[], unsigned i) {\n"
+              "    if (i <= 42 && a[i] < func(i)) {\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(const int a[], unsigned i) {\n"
+              "    if (foo(a[i] + 3) < func(i) && i <= 42) {\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) Array index 'i' is used before limits check.\n", errout.str());
 
         check("void f(int i) {\n" // sizeof
               "  sizeof(a)/sizeof(a[i]) && i < 10;\n"
