Skip to content

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

Open
pfultz2 wants to merge 25 commits into
cppcheck-opensource:mainfrom
pfultz2:valueflow-forward-condition-fork3
Open

Partial fix for 9049: False negative: uninitialized variable with nested ifs#8680
pfultz2 wants to merge 25 commits into
cppcheck-opensource:mainfrom
pfultz2:valueflow-forward-condition-fork3

Conversation

@pfultz2

@pfultz2 pfultz2 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

This fixes the case for this:

unsigned int g();
void f(bool a) {
    unsigned int dimensions = 0;
    bool mightBeLarger;
    if (a) {
        dimensions = g();
        if (dimensions >= 1)
            mightBeLarger = false;
    } else {
        mightBeLarger = false;
    }
    if (dimensions == 1)
        return;
    if (!mightBeLarger) {}
}

Which doesnt use a compund condition dimensions >= 1 && b) and it also requires complete variables and functions. The compund condition could be handled in the future by forking within the condition, so if(a && b) { ... } can be treated as if(a) { if(b) { ... }}.

Here is a summary of the changes:

  1. Fork-based condition handling — lib/forwardanalyzer.cpp (the largest change)
  • When a condition can't be resolved, the traversal now forks: the then-branch is walked by a separate ForwardTraversal, in analyze-only mode when the value can't actually flow into it (opaque/correlated conditions like if (f(x))), so the branch's effect is tracked but nothing is reported there.
  • Branch breaks are deferred: if the else kills the value on the main path, the then-fork can still carry it forward.
  • Escapes the traversal didn't flag (e.g. unknown noreturn calls) are detected via isEscapeScope, and exit/abort are now recognized as escape functions (lib/astutils.cpp).
  1. Program-state anchoring at block boundaries — lib/vf_analyzers.cpp + lib/analyzer.h
  • assume() now anchors the assumed state at the block's end (not the condition) when control is leaving an already-traversed branch. This keeps an assumption on a variable modified inside the block (e.g. a nested if narrowing a value computed there) from being discarded as "modified" once control leaves the block.
  • New Assume::Pending flag marks the pre-traversal assume (branch walked separately) so it doesn't record premature boundary state.
  • ProgramMemoryState::assume gained an optional origin parameter so the anchor point can be overridden.
  1. vars-aware execute — lib/programmemory.cpp / .h
  • execute/conditionIsTrue/conditionIsFalse now take the tracked values (vars). A tracked value is the authoritative current value of its expression (returned and written back into the program memory), and any cached compound that depends on a tracked value is re-evaluated instead of served stale (getTrackedValue / dependsOnTrackedValue).
  • The per-assignment substitution was removed from fillProgramMemoryFromAssignments and now lives entirely in execute so it can be used for all executions.

Comment thread lib/analyzer.h Fixed
Comment thread lib/astutils.cpp
Comment on lines +2233 to +2234
if (Token::Match(ftok, "exit|abort"))
return true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also terminate() but it feels like should already be handled via the noreturn handling below (i.e. library configuration).

Comment thread lib/forwardanalyzer.cpp
const bool inElse = scope->type == ScopeType::eElse;
const bool inDoWhile = scope->type == ScopeType::eDo;
const bool inLoop = contains({ScopeType::eDo, ScopeType::eFor, ScopeType::eWhile}, scope->type);
const bool hasElse = Token::simpleMatch(tok, "} else {");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is grouped with other similar variables this is fine but in a future we should try to reduce the scope of this (i.e. avoid computing them until they are used).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was moved here so its not calculated multiple times. Probably could use the memoize function to lazily compute it once, but I think its beyond the scope here because I need to evaluate if its slower or not.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to moving them after the if (!condTok) check. But as also mentioned no need to do so.

@firewave

Copy link
Copy Markdown
Collaborator

The comments and the description appear to hint at AI assistance. Is that the case?

@chrchr-github

Copy link
Copy Markdown
Collaborator

Check time: lib/astutils.cpp: 1h 31m 59.563s vs 1m 32.907s before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants