Fix 10331: wrong conditional value after assignment+return (#3461)

Author: pfultz2

lib/forwardanalyzer.cpp

@@ -12,6 +12,14 @@
 #include <tuple>
 #include <utility>
 
+struct OnExit {
+    std::function<void()> f;
+
+    ~OnExit() {
+        f();
+    }
+};
+
 struct ForwardTraversal {
     enum class Progress { Continue, Break, Skip };
     enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };
@@ -25,6 +33,7 @@ struct ForwardTraversal {
     bool analyzeTerminate;
     Analyzer::Terminate terminate = Analyzer::Terminate::None;
     bool forked = false;
+    std::vector<Token*> loopEnds = {};
 
     Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) {
         if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
@@ -85,9 +94,15 @@ struct ForwardTraversal {
 
     template<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
     Progress traverseTok(T* tok, std::function<Progress(T*)> f, bool traverseUnknown, T** out = nullptr) {
-        if (Token::Match(tok, "asm|goto|continue|setjmp|longjmp"))
-            return Break();
-        else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
+        if (Token::Match(tok, "asm|goto|setjmp|longjmp"))
+            return Break(Analyzer::Terminate::Bail);
+        else if (Token::simpleMatch(tok, "continue")) {
+            if (loopEnds.empty())
+                return Break(Analyzer::Terminate::Escape);
+            // If we are in a loop then jump to the end
+            if (out)
+                *out = loopEnds.back();
+        } else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
             traverseRecursive(tok->astOperand1(), f, traverseUnknown);
             traverseRecursive(tok->astOperand2(), f, traverseUnknown);
             return Break(Analyzer::Terminate::Escape);
@@ -361,6 +376,10 @@ struct ForwardTraversal {
     }
 
     Progress updateInnerLoop(Token* endBlock, Token* stepTok, Token* condTok) {
+        loopEnds.push_back(endBlock);
+        OnExit oe{[&] {
+                loopEnds.pop_back();
+            }};
         if (endBlock && updateScope(endBlock) == Progress::Break)
             return Break();
         if (stepTok && updateRecursive(stepTok) == Progress::Break)
@@ -632,7 +651,7 @@ struct ForwardTraversal {
                     }
                     actions |= (thenBranch.action | elseBranch.action);
                     if (bail)
-                        return Break();
+                        return Break(Analyzer::Terminate::Bail);
                     if (thenBranch.isDead() && elseBranch.isDead()) {
                         if (thenBranch.isModified() && elseBranch.isModified())
                             return Break(Analyzer::Terminate::Modified);

lib/valueflow.cpp

@@ -5313,6 +5313,7 @@ struct ConditionHandler {
                     startTokens[1] = top->link()->linkAt(1)->tokAt(2);
 
                 int changeBlock = -1;
+                int bailBlock = -1;
 
                 for (int i = 0; i < 2; i++) {
                     const Token* const startToken = startTokens[i];
@@ -5326,6 +5327,9 @@ struct ConditionHandler {
                     deadBranch[i] = r.terminate == Analyzer::Terminate::Escape;
                     if (r.action.isModified() && !deadBranch[i])
                         changeBlock = i;
+                    if (r.terminate != Analyzer::Terminate::None && r.terminate != Analyzer::Terminate::Escape &&
+                        r.terminate != Analyzer::Terminate::Modified)
+                        bailBlock = i;
                     changeKnownToPossible(values);
                 }
                 if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) {
@@ -5336,6 +5340,13 @@ struct ConditionHandler {
                                 "valueFlowAfterCondition: " + cond.vartok->expressionString() +
                                 " is changed in conditional block");
                     return;
+                } else if (bailBlock >= 0) {
+                    if (settings->debugwarnings)
+                        bailout(tokenlist,
+                                errorLogger,
+                                startTokens[bailBlock]->link(),
+                                "valueFlowAfterCondition: bailing in conditional block");
+                    return;
                 }
 
                 // After conditional code..

test/testnullpointer.cpp

@@ -121,6 +121,7 @@ class TestNullPointer : public TestFixture {
         TEST_CASE(nullpointer79); // #10400
         TEST_CASE(nullpointer80); // #10410
         TEST_CASE(nullpointer81); // #8724
+        TEST_CASE(nullpointer82); // #10331
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2474,6 +2475,26 @@ class TestNullPointer : public TestFixture {
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer82() // #10331
+    {
+        check("bool g();\n"
+              "int* h();\n"
+              "void f(int* ptr) {\n"
+              "    if (!ptr) {\n"
+              "        if (g())\n"
+              "            goto done;\n"
+              "        ptr = h();\n"
+              "        if (!ptr)\n"
+              "            return;\n"
+              "    }\n"
+              "    if (*ptr == 1)\n"
+              "        return;\n"
+              "\n"
+              "done:\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"

test/testvalueflow.cpp

@@ -1703,7 +1703,8 @@ class TestValueFlow : public TestFixture {
                 "    if (x==123){}\n"
                 "}");
         ASSERT_EQUALS_WITHOUT_LINENUMBERS(
-            "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n",
+            "[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n"
+            "[test.cpp:2]: (debug) valueflow.cpp::(valueFlow) bailout: valueFlowAfterCondition: bailing in conditional block\n",
             errout.str());
 
         // #5721 - FP
@@ -2952,6 +2953,18 @@ class TestValueFlow : public TestFixture {
                "    return 0;\n"
                "}\n";
         ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 1));
+
+        code = "int f(int x) {\n"
+               "    if (x == 1) {\n"
+               "        for(int i=0;i<1;i++) {\n"
+               "            if (x == 1)\n"
+               "                continue;\n"
+               "        }\n"
+               "    }\n"
+               "    return x;\n"
+               "}\n";
+        ASSERT_EQUALS(true, testValueOfX(code, 8U, 1));
+        ASSERT_EQUALS(false, testValueOfXImpossible(code, 8U, 1));
     }
 
     void valueFlowAfterConditionExpr() {