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.

[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)