Fix #8260 Improve check: Pointer calculation result not null

[francois-berder]

No description provided.

[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

[pfultz2]

This should be done in valueflow. If there is a plus with a non zero value on a pointer, it should be set to !0. This can probably be done in the valueFlowImpossibleValues pass.

[francois-berder]

@pfultz2 I finally had time to fix my PR. I rebased my PR on top of main branch, implemented your suggestion and added more test cases.

[sonarqubecloud]

Quality Gate passed

Issues
2 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]

could you try to execute the test-my-pr.py script. I updated test-my-pr.py today so make sure you syncronize and rebase..

[danmar]

I got some review comments by AI. Please check. Feel free to ignore stuff you disagree with:

1. Potential null pointer dereference — tok->astParent() in the boolean branch

} else if (astIsPointer(tok) && isUsedAsBool(tok, *mSettings) && !tok->astParent()->isComparisonOp()) {

tok->astParent() is not guarded. isUsedAsBool may return true in some contexts where the parent is null, so calling
->isComparisonOp() on it would crash. This should be written as:

} else if (astIsPointer(tok) && isUsedAsBool(tok, *mSettings) &&
(!tok->astParent() || !tok->astParent()->isComparisonOp())) {

2. Performance: missing early-exit filter in checkPointerAdditionResultNotNull

The old code had if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) continue; as an early filter. The new
code removes this and calls getPointerAdditionCalcToken(tok) on every token in every function scope. getPointerAdditionCalcToken
does non-trivial work (iterates up to 3 tokens, iterates valueflow values). Consider adding back a cheap pre-filter, for example:

if (!tok->isComparisonOp() && !(astIsPointer(tok) && isUsedAsBool(tok, *mSettings)))
continue;

3. Misleading error message for implicit boolean use

The message "Comparison is wrong. Result of 'q' can't be 0..." says "Comparison is wrong" but for if (q) there is no comparison —

4. Missing test cases

• if (!q) where q = ptr + 1 — should this also warn? Currently it seems like it would (since !tok->astParent()->isComparisonOp() would be true for !), but it's not tested.
• The original example from issue Fix #14532 (Release 2.20: Update Cppcheck Premium checkers mapping) #8260 was noted missing by @chrchr-github — worth confirming it's covered.
• A negative test where the offset is zero (ptr + 0) — should not warn, and isPointerArithmeticAdd correctly excludes it via != 0, but a regression test would make that explicit.

[francois-berder]

could you try to execute the test-my-pr.py script. I updated test-my-pr.py today so make sure you syncronize and rebase..

I confirm I was able to run test-my-pr.py script. Thank you for fixing this.

[danmar]

why not return a bool from this function?

[francois-berder]

I ran tools/test-my-pr.py but it seems I did it on old code so I have to run it again.

[francois-berder]

I ran tools/test-my-pr with 300 packages (I cannot download lots of packages because I am on a 4G connection and I do not want to run out of data). There is no diff with main.
my_check_diff_timing.log

my_check_diff.log

[francois-berder]

I also ran it on 300 packages without the last commit with no diff with main:
my_check_diff.log
my_check_diff_timing.log

[danmar]

just a small nit. almost done!

[danmar]

we should say where the overflow would happen. it's not on line 3. maybe the calculation is 100 lines above the condition and the developer wonders why we warn..