Fix #12612 FP uninitvar with pointer alias in subfunction

[chrchr-github]

No description provided.

[pfultz2]

Why is this extra check needed?

[chrchr-github]

Which check exactly? We now match in case there is an alias that has an uninit value pointing to the variable in question. But I'm not sure if it is correct to only check for uninit values.

[pfultz2]

That should happen in the analyzeToken function, not here. We are already checking for aliases there as well.

Finally the tokvalue is not always an alias either.

[chrchr-github]

analyzeToken() has this:

Action la = analyzeLifetime(lifeTok);
if (la.matches()) { ...

So matches() needs to return true for the propagation of the uninit value to stop.
I looked at how that works for local code and tried to make the same stuff happen with a subfunction/valueFlowInjectParameter()

[pfultz2]

So matches() needs to return true for the propagation of the uninit value to stop.

Its not that matches() needs to return true, but that analyzeLifetime should return Action::Match, but I also think it can return Action::Read as well as the isAliasModified will handle it for cases where its not an exact match.

So it seems like you are modifying match so analyzeLifetime returns Action::Match, which is not the right approach. The match function is for exact matches, not to match aliases. That can lead to FPs. This needs to be done in either analyzeToken or analyzeLifetime where we are already checking for aliases.

[chrchr-github]

MultiValueFlowAnalyzer::values is inaccessible in analyzeLifetime()and analyzeToken().

[pfultz2]

Why do you need to read the values? The lifetime values are stored in the token directly.

[pfultz2]

For this case, p has the lifetime of lifetime[Object]=(a)@1, so analyzeLifetime should return Action::Match.

[pfultz2]

Ah wait, I see the issue. MultiValueFlowAnalyzer uses the variables ids of the parameters not the arguments passed.

We probably need to pass a map to MultiValueFlowAnalyzer that takes the exprid of the expression passed and the Variable of the function parameter. Something like:

    std::unordered_map<nonneg int, ValueFlow::Value> values;
    std::unordered_map<nonneg int, const Variable*> vars;
    std::unordered_map<nonneg int, const Variable*> ivars;

    MultiValueFlowAnalyzer(const std::unordered_map<const Variable*, ValueFlow::Value>& args, const std::unordered_map<nonneg int, const Variable*>& iargs, const Settings& set)
        : ValueFlowAnalyzer(set) {
        ...
    }

Then the match function can check the vars and ivars. We need to update getValue to check ivars as well.

[pfultz2]

Instead of skipping subfunction lifetimes, you could just check that the path is the same if the propagated value's path(ie the value returned from getValue(lifetok)) is not zero.

[chrchr-github]

Subfunction lifetimes were skipped before this change, and we didn't proceed to analyzeLifetime() below.

[pfultz2]

I see, I read it backwards. Its enabling it. We still need to check the correct path to avoid FPs.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[danmar]

@pfultz2 since you reviewed before .. I would really appreciate your eyes on this again..

[danmar]

I think test-my-pr.py would be good to run. However it currently does not work very well. I believe after I merge #8491 it will work better..

[danmar]

I asked Claude to review this. Feel free to ignore comments.. but here is what Claude had to say:

Hunk 2 — vf_analyzers.cpp:1074 (MultiValueFlowAnalyzer::match)

Change: Extend match() to also return true when any tracked value is UNINIT and its tokvalue->varId() equals the queried token's varId.

Concerns:

1. Potential null pointer dereference. ValueFlow::Value::tokvalue is default-initialised to nullptr (vfvalue.h:323). For ValueType::UNINIT values, tokvalue is not routinely set — only
   LIFETIME and SYMBOLIC values reliably carry it. The lambda:
   p.second.isUninitValue() && p.second.tokvalue->varId() == tok->varId()
   will crash whenever tokvalue is null, which is the common case for plain UNINIT values. A null-check is required before dereferencing.
2. Wrong abstraction layer. match() is meant to answer "is this token the tracked variable?" — not "is this token an alias of a variable stored as an UNINIT value?" That alias-following
   logic belongs in analyzeToken() or analyzeLifetime(), where it already exists for SingleValueFlowAnalyzer. Putting it in match() risks spurious matches elsewhere in the analyzer (e.g. in
   isModified(), isAliasModified(), and analyze() paths that all call match() for different purposes). This concern was also raised by @pfultz2 in the review thread.
3. Unclear whether this hunk is even needed. The scenario from issue #12612 involves a SubFunctionLifetimeValue on the pointer inside the subfunction — Hunk 1 already fixes that by
   letting SingleValueFlowAnalyzer follow those lifetime values. It is not obvious what code path reaches MultiValueFlowAnalyzer::match() for this bug. If it is dead code relative to the fix
   it should be removed; if it is needed, a dedicated test that isolates this path would clarify the intent.

Test coverage

One test case is added, which covers the minimal reproduction. Suggested additions:

• A case with a nested alias (char* p = b; char* q = p; *q = '\0';) to verify alias chains work
• A true-positive case where the pointer write is missing, confirming the warning still fires (guards against false negatives)