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.cpp

@@ -262,26 +262,33 @@
     return mValues->find(ExprIdToken::create(exprid));
 }
 
-static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings);
-
-static bool evaluateCondition(MathLib::bigint r, const Token* condition, ProgramMemory& pm, const Settings& settings)
+static ValueFlow::Value execute(const Token* expr,
+                                ProgramMemory& pm,
+                                const Settings& settings,
+                                const ProgramMemory::Map& vars = {});
+
+static bool evaluateCondition(MathLib::bigint r,
+                              const Token* condition,
+                              ProgramMemory& pm,
+                              const Settings& settings,
+                              const ProgramMemory::Map& vars = {})
 {
     if (!condition)
         return false;
     MathLib::bigint result = 0;
     bool error = false;
-    execute(condition, pm, &result, &error, settings);
+    execute(condition, pm, &result, &error, settings, vars);
     return !error && result == r;
 }
 
-bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings)
+bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings, const ProgramMemory::Map& vars)
 {
-    return evaluateCondition(0, condition, pm, settings);
+    return evaluateCondition(0, condition, pm, settings, vars);
 }
 
-bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings)
+bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings, const ProgramMemory::Map& vars)
 {
-    return evaluateCondition(1, condition, pm, settings);
+    return evaluateCondition(1, condition, pm, settings, vars);
 }
 
 static bool frontIs(const std::vector<MathLib::bigint>& v, bool i)
@@ -429,22 +436,12 @@
     for (const Token *tok2 = tok; tok2; tok2 = tok2->previous()) {
         if ((Token::simpleMatch(tok2, "=") || Token::Match(tok2->previous(), "%var% (|{")) && tok2->astOperand1() &&
             tok2->astOperand2()) {
-            bool setvar = false;
             const Token* vartok = tok2->astOperand1();
-            for (const auto& p:vars) {
-                if (p.first.getExpressionId() != vartok->exprId())
-                    continue;
-                if (vartok == tok)
-                    continue;
-                pm.setValue(vartok, p.second);
-                setvar = true;
-            }
-            if (!setvar) {
-                if (!pm.hasValue(vartok->exprId())) {
-                    const Token* valuetok = tok2->astOperand2();
-                    ProgramMemory local = state;
-                    pm.setValue(vartok, execute(valuetok, local, settings));
-                }
+            if (!pm.hasValue(vartok->exprId())) {
+                const Token* valuetok = tok2->astOperand2();
+                ProgramMemory local = state;
+                // Tracked values are substituted by execute() when the expression is evaluated.
+                pm.setValue(vartok, execute(valuetok, local, settings, vars));
             }
         } else if (Token::simpleMatch(tok2, ")") && tok2->link() &&
                    Token::Match(tok2->link()->previous(), "assert|ASSERT ( !!)")) {
@@ -547,19 +544,21 @@
     replace(std::move(local), tok);
 }
 
-void ProgramMemoryState::assume(const Token* tok, bool b, bool isEmpty)
+void ProgramMemoryState::assume(const Token* tok, bool b, bool isEmpty, const Token* origin)
 {
     ProgramMemory pm = state;
     if (isEmpty)
         pm.setContainerSizeValue(tok, 0, b);
     else
         programMemoryParseCondition(pm, tok, nullptr, settings, b);
-    const Token* origin = tok;
-    const Token* top = tok->astTop();
-    if (Token::Match(top->previous(), "for|while|if (") && !Token::simpleMatch(tok->astParent(), "?")) {
-        origin = top->link()->next();
-        if (!b && origin->link()) {
-            origin = origin->link();
+    if (!origin) {
+        origin = tok;
+        const Token* top = tok->astTop();
+        if (Token::Match(top->previous(), "for|while|if (") && !Token::simpleMatch(tok->astParent(), "?")) {
+            origin = top->link()->next();
+            if (!b && origin->link()) {
+                origin = origin->link();
+            }
         }
     }
     replace(std::move(pm), origin);
@@ -1316,14 +1315,37 @@
     struct Executor {
         ProgramMemory* pm;
         const Settings& settings;
+        // Values tracked by the forward/reverse analysis. A tracked value is the authoritative
+        // current value of its expression and takes precedence over the program memory.
+        const ProgramMemory::Map* vars = nullptr;
         int fdepth = 4;
         int depth = 10;
 
         Executor(ProgramMemory* pm, const Settings& settings) : pm(pm), settings(settings)
         {
             assert(pm != nullptr);
         }
 
+        // Is the tracked value for this expression available?
+        const ValueFlow::Value* getTrackedValue(const Token* expr) const
+        {
+            if (!vars || expr->exprId() == 0)
+                return nullptr;
+            const auto it = vars->find(ExprIdToken::create(expr->exprId()));
+            return it == vars->end() ? nullptr : &it->second;
+        }
+
+        // Does the expression read a tracked value? If so, any value cached for it may be stale
+        // (the tracked value may have changed since), so it must be re-evaluated, not served cached.
+        bool dependsOnTrackedValue(const Token* expr) const
+        {
+            if (!vars || vars->empty())
+                return false;
+            return findAstNode(expr, [&](const Token* tok) {
+                return getTrackedValue(tok) != nullptr;
+            }) != nullptr;
+        }
+
         static ValueFlow::Value unknown() {
             return ValueFlow::Value::unknown();
         }
@@ -1622,7 +1644,15 @@
                 }
                 return execute(expr->astOperand1());
             }
-            if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) {
+            // Return the tracked value and write it back when it differs, so later reads see the
+            // same value (as fillProgramMemoryFromAssignments used to do).
+            if (const ValueFlow::Value* tracked = getTrackedValue(expr)) {
+                const ValueFlow::Value* stored = pm->getValue(expr->exprId(), /*impossible*/ true);
+                if (!stored || *stored != *tracked)
+                    pm->setValue(expr, *tracked);
+                return *tracked;
+            }
+            if (expr->exprId() > 0 && pm->hasValue(expr->exprId()) && !dependsOnTrackedValue(expr)) {
                 ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId());
                 if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) {
                     result.intvalue = !result.intvalue;
@@ -1815,9 +1845,13 @@
     };
 }     // namespace
 
-static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings)
+static ValueFlow::Value execute(const Token* expr,
+                                ProgramMemory& pm,
+                                const Settings& settings,
+                                const ProgramMemory::Map& vars)
 {
     Executor ex{&pm, settings};
+    ex.vars = &vars;
     return ex.execute(expr);
 }
 
@@ -1907,9 +1941,10 @@
              ProgramMemory& programMemory,
              MathLib::bigint* result,
              bool* error,
-             const Settings& settings)
+             const Settings& settings,
+             const ProgramMemory::Map& vars)
 {
-    ValueFlow::Value v = execute(expr, programMemory, settings);
+    ValueFlow::Value v = execute(expr, programMemory, settings, vars);
     if (!v.isIntValue() || v.isImpossible()) {
         if (error)
             *error = true;

lib/programmemory.h

@@ -171,7 +171,7 @@ struct ProgramMemoryState {
 
     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);
 
@@ -184,21 +184,30 @@ void execute(const Token* expr,
              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()