Fix issue 8143: valueFlowCondition: before and inside while (#3045)

lib/astutils.cpp

@@ -81,6 +81,24 @@ const Token* findAstNode(const Token* ast, const std::function<bool(const Token*
     return result;
 }
 
+const Token* findExpression(MathLib::bigint exprid,
+                            const Token* start,
+                            const Token* end,
+                            const std::function<bool(const Token*)>& pred)
+{
+    if (!precedes(start, end))
+        return nullptr;
+    if (exprid == 0)
+        return nullptr;
+    for (const Token* tok = start; tok != end; tok = tok->next()) {
+        if (tok->exprId() != exprid)
+            continue;
+        if (pred(tok))
+            return tok;
+    }
+    return nullptr;
+}
+
 static void astFlattenRecursive(const Token *tok, std::vector<const Token *> *result, const char* op, nonneg int depth = 0)
 {
     ++depth;

lib/astutils.h

@@ -28,6 +28,7 @@
 #include <vector>
 
 #include "errortypes.h"
+#include "mathlib.h"
 #include "utils.h"
 
 class Function;
@@ -52,6 +53,10 @@ void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *
 void visitAstNodes(Token *ast, std::function<ChildrenToVisit(Token *)> visitor);
 
 const Token* findAstNode(const Token* ast, const std::function<bool(const Token*)>& pred);
+const Token* findExpression(MathLib::bigint exprid,
+                            const Token* start,
+                            const Token* end,
+                            const std::function<bool(const Token*)>& pred);
 
 std::vector<const Token*> astFlatten(const Token* tok, const char* op);
 

lib/reverseanalyzer.cpp

@@ -116,8 +116,11 @@ struct ReverseTraversal {
         return nullptr;
     }
 
-    void traverse(Token* start) {
+    void traverse(Token* start, const Token* end = nullptr)
-        for (Token* tok = start->previous(); tok; tok = tok->previous()) {
+    {
+        if (start == end)
+            return;
+        for (Token* tok = start->previous(); tok != end; tok = tok->previous()) {
             if (tok == start || (tok->str() == "{" && (tok->scope()->type == Scope::ScopeType::eFunction ||
                                  tok->scope()->type == Scope::ScopeType::eLambda))) {
                 break;
@@ -171,7 +174,7 @@ struct ReverseTraversal {
                                                     assignTok->astOperand2()->scope()->bodyEnd,
                                                     a,
                                                     settings);
-                            valueFlowGenericReverse(assignTok->astOperand1()->previous(), a, settings);
+                            valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, a, settings);
                         }
                     }
                 }
@@ -284,3 +287,9 @@ void valueFlowGenericReverse(Token* start, const ValuePtr<Analyzer>& a, const Se
     ReverseTraversal rt{a, settings};
     rt.traverse(start);
 }
+
+void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings)
+{
+    ReverseTraversal rt{a, settings};
+    rt.traverse(start, end);
+}

lib/reverseanalyzer.h

@@ -26,5 +26,6 @@ template <class T>
 class ValuePtr;
 
 void valueFlowGenericReverse(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings);
+void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings);
 
 #endif

lib/valueflow.cpp

@@ -2432,6 +2432,7 @@ static void valueFlowReverse(TokenList* tokenlist,
 }
 
 static void valueFlowReverse(Token* tok,
+                             const Token* const endToken,
                              const Token* const varToken,
                              const std::list<ValueFlow::Value>& values,
                              TokenList* tokenlist,
@@ -2442,16 +2443,25 @@ static void valueFlowReverse(Token* tok,
         auto aliases = getAliasesFromValues(values);
         for (const ValueFlow::Value& v : values) {
             VariableAnalyzer a(var, v, aliases, tokenlist);
-            valueFlowGenericReverse(tok, a, settings);
+            valueFlowGenericReverse(tok, endToken, a, settings);
         }
     } else {
         for (const ValueFlow::Value& v : values) {
             ExpressionAnalyzer a(varToken, v, tokenlist);
-            valueFlowGenericReverse(tok, a, settings);
+            valueFlowGenericReverse(tok, endToken, a, settings);
         }
     }
 }
 
+static void valueFlowReverse(Token* tok,
+                             const Token* const varToken,
+                             const std::list<ValueFlow::Value>& values,
+                             TokenList* tokenlist,
+                             const Settings* settings)
+{
+    valueFlowReverse(tok, nullptr, varToken, values, tokenlist, settings);
+}
+
 std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
 {
     std::string result;
@@ -4058,6 +4068,7 @@ struct ConditionHandler {
                          const Settings* settings) const = 0;
 
     virtual void reverse(Token* start,
+                         const Token* endToken,
                          const Token* exprTok,
                          const std::list<ValueFlow::Value>& values,
                          TokenList* tokenlist,
@@ -4139,6 +4150,8 @@ struct ConditionHandler {
 
             if (Token::Match(top, "%assign%"))
                 return;
+            if (Token::Match(cond.vartok->astParent(), "%assign%|++|--"))
+                return;
 
             if (Token::simpleMatch(tok->astParent(), "?") && tok->astParent()->isExpandedMacro()) {
                 if (settings->debugwarnings)
@@ -4159,6 +4172,23 @@ struct ConditionHandler {
                 return;
             }
 
+            std::list<ValueFlow::Value> values = cond.true_values;
+            if (cond.true_values != cond.false_values)
+                values.insert(values.end(), cond.false_values.begin(), cond.false_values.end());
+
+            // extra logic for unsigned variables 'i>=1' => possible value can also be 0
+            if (Token::Match(tok, "<|>")) {
+                values.remove_if([](const ValueFlow::Value& v) {
+                    if (v.isIntValue())
+                        return v.intvalue != 0;
+                    return false;
+                });
+                if (cond.vartok->valueType() && cond.vartok->valueType()->sign != ValueType::Sign::UNSIGNED)
+                    return;
+            }
+            if (values.empty())
+                return;
+
             // bailout: for/while-condition, variable is changed in while loop
             if (Token::Match(top->previous(), "for|while (") && Token::simpleMatch(top->link(), ") {")) {
 
@@ -4177,40 +4207,31 @@ struct ConditionHandler {
                 }
 
                 // Variable changed in loop code
-                if (Token::Match(top->previous(), "for|while (")) {
+                const Token* const start = top;
-                    const Token* const start = top;
+                const Token* const block = top->link()->next();
-                    const Token* const block = top->link()->next();
+                const Token* const end = block->link();
-                    const Token* const end = block->link();
 
-                    if (isExpressionChanged(cond.vartok, start, end, settings, tokenlist->isCPP())) {
+                if (isExpressionChanged(cond.vartok, start, end, settings, tokenlist->isCPP())) {
-                        if (settings->debugwarnings)
+                    // If its reassigned in loop then analyze from the end
-                            bailout(tokenlist,
+                    if (!Token::Match(tok, "%assign%|++|--") &&
-                                    errorLogger,
+                        findExpression(cond.vartok->exprId(), start, end, [&](const Token* tok2) {
-                                    tok,
+                            return Token::Match(tok2->astParent(), "%assign%") && astIsLHS(tok2);
-                                    "variable '" + cond.vartok->expressionString() + "' used in loop");
+                        })) {
-                        return;
+                        // Start at the end of the loop body
+                        Token* bodyTok = top->link()->next();
+                        reverse(bodyTok->link(), bodyTok, cond.vartok, values, tokenlist, settings);
                     }
+                    if (settings->debugwarnings)
+                        bailout(tokenlist,
+                                errorLogger,
+                                tok,
+                                "variable '" + cond.vartok->expressionString() + "' used in loop");
+                    return;
                 }
             }
 
-            std::list<ValueFlow::Value> values = cond.true_values;
-            if (cond.true_values != cond.false_values)
-                values.insert(values.end(), cond.false_values.begin(), cond.false_values.end());
-
-            // extra logic for unsigned variables 'i>=1' => possible value can also be 0
-            if (Token::Match(tok, "<|>")) {
-                values.remove_if([](const ValueFlow::Value& v) {
-                    if (v.isIntValue())
-                        return v.intvalue != 0;
-                    return false;
-                });
-                if (cond.vartok->valueType() && cond.vartok->valueType()->sign != ValueType::Sign::UNSIGNED)
-                    return;
-            }
-            if (values.empty())
-                return;
             Token* startTok = tok->astParent() ? tok->astParent() : tok->previous();
-            reverse(startTok, cond.vartok, values, tokenlist, settings);
+            reverse(startTok, nullptr, cond.vartok, values, tokenlist, settings);
         });
     }
 
@@ -4453,11 +4474,13 @@ struct SimpleConditionHandler : ConditionHandler {
     }
 
     virtual void reverse(Token* start,
+                         const Token* endToken,
                          const Token* exprTok,
                          const std::list<ValueFlow::Value>& values,
                          TokenList* tokenlist,
-                         const Settings* settings) const OVERRIDE {
+                         const Settings* settings) const OVERRIDE
-        return valueFlowReverse(start, exprTok, values, tokenlist, settings);
+    {
+        return valueFlowReverse(start, endToken, exprTok, values, tokenlist, settings);
     }
 
     virtual Condition parse(const Token* tok, const Settings*) const OVERRIDE {
@@ -5803,6 +5826,7 @@ static Analyzer::Action valueFlowContainerForward(Token* tok,
 }
 
 static void valueFlowContainerReverse(Token* tok,
+                                      const Token* const endToken,
                                       const Token* const varToken,
                                       const std::list<ValueFlow::Value>& values,
                                       TokenList* tokenlist,
@@ -5812,10 +5836,19 @@ static void valueFlowContainerReverse(Token* tok,
     auto aliases = getAliasesFromValues(values);
     for (const ValueFlow::Value& value : values) {
         ContainerVariableAnalyzer a(var, value, aliases, tokenlist);
-        valueFlowGenericReverse(tok, a, settings);
+        valueFlowGenericReverse(tok, endToken, a, settings);
     }
 }
 
+static void valueFlowContainerReverse(Token* tok,
+                                      const Token* const varToken,
+                                      const std::list<ValueFlow::Value>& values,
+                                      TokenList* tokenlist,
+                                      const Settings* settings)
+{
+    valueFlowContainerReverse(tok, nullptr, varToken, values, tokenlist, settings);
+}
+
 static bool isContainerSizeChanged(const Token *tok, int depth)
 {
     if (!tok)
@@ -6162,15 +6195,17 @@ struct ContainerConditionHandler : ConditionHandler {
     }
 
     virtual void reverse(Token* start,
+                         const Token* endTok,
                          const Token* exprTok,
                          const std::list<ValueFlow::Value>& values,
                          TokenList* tokenlist,
-                         const Settings* settings) const OVERRIDE {
+                         const Settings* settings) const OVERRIDE
+    {
         if (values.empty())
             return;
         if (!exprTok->variable())
             return;
-        return valueFlowContainerReverse(start, exprTok, values, tokenlist, settings);
+        return valueFlowContainerReverse(start, endTok, exprTok, values, tokenlist, settings);
     }
 
     virtual Condition parse(const Token* tok, const Settings*) const OVERRIDE {

test/testnullpointer.cpp

@@ -109,6 +109,7 @@ private:
         TEST_CASE(nullpointer66); // #10024
         TEST_CASE(nullpointer67); // #10062
         TEST_CASE(nullpointer68);
+        TEST_CASE(nullpointer69);         // #8143
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2135,6 +2136,47 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer69()
+    {
+        check("void f(const Scope *scope) {\n"
+              "    if (scope->definedType) {}\n"
+              "    while (scope) {\n"
+              "        scope = scope->nestedIn;\n"
+              "        enumerator = scope->findEnumerator();\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:5]: (warning) Either the condition 'scope' is redundant or there is possible null pointer dereference: scope.\n",
+            errout.str());
+
+        check("void f(const Scope *scope) {\n"
+              "    if (scope->definedType) {}\n"
+              "    while (scope && scope->nestedIn) {\n"
+              "        if (scope->type == Scope::eFunction && scope->functionOf)\n"
+              "            scope = scope->functionOf;\n"
+              "        else\n"
+              "            scope = scope->nestedIn;\n"
+              "        enumerator = scope->findEnumerator();\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3] -> [test.cpp:8]: (warning) Either the condition 'scope' is redundant or there is possible null pointer dereference: scope.\n",
+            errout.str());
+
+        check("struct a {\n"
+              "  a *b() const;\n"
+              "  void c();\n"
+              "};\n"
+              "void d() {\n"
+              "  for (a *e;;) {\n"
+              "    e->b()->c();\n"
+              "    while (e)\n"
+              "      e = e->b();\n"
+              "  }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"