From 090eba7e7fff6ef1e01362337a588a0071ea4490 Mon Sep 17 00:00:00 2001
From: Paul Fultz II <pfultz2@yahoo.com>
Date: Thu, 3 Sep 2020 00:17:36 -0500
Subject: [PATCH] FIx issue 6140: ValueFlow: valueFlowForward, loop (#2770)

---
 lib/forwardanalyzer.cpp  | 40 +++++++++++++++++++++++++++++++---------
 test/testnullpointer.cpp | 11 +++++++++++
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp
index a3dd51111..db1b478c5 100644
--- a/lib/forwardanalyzer.cpp
+++ b/lib/forwardanalyzer.cpp
@@ -65,12 +65,19 @@ struct ForwardTraversal {
             return Progress::Continue;
         if (recursion > 10000)
             return Progress::Skip;
-        if (tok->astOperand1() && traverseRecursive(tok->astOperand1(), f, traverseUnknown, recursion+1) == Progress::Break)
+        T* firstOp = tok->astOperand1();
+        T* secondOp = tok->astOperand2();
+        // Evaluate RHS of assignment before LHS
+        if (tok->isAssignmentOp())
+            std::swap(firstOp, secondOp);
+        if (firstOp && traverseRecursive(firstOp, f, traverseUnknown, recursion+1) == Progress::Break)
             return Progress::Break;
-        Progress p = traverseTok(tok, f, traverseUnknown);
+        Progress p = tok->isAssignmentOp() ? Progress::Continue : traverseTok(tok, f, traverseUnknown);
         if (p == Progress::Break)
             return Progress::Break;
-        if (p == Progress::Continue && tok->astOperand2() && traverseRecursive(tok->astOperand2(), f, traverseUnknown, recursion+1) == Progress::Break)
+        if (p == Progress::Continue && secondOp && traverseRecursive(secondOp, f, traverseUnknown, recursion+1) == Progress::Break)
+            return Progress::Break;
+        if (tok->isAssignmentOp() && traverseTok(tok, f, traverseUnknown) == Progress::Break)
             return Progress::Break;
         return Progress::Continue;
     }
@@ -271,11 +278,7 @@ struct ForwardTraversal {
 
             // Evaluate RHS of assignment before LHS
             if (Token* assignTok = assignExpr(tok)) {
-                if (updateRecursive(assignTok->astOperand2()) == Progress::Break)
-                    return Progress::Break;
-                if (updateRecursive(assignTok->astOperand1()) == Progress::Break)
-                    return Progress::Break;
-                if (update(assignTok) == Progress::Break)
+                if (updateRecursive(assignTok) == Progress::Break)
                     return Progress::Break;
                 tok = nextAfterAstRightmostLeaf(assignTok);
                 if (!tok)
@@ -299,7 +302,7 @@ struct ForwardTraversal {
                     return Progress::Break;
                 if (Token::Match(tok->link()->previous(), ")|else {")) {
                     const bool inElse = Token::simpleMatch(tok->link()->previous(), "else {");
-                    const Token* condTok = getCondTokFromEnd(tok);
+                    Token* condTok = getCondTokFromEnd(tok);
                     if (!condTok)
                         return Progress::Break;
                     if (!condTok->hasKnownIntValue()) {
@@ -308,6 +311,16 @@ struct ForwardTraversal {
                     } else if (condTok->values().front().intvalue == inElse) {
                         return Progress::Break;
                     }
+                    // Handle for loop
+                    Token* stepTok = getStepTokFromEnd(tok);
+                    bool checkThen, checkElse;
+                    std::tie(checkThen, checkElse) = evalCond(condTok);
+                    if (stepTok && !checkElse) {
+                        if (updateRecursive(stepTok) == Progress::Break)
+                            return Progress::Break;
+                        if (updateRecursive(condTok) == Progress::Break)
+                            return Progress::Break;
+                    }
                     analyzer->assume(condTok, !inElse, tok);
                     if (Token::simpleMatch(tok, "} else {"))
                         tok = tok->linkAt(2);
@@ -528,6 +541,15 @@ struct ForwardTraversal {
         return tok->astOperand2()->astOperand2()->astOperand2();
     }
 
+    static Token* getStepTokFromEnd(Token* tok) {
+        if (!Token::simpleMatch(tok, "}"))
+            return nullptr;
+        Token* end = tok->link()->previous();
+        if (!Token::simpleMatch(end, ")"))
+            return nullptr;
+        return getStepTok(end->link());
+    }
+
 };
 
 void valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<ForwardAnalyzer>& fa, const Settings* settings)
diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp
index 952b75560..3c7c1421e 100644
--- a/test/testnullpointer.cpp
+++ b/test/testnullpointer.cpp
@@ -289,6 +289,17 @@ private:
               "  if (!d) throw;\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        check("struct b {\n"
+              "    b * c;\n"
+              "    int i;\n"
+              "};\n"
+              "void f(b* e1, b* e2) {\n"
+              "    for (const b* d = e1; d != e2; d = d->c) {\n"
+              "        if (d && d->i != 0) {}\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (warning) Either the condition 'd' is redundant or there is possible null pointer dereference: d.\n", errout.str());
     }
 
     void nullpointer1() {