Fix false positive with negative array index in issue 8536 (danmar#1202)

pfultz2 · danmar · commit 05618771827a · 2018-05-06T08:35:29.000+02:00
* Fix FP with negative array index in valueflow

* Remove values when valueflow fails

* Add valueflow test
diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -1722,7 +1722,7 @@ static bool valueFlowForward(Token * const               startToken,
                 // '{'
                 Token * const startToken1 = tok2->linkAt(1)->next();
 
-                valueFlowForward(startToken1->next(),
+                bool vfresult = valueFlowForward(startToken1->next(),
                                  startToken1->link(),
                                  var,
                                  varid,
@@ -1741,7 +1741,7 @@ static bool valueFlowForward(Token * const               startToken,
                 // goto '}'
                 tok2 = startToken1->link();
 
-                if (isReturnScope(tok2)) {
+                if (isReturnScope(tok2) || !vfresult) {
                     if (condAlwaysTrue)
                         return false;
                     removeValues(values, truevalues);
@@ -1750,7 +1750,7 @@ static bool valueFlowForward(Token * const               startToken,
                 if (Token::simpleMatch(tok2, "} else {")) {
                     Token * const startTokenElse = tok2->tokAt(2);
 
-                    valueFlowForward(startTokenElse->next(),
+                    vfresult = valueFlowForward(startTokenElse->next(),
                                      startTokenElse->link(),
                                      var,
                                      varid,
@@ -1769,7 +1769,7 @@ static bool valueFlowForward(Token * const               startToken,
                     // goto '}'
                     tok2 = startTokenElse->link();
 
-                    if (isReturnScope(tok2)) {
+                    if (isReturnScope(tok2) || !vfresult) {
                         if (condAlwaysFalse)
                             return false;
                         removeValues(values, falsevalues);
diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp
@@ -134,6 +134,7 @@ class TestBufferOverrun : public TestFixture {
         TEST_CASE(array_index_calculation);
         TEST_CASE(array_index_negative1);
         TEST_CASE(array_index_negative2);    // ticket #3063
+        TEST_CASE(array_index_negative3);
         TEST_CASE(array_index_for_decr);
         TEST_CASE(array_index_varnames);     // FP: struct member. #1576
         TEST_CASE(array_index_for_continue); // for,continue
@@ -1778,6 +1779,22 @@ class TestBufferOverrun : public TestFixture {
         ASSERT_EQUALS("[test.cpp:4]: (error) Array 'test.a[10]' accessed at index -1, which is out of bounds.\n", errout.str());
     }
 
+    void array_index_negative3() {
+      check("int f(int i) {\n"
+            "    int p[2] = {0, 0};\n"
+            "    if(i >= 2)\n"
+            "        return 0;\n"
+            "    else if(i == 0)\n"
+            "        return 0;\n"
+            "    return p[i - 1];\n"
+            "}\n"
+            "void g(int i) {\n"
+            "    if( i == 0 )\n"
+            "        return f(i);\n"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void array_index_for_decr() {
         check("void f()\n"
               "{\n"
diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp
@@ -2455,6 +2455,18 @@ class TestValueFlow : public TestFixture {
         ASSERT_EQUALS(false, testValueOfX(code, 3U, 0));
         ASSERT_EQUALS(true, testValueOfX(code, 3U, 4));
 
+        code = "int f(int i) {\n"
+               "  if(i >= 2)\n"
+               "    return 0;\n"
+               "  else if(i == 0)\n"
+               "    return 0;\n"
+               "  int a = i;\n"
+               "}\n"
+               "void g(int i) {\n"
+               "  return f(0);\n"
+               "}";
+        ASSERT_EQUALS(false, testValueOfX(code, 6U, 0));
+
         code = "void f1(int x) { a=x; }\n"
                "void f2(int y) { f1(y<123); }\n";
         ASSERT_EQUALS(true, testValueOfX(code, 1U, 0));
