Partial fix for 9049: False negative: uninitialized variable with nested ifs

lib/analyzer.h

@@ -153,9 +153,13 @@ struct Analyzer {
     struct Assume {
         enum Flags : std::uint8_t {
             None = 0,
-            Quiet = (1 << 0),
-            Absolute = (1 << 1),
-            ContainerEmpty = (1 << 2),
+            Quiet = (1u << 0),
+            Absolute = (1u << 1),
+            ContainerEmpty = (1u << 2),
+            // The branch this condition guards is not traversed yet (a separate path walks it), so
+            // the assume must not record the program state at the branch boundaries - they would be
+            // premature. When unset, the branch has been traversed and control is leaving it.
+            Pending = (1u << 3),
         };
     };
 

lib/astutils.cpp

@@ -2230,6 +2230,8 @@ bool isEscapeFunction(const Token* ftok, const Library& library)
 {
     if (!Token::Match(ftok, "%name% ("))
         return false;
+    if (Token::Match(ftok, "exit|abort"))
+        return true;
     const Function* function = ftok->function();
     if (function) {
         if (function->isEscapeFunction())

lib/programmemory.h

@@ -28,6 +28,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <tuple>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -161,20 +162,38 @@
 };
 
 struct ProgramMemoryState {
+    struct ChangedKeyHash {
+        std::size_t operator()(const std::tuple<const Token*, const Token*, const Token*>& t) const {
+            const std::hash<const Token*> h;
+            std::size_t seed = h(std::get<0>(t));
+            seed ^= h(std::get<1>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+            seed ^= h(std::get<2>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+            return seed;
+        }
+    };
+    using ChangedCache = std::unordered_map<std::tuple<const Token*, const Token*, const Token*>, const Token*, ChangedKeyHash>;
+    // The token modifying expr between start and end, or nullptr.
+    using FindChangedFn = std::function<const Token*(const Token* expr, const Token* start, const Token* end)>;
+
     ProgramMemory state;
     std::map<nonneg int, const Token*> origins;
     const Settings& settings;
+    // Memoized findExpressionChanged() pre-filter; structural, so never invalidated.
+    std::shared_ptr<ChangedCache> changedCache;
 
     explicit ProgramMemoryState(const Settings& s);
 
     void replace(ProgramMemory pm, const Token* origin = nullptr);
 
     void addState(const Token* tok, const ProgramMemory::Map& vars);
 
-    void assume(const Token* tok, bool b, bool isEmpty = false);
+    void assume(const Token* tok, bool b, bool isEmpty = false, const Token* origin = nullptr);
 
     void removeModifiedVars(const Token* tok);
 
+    // A findExpressionChanged() closure memoized in changedCache
+    FindChangedFn getCachedFindExpressionChanged(bool skipDeadCode) const;
+
     ProgramMemory get(const Token* tok, const Token* ctx, const ProgramMemory::Map& vars) const;
 };
 
@@ -184,21 +203,30 @@
              ProgramMemory& programMemory,
              MathLib::bigint* result,
              bool* error,
-             const Settings& settings);
+             const Settings& settings,
+             const ProgramMemory::Map& vars = {});
 
 /**
  * Is condition always false when variable has given value?
  * \param condition   top ast token in condition
  * \param pm   program memory
+ * \param vars  optional tracked values that take precedence over the program memory
  */
-bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings);
+bool conditionIsFalse(const Token* condition,
+                      ProgramMemory pm,
+                      const Settings& settings,
+                      const ProgramMemory::Map& vars = {});
 
 /**
  * Is condition always true when variable has given value?
  * \param condition   top ast token in condition
  * \param pm   program memory
+ * \param vars  optional tracked values that take precedence over the program memory
  */
-bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings);
+bool conditionIsTrue(const Token* condition,
+                     ProgramMemory pm,
+                     const Settings& settings,
+                     const ProgramMemory::Map& vars = {});
 
 /**
  * Get program memory by looking backwards from given token.

lib/vf_analyzers.cpp

@@ -678,16 +678,20 @@ struct ValueFlowAnalyzer : Analyzer {
         if (const ValueFlow::Value* v = tok->getKnownValue(ValueFlow::Value::ValueType::INT))
             return {v->intvalue};
         std::vector<MathLib::bigint> result;
-        ProgramMemory pm = getProgramMemoryFunc();
+        // Pass the tracked values so a cached program-memory value that depends on one (e.g. 'h(p)'
+        // after 'p' was reassigned) is re-evaluated rather than served stale. The memory is built
+        // from the same state, so compute it once and hand it to the builder.
+        const ProgramState vars = getProgramState();
+        ProgramMemory pm = getProgramMemoryFunc(vars);
         if (Token::Match(tok, "&&|%oror%")) {
-            if (conditionIsTrue(tok, pm, getSettings()))
+            if (conditionIsTrue(tok, pm, getSettings(), vars))
                 result.push_back(1);
-            if (conditionIsFalse(tok, std::move(pm), getSettings()))
+            if (conditionIsFalse(tok, std::move(pm), getSettings(), vars))
                 result.push_back(0);
         } else {
             MathLib::bigint out = 0;
             bool error = false;
-            execute(tok, pm, &out, &error, getSettings());
+            execute(tok, pm, &out, &error, getSettings(), vars);
             if (!error)
                 result.push_back(out);
         }
@@ -696,16 +700,16 @@ struct ValueFlowAnalyzer : Analyzer {
 
     std::vector<MathLib::bigint> evaluateInt(const Token* tok) const
     {
-        return evaluateInt(tok, [&] {
-            return ProgramMemory{getProgramState()};
+        return evaluateInt(tok, [](const ProgramState& vars) {
+            return ProgramMemory{vars};
         });
     }
 
     std::vector<MathLib::bigint> evaluate(Evaluate e, const Token* tok, const Token* ctx = nullptr) const override
     {
         if (e == Evaluate::Integral) {
-            return evaluateInt(tok, [&] {
-                return pms.get(tok, ctx, getProgramState());
+            return evaluateInt(tok, [&](const ProgramState& vars) {
+                return pms.get(tok, ctx, vars);
             });
         }
         if (e == Evaluate::ContainerEmpty) {
@@ -723,30 +727,43 @@ struct ValueFlowAnalyzer : Analyzer {
         return {};
     }
 
-    void assume(const Token* tok, bool state, unsigned int flags) override {
-        // Update program state
-        pms.removeModifiedVars(tok);
-        pms.addState(tok, getProgramState());
-        pms.assume(tok, state, flags & Assume::ContainerEmpty);
-
+    void assume(const Token* tok, bool state, unsigned int flags) override
+    {
         bool isCondBlock = false;
         const Token* parent = tok->astParent();
         if (parent) {
             isCondBlock = Token::Match(parent->previous(), "if|while (");
         }
 
+        const Token* endBlock = nullptr;
         if (isCondBlock) {
             const Token* startBlock = parent->link()->next();
             if (Token::simpleMatch(startBlock, ";") && Token::simpleMatch(parent->tokAt(-2), "} while ("))
                 startBlock = parent->linkAt(-2);
-            const Token* endBlock = startBlock->link();
-            if (state) {
-                pms.removeModifiedVars(endBlock);
-                pms.addState(endBlock->previous(), getProgramState());
-            } else {
-                if (Token::simpleMatch(endBlock, "} else {"))
-                    pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
-            }
+            endBlock = startBlock->link();
+        }
+
+        // Without Pending the 'then' block has been traversed and control is leaving it, so anchor
+        // the assumed state at the block end instead of the condition. That keeps assumptions on
+        // variables modified inside the block (e.g. an 'if' narrowing a value computed there) from
+        // being discarded as "modified" once control leaves the block.
+        const bool scopeEnd = !(flags & Assume::Pending) && state && endBlock;
+        const Token* anchor = scopeEnd ? endBlock : tok;
+        const Token* origin = scopeEnd ? endBlock : nullptr;
+
+        // Update program state
+        pms.removeModifiedVars(anchor);
+        pms.addState(anchor, getProgramState());
+        pms.assume(tok, state, flags & Assume::ContainerEmpty, origin);
+
+        // The false path (the true path uses scopeEnd above): record the assumed state where control
+        // continues - the end of the else block, or the closing brace when there is no else - so it
+        // reaches the enclosing scope.
+        if (isCondBlock && !(flags & Assume::Pending) && !state) {
+            if (Token::simpleMatch(endBlock, "} else {"))
+                pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
+            else
+                pms.addState(endBlock, getProgramState());
         }
 
         if (!(flags & Assume::Quiet)) {

test/testautovariables.cpp

@@ -4848,7 +4848,9 @@ class TestAutoVariables : public TestFixture {
               "        return *iPtr;\n"
               "    return 0;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:5:16] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n", errout_str());
+        ASSERT_EQUALS(
+            "[test.cpp:5:16] -> [test.cpp:7:10] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n",
+            errout_str());
 
         // #11753
         check("int main(int argc, const char *argv[]) {\n"

test/testcondition.cpp

@@ -4911,6 +4911,30 @@ class TestCondition : public TestFixture {
         ASSERT_EQUALS("[test.cpp:3:10]: (style) Condition 'b()' is always false [knownConditionTrueFalse]\n"
                       "[test.cpp:4:9]: (style) Condition '!b()' is always true [knownConditionTrueFalse]\n",
                       errout_str());
+
+        check("int g();\n" // a value modified inside a nested branch must be lowered to possible
+              "void f(int outer, int inner) {\n"
+              "    int bits = 0;\n"
+              "    if (outer) {\n"
+              "        if (inner == 1)\n"
+              "            bits = g();\n"
+              "    }\n"
+              "    if (bits > 0) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
+
+        check("int g();\n" // the modifying branch has an escaping sibling - still must be lowered
+              "void f(int t, int u) {\n"
+              "    int v = 0;\n"
+              "    if (t) {\n"
+              "        if (u == 2)\n"
+              "            v = g();\n"
+              "        else\n"
+              "            return;\n"
+              "    }\n"
+              "    if (v > 0) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout_str());
     }
 
     void alwaysTrueSymbolic()

test/testnullpointer.cpp

@@ -2450,6 +2450,9 @@ class TestNullPointer : public TestFixture {
 
     void nullpointer77()
     {
+        // No warning: 'i' is passed to the unknown function 'h' in the same condition that guards the
+        // dereference. 'h' may validate the pointer (e.g. return false for null), so '*i' can be safe
+        // - this is the common "if (check(p) && p->...)" pattern, so we must not assume 'i' is null.
         check("bool h(int*);\n"
               "void f(int* i) {\n"
               "    int* i = nullptr;\n"
@@ -2465,6 +2468,8 @@ class TestNullPointer : public TestFixture {
               "}\n");
         ASSERT_EQUALS("", errout_str());
 
+        // Likewise here, even though 'i' is null when the first 'h(i)' was true: the second 'h(i)' is an
+        // independent call that may validate 'i', so '*i' is not necessarily a null dereference.
         check("bool h(int*);\n"
               "void f(int* x) {\n"
               "    int* i = x;\n"

test/testother.cpp

@@ -11491,8 +11491,9 @@ class TestOther : public TestFixture {
               "        x = a + b;\n"
               "    return x;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:2:11] -> [test.cpp:4:9]: (style) Variable 'x' is assigned an expression that holds the same value. [redundantAssignment]\n",
-                      errout_str());
+        ASSERT_EQUALS(
+            "[test.cpp:2:11] -> [test.cpp:3:9] -> [test.cpp:4:9]: (style) Variable 'x' is assigned an expression that holds the same value. [redundantAssignment]\n",
+            errout_str());
     }
 
     void varFuncNullUB() { // #4482

test/teststl.cpp

@@ -368,7 +368,9 @@ class TestStl : public TestFixture {
                     "    if(b) ++x;\n"
                     "    return s[x];\n"
                     "}");
-        ASSERT_EQUALS("[test.cpp:5:13]: error: Out of bounds access in 's[x]', if 's' size is 6 and 'x' is 6 [containerOutOfBounds]\n", errout_str());
+        ASSERT_EQUALS(
+            "[test.cpp:5:13]: error: Out of bounds access in 's[x]', if 's' size is 6 and 'x' is 7 [containerOutOfBounds]\n",
+            errout_str());
 
         checkNormal("void f() {\n"
                     "    static const int N = 4;\n"

test/testuninitvar.cpp

@@ -4265,7 +4265,7 @@ class TestUninitVar : public TestFixture {
                         "    else {}\n"
                         "    return y;\n"
                         "}");
-        TODO_ASSERT_EQUALS("", "[test.cpp:5:9] -> [test.cpp:7:12]: (warning) Uninitialized variable: y [uninitvar]\n", errout_str());
+        ASSERT_EQUALS("", errout_str()); // #4560: escaping else keeps x known, so x is true and y is initialized
 
         valueFlowUninit("int f(int a) {\n" // #6583
                         "    int x;\n"
@@ -4284,7 +4284,8 @@ class TestUninitVar : public TestFixture {
                         "    else y = 123;\n" // <- y is always initialized
                         "    return y;\n"
                         "}");
-        TODO_ASSERT_EQUALS("", "[test.cpp:5:9] -> [test.cpp:7:12]: (warning) Uninitialized variable: y [uninitvar]\n", errout_str());
+        ASSERT_EQUALS("",
+                      errout_str()); // #4560: fork-based condition analysis tracks x==0 -> else branch -> y initialized
 
         valueFlowUninit("void f(int x) {\n" // #3948
                         "  int value;\n"
@@ -5614,6 +5615,47 @@ class TestUninitVar : public TestFixture {
                         "}");
         ASSERT_EQUALS("[test.cpp:18:9] -> [test.cpp:12:13] -> [test.cpp:8:15]: (warning) Uninitialized variable: s->flag [uninitvar]\n", errout_str());
 
+        // A value narrowed by a nested condition (dimensions < 1 here) must survive the enclosing
+        // 'if' so a later correlated condition can be resolved: dimensions == 1 is then false, the
+        // function does not return, and the uninitialized read is reached.
+        valueFlowUninit("unsigned int g();\n"
+                        "void f(bool a) {\n"
+                        "    unsigned int dimensions = 0;\n"
+                        "    bool mightBeLarger;\n"
+                        "    if (a) {\n"
+                        "        dimensions = g();\n"
+                        "        if (dimensions >= 1)\n"
+                        "            mightBeLarger = false;\n"
+                        "    } else {\n"
+                        "        mightBeLarger = false;\n"
+                        "    }\n"
+                        "    if (dimensions == 1)\n"
+                        "        return;\n"
+                        "    if (!mightBeLarger) {}\n"
+                        "}");
+        ASSERT_EQUALS(
+            "[test.cpp:5:9] -> [test.cpp:7:24] -> [test.cpp:14:10]: (warning) Uninitialized variable: mightBeLarger [uninitvar]\n",
+            errout_str());
+
+        // Same shape, but the later condition (dimensions == 0) is implied by the narrowing, so the
+        // early return fires on the uninitialized path and there must be no warning.
+        valueFlowUninit("unsigned int g();\n"
+                        "void f(bool a) {\n"
+                        "    unsigned int dimensions = 0;\n"
+                        "    bool mightBeLarger;\n"
+                        "    if (a) {\n"
+                        "        dimensions = g();\n"
+                        "        if (dimensions >= 1)\n"
+                        "            mightBeLarger = false;\n"
+                        "    } else {\n"
+                        "        mightBeLarger = false;\n"
+                        "    }\n"
+                        "    if (dimensions == 0)\n"
+                        "        return;\n"
+                        "    if (!mightBeLarger) {}\n"
+                        "}");
+        ASSERT_EQUALS("", errout_str());
+
         // Ticket #2207 - False negative
         valueFlowUninit("void foo() {\n"
                         "    int a;\n"

test/testvalueflow.cpp

@@ -6097,9 +6097,9 @@ class TestValueFlow : public TestFixture {
                "  c++;\n"
                "}\n";
         values = tokenValues(code, "c ++ ; }");
-        TODO_ASSERT_EQUALS(true, false, values.size() == 2);
-        // ASSERT_EQUALS(true, values.front().isUninitValue() || values.back().isUninitValue());
-        // ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible());
+        ASSERT_EQUALS(true, values.size() == 2);
+        ASSERT_EQUALS(true, values.front().isUninitValue() || values.back().isUninitValue());
+        ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible());
         // ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0);
 
         code = "void b(bool d, bool e) {\n"