From 8c2c81dbcd930517ecd5504a6d502de267b2b638 Mon Sep 17 00:00:00 2001
From: Ken-Patrick Lehrmann <kp.lehrmann+github@gmail.com>
Date: Fri, 5 Jun 2020 18:06:03 +0200
Subject: [PATCH] Fix some false positive in loop forward analysis (#2669)

* Fix some false positive in loop forward analysis

In cases like:
```
bool b();
void f()
{
    int val[50];
    int i, sum=0;
    for (i = 1; b() && i < 50; i++)
        sum += val[i];
    for (; i < 50; i++)
        sum -= val[i];
}
```
The forward analysis assumed the second loop was entered, and we ended
up with false positive in it:
 `Array 'val[50]' accessed at index 50, which is out of bounds`

* Fix style
---
 lib/forwardanalyzer.cpp    | 13 ++++++++++--
 test/testbufferoverrun.cpp | 41 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp
index 08b86e3b1..a052ae782 100644
--- a/lib/forwardanalyzer.cpp
+++ b/lib/forwardanalyzer.cpp
@@ -220,8 +220,17 @@ struct ForwardTraversal {
                 return Progress::Break;
         }
         // Traverse condition after lowering
-        if (condTok && updateRecursive(condTok) == Progress::Break)
-            return Progress::Break;
+        if (condTok) {
+            if (updateRecursive(condTok) == Progress::Break)
+                return Progress::Break;
+
+            bool checkThen, checkElse;
+            std::tie(checkThen, checkElse) = evalCond(condTok);
+            if (checkElse)
+                // condition is false, we don't enter the loop
+                return Progress::Break;
+        }
+
         forkScope(endBlock, allAnalysis.isModified());
         if (bodyAnalysis.isModified()) {
             Token* writeTok = findRange(endBlock->link(), endBlock, std::mem_fn(&ForwardAnalyzer::Action::isModified));
diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp
index db1041c36..f443ef9d7 100644
--- a/test/testbufferoverrun.cpp
+++ b/test/testbufferoverrun.cpp
@@ -156,6 +156,7 @@ private:
         TEST_CASE(array_index_function_parameter);
         TEST_CASE(array_index_enum_array); // #8439
         TEST_CASE(array_index_container); // #9386
+        TEST_CASE(array_index_two_for_loops);
 
         TEST_CASE(buffer_overrun_2_struct);
         TEST_CASE(buffer_overrun_3);
@@ -2211,6 +2212,46 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void array_index_two_for_loops() {
+        check("bool b();\n"
+              "void f()\n"
+              "{\n"
+              "    int val[50];\n"
+              "    int i, sum=0;\n"
+              "    for (i = 1; b() && i < 50; i++)\n"
+              "        sum += val[i];\n"
+              "    if (i < 50)\n"
+              "        sum -= val[i];\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool b();\n"
+              "void f()\n"
+              "{\n"
+              "    int val[50];\n"
+              "    int i, sum=0;\n"
+              "    for (i = 1; b() && i < 50; i++)\n"
+              "        sum += val[i];\n"
+              "    for (; i < 50;) {\n"
+              "        sum -= val[i];\n"
+              "        break;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("bool b();\n"
+              "void f()\n"
+              "{\n"
+              "    int val[50];\n"
+              "    int i, sum=0;\n"
+              "    for (i = 1; b() && i < 50; i++)\n"
+              "        sum += val[i];\n"
+              "    for (; i < 50; i++)\n"
+              "        sum -= val[i];\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void buffer_overrun_2_struct() {
         check("struct ABC\n"
               "{\n"