Skip to content

Fix #15005: FP uninitdata with compound assignment or increment - #8822

Open
aadanen wants to merge 7 commits into
cppcheck-opensource:mainfrom
aadanen:compound_assignment
Open

aadanen wants to merge 7 commits into
cppcheck-opensource:mainfrom
aadanen:compound_assignment

Conversation

@aadanen

@aadanen aadanen commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

isVariableUsage() currently returns nullptr for the lhs of assignment operators, because unless it's a pointer that gets dereferenced, it is just getting overwritten and its potentially uninitialized value is not read. I extend this idea to compound assignment and increment/decrement operators.

While working on this, I found some issues with CTU analysis.

        valueFlowUninit("void increment(int& i) { ++i; }\n" // #6475
                        "int f() {\n"
                        "    int n;\n"
                        "    increment(n);\n"
                        "    return n;\n"
                        "}\n");
        ASSERT_EQUALS("[test.cpp:4:15] -> [test.cpp:1:28]: (warning) Uninitialized variable: i [uninitvar]\n", errout_str());

this test only works because of the false positive "usage" from the increment operator. For example, this test breaks if we use regular assignment because of the current code properly handling "="

this code

void increment(int& i) { i = i + 1; }
int f() {
    int n;
    increment(n);
    return n;
}

misses the ctu error, and only throws

Checking examples/ctu3.cpp ...
examples/ctu3.cpp:1:30: warning: Uninitialized variable: i [uninitvar]
void increment(int& i) { i = i + 1; }
                             ^
examples/ctu3.cpp:4:15: note: Calling function 'increment', 1st argument 'n' value is <Uninit>
    increment(n);
              ^
examples/ctu3.cpp:1:30: note: Uninitialized variable: i
void increment(int& i) { i = i + 1; }
                             ^

I think that the problem has to do with

// NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature
static bool isVariableUsage(const Settings &settings, const Token *vartok, MathLib::bigint *value)
{
    (void)value;
    return !!CheckUninitVarImpl::isVariableUsage(vartok, settings.library, true, CheckUninitVarImpl::Alloc::ARRAY);
}

the hardcoded pointer=true and alloc=ARRAY assignments tell isVariableUsage to return nullptr if it i isn't dereferenced, which it isn't. If we are going to hardcode these values to be this conservative then this test should fail because it currently only passes due to the FP bug I want to fix. I currently have a workaround where I use vartok->variable() to determine whether something is actually a pointer/array.

Maybe there can be another PR where we rework the ctu/uninitvar connection. Notably since I don't touch the "=" branch of the code the false negative I mentioned above remains unfixed. Let me know if I should apply my vartok workaround to that code in this PR, if it should be another PR, or if its just the wrong idea overall.

Comment thread lib/checkuninitvar.cpp Outdated
Comment thread lib/checkuninitvar.cpp Outdated

@danmar danmar left a comment

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.

it looks good to me. just have some nits to simplify the code a little.

Comment thread lib/checkuninitvar.cpp Outdated
Comment thread lib/checkuninitvar.cpp
apply simplification

Co-authored-by: Daniel Marjamäki <daniel.marjamaki@gmail.com>
@aadanen

aadanen commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

If the simplifications were the last changes to make, I think this is ready to merge! Thanks for the review :)

Comment thread test/testuninitvar.cpp Outdated
@aadanen
aadanen force-pushed the compound_assignment branch from cbf4546 to c5bef2a Compare October 5, 2026 14:59
@dmcppcheck

Copy link
Copy Markdown

Test results for commit c5bef2a (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8822/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: -0.5% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8822/2026-10-05_14-59-08-c5bef2ae24cd_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck

Copy link
Copy Markdown

@claude review

Comment thread lib/checkuninitvar.cpp
Comment thread test/testuninitvar.cpp
@claude

claude Bot commented Oct 10, 2026

Copy link
Copy Markdown

Review summary: the approach (treating the LHS of compound assignment and inc/dec like plain = for non-dereferenced pointers) looks right for #15005. Two points, posted inline:

  1. The new branch in isVariableUsage doesn't require astIsLhs(tok), so it also suppresses uses where the pointer is on the RHS of a compound assignment, which could hide real problems.
  2. Overriding pointer/alloc with vartok->variable() works around the hardcoded CTU callback arguments, but it affects every caller. It would be better to fix the CTU callback in a separate change.
    The tests should also cover the dereferenced cases that must still warn (*p += 1, (*p)++, p[0]++) and a CTU case with a pointer parameter.

Note: I reviewed this by reading the code only. I did not build it or run the tests.

🤖 Generated with Claude Code

@dmcppcheck

Copy link
Copy Markdown

@claude review

Comment thread lib/checkuninitvar.cpp
@claude

claude Bot commented Oct 10, 2026

Copy link
Copy Markdown

Review summary: the fix for #15005 looks right for plain pointer compound assignment and increment (p += 1, p++), and the tests cover the deref cases *p += 1, (*p)++ and p[0]++.

One issue, posted inline: the new inc/dec bailout doesn't notice a dereference of the inc/dec result, so c = *p++; on freshly allocated memory probably stops being reported (false negative). I worked this out from the code but couldn't build it here, so it isn't confirmed by running it. Please add a test for *p++ / p++[0].

Minor: in the new CTU test, a is undeclared and there is an extra blank line after it.

🤖 Generated with Claude Code

@dmcppcheck

Copy link
Copy Markdown

Test results for commit b1de83c (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8822/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: -0.9% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8822/2026-10-10_00-56-00-b1de83c73be9_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

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.

3 participants