Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Range propagation is currently unsound for wrapping arithmetic, casts, and compound updates, with additional precision and performance issues.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Refactors value-flow program memory to retain multiple constraints, fixing incorrect branch decisions for bounded conditions such as x > 3.
Changes:
- Stores and merges multiple value constraints per expression.
- Propagates ranges through arithmetic, conditions, containers, and execution.
- Adds regression and unit coverage for range-aware analysis.
| File | Description |
|---|---|
lib/programmemory.cpp |
Implements multi-value storage and range execution. |
lib/programmemory.h |
Defines the new list-based API. |
lib/valueflow.cpp |
Improves bound solving and loop handling. |
lib/vfvalue.h |
Adds range-edge helpers. |
lib/vf_common.h |
Adds shared saturation detection. |
lib/vf_analyzers.cpp |
Adapts analyzer state to value lists. |
lib/token.cpp |
Exposes contradiction removal. |
lib/token.h |
Declares contradiction-removal API. |
test/testprogrammemory.cpp |
Tests constraints and range execution. |
test/testvalueflow.cpp |
Tests range-based value flow. |
test/testnullpointer.cpp |
Tests null-pointer analysis with ranges. |
test/testcondition.cpp |
Covers issue 15042’s false positive. |
Makefile |
Adds the new header dependency. |
oss-fuzz/Makefile |
Updates fuzz-build dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (op == "+") { | ||
| result = edge + k; | ||
| } else if (op == "-") { | ||
| increasing = rangeIsLhs; | ||
| result = rangeIsLhs ? edge - k : k - edge; |
| const ValueFlow::Value* sizeValue = pm->getValue(value.tokvalue->exprId()); | ||
| if (sizeValue && sizeValue->isContainerSizeValue()) | ||
| return *sizeValue; | ||
| if (sizeValue && sizeValue->isContainerSizeValue()) { | ||
| sizes.push_back(*sizeValue); | ||
| break; | ||
| } |
| // Shift every value of the variable; bounds and impossible values move along | ||
| for (ValueFlow::Value& v : lhs) { | ||
| if (expr->str() == "++") | ||
| v.intvalue++; | ||
| else | ||
| v.intvalue--; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| bool increasing = true; | ||
| MathLib::bigint result = 0; | ||
| if (op == "+") { | ||
| result = edge + k; |
There was a problem hiding this comment.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve button
edge + k (and edge - k / k - edge below) can overflow the analyzer's own MathLib::bigint. isSaturated() only rejects values that are exactly at the limit. I confirmed it with a UBSan build of programmemory.cpp from this branch:
void n(long long x) {
int* p = 0;
if (x > 0x7ffffffffffffff0LL) {
if (x + 100 < 0) p = (int*)&x;
*p = 1;
}
}lib/programmemory.cpp:882:16: runtime error: signed integer overflow: 100 + 9223372036854775793 cannot be represented in type 'long long int'
Maybe return unknown() on overflow here, as the * branch already does via multiplyOverflows(). ceilDiv/floorDiv in valueflow.cpp have a similar corner case, LLONG_MIN / -1, which happens when rangeEdge() adds or subtracts 1 next to the limit.
For the false-positive question, I compared this branch against its merge-base on lib/, cli/, gui/, test/cfg/, samples/ and simplecpp (--enable=style,warning,performance,portability --inconclusive). The output was identical. I also tried a set of nested-condition cases with the p = 0; if (cond) p = &x; *p = 1; pattern, including unsigned wrap-around, narrow types and %/&. I found no new false positives. The only new warnings were correct "Possible null pointer dereference" for x % 4 == 0, (x & 1) == 0 and x - y == 0 under x > 3, where the old code assumed x == 4.
|
Test results for commit 51b980a (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8899/ 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. |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (delta.isIntValue() && mayWrap(lhs, lhsTok, op, delta.intvalue)) | ||
| return forget(lhs); | ||
| for (ValueFlow::Value& v : lhs) { | ||
| const ValueFlow::Value r = evaluate(op, v, delta); |
There was a problem hiding this comment.
Possible wrong value: x op= y when y only has constraints.
The call site says "its constraints make the result unknown", but this code doesn't check for that. It passes rhs.front() as delta, and after this PR rhs.front() is often a constraint, for example after if (y > 3) or if (y != 0).
Take x = 5; if (y != 0) x += y;. Here delta is "y == 0 is impossible". evaluate("+", 5, impossible 0) returns "5 is impossible", and the loop copies intvalue (and bound) into v. v keeps its possible/known kind, so x ends up recorded as exactly 5.
With y > 3, the range path in evaluate() returns an impossible bound. v then becomes a possible value of 8 with Bound::Upper, which getIntValue() reports as x == 8.
The old code had the same problem for impossible points, but this PR records constraints much more often. Suggested fix: forget the variable when the result's kind doesn't match the operand's, e.g. if (r.isUninitValue() || r.isImpossible() != v.isImpossible()) return forget(lhs);. Or require singleValue(rhs, false) at the call site. Please also add a test like the one above in executeCompoundAssignment.
|
Reviewed the One issue, commented inline: in Minor, no change requested: an expression marked unknown ( I could not build or run the branch here, so this review is from reading the code only. 🤖 Generated with Claude Code |
I fixed most of the regression. There is on case with rpp that is FN, but it requires a much larger fix, so I opened the issue 15101 for that: |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
Two new false positives found by testing; neither is in the existing comments. No crash, hang or failing test.
- Unsigned
c - xconditions (lib/valueflow.cpp,solveExprValue): the newinvertBound()is correct for signedxand removes wrong results that main had. For an unsignedxthe subtraction wraps, so64 - used > 8does not implyused < 56. The PR now reportsCondition 'used>64' is always falseand a definiteNull pointer dereference, where main gave only a "Possible null pointer dereference". Main was also wrong for this pattern, in the other direction, so this changes which false positives appear rather than introducing the class. - Ranges that the type pins to one value:
unsigned char c; if (c > 254) { if (c == 255) d = 1; return 10 / d; }now givesDivision by zero. Main did not warn, because it treatedcas exactly 255. This is a narrow case.
Otherwise the PR and main gave identical output on 33 varied nested-condition cases, and the other differences I saw were removed false positives or correct new "possible" warnings. A synthetic file with many if (x == N) return; lines was about 2.7× slower with the PR, but the growth is linear.
Testing
Build: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both succeeded.
Unit tests (PR): ./testrunner for TestProgramMemory, TestValueFlow, TestCondition, TestNullPointer and TestToken all ran with 0 failures. No Python tests in test/cli are touched by the PR.
Own tests: about 110 small functions in /work/scratch/t, run through both binaries with cppcheck -q --enable=all --inconclusive --template='{line}:{id}:{message}' and diffed. They cover nested conditions, compound assignment, ++/--, unary minus, casts, container size and empty, function calls and loops.
1. New false positive: unsigned c - x
unsigned r1(unsigned len) { if (16 - len > 8) { if (len >= 8) return 1; } return 0; }
void r2(unsigned u) { if (10 - u >= 4) { if (u == 20) {} } }
int r3(unsigned used) { int *p = 0; int x = 0; if (64 - used > 8) { if (used > 64) p = &x; return *p; } return 0; }cppcheck -q --enable=all --inconclusive --template='{line}:{id}:{message}' g.c
main:
1:knownConditionTrueFalse:Condition 'len>=8' is always true
3:nullPointer:Possible null pointer dereference: p
PR:
1:knownConditionTrueFalse:Condition 'len>=8' is always false
2:knownConditionTrueFalse:Condition 'u==20' is always false
3:knownConditionTrueFalse:Condition 'used>64' is always false
3:nullPointer:Null pointer dereference: p
All four PR lines are wrong. For example, used = 100 gives 64 - 100 = 4294967260 > 8, so used > 64 is true there. Main's line 1 was wrong too. The same code with int or unsigned char operands is handled correctly by the PR.
2. New false positive: range that the type limits to one value
int n1(unsigned char c) { int d = 0; if (c > 254) { if (c == 255) d = 1; return 10 / d; } return 0; }cppcheck -q f.cpp
PR only (main is silent):
f.cpp:3:84: warning: Division by zero. [zerodivcond]
The lower end is fine: unsigned u; if (u < 1) { if (u == 0) ... } gives "always true" on both.
Other observations
-
Differences that are improvements or correct: a main false positive is gone for
if (x >= 0) { if (x != 0) d = 1; } if (x > 0) return 10 / d;. New warnings forx <= 3thenx > 2, and for2 <= x <= 3thenx == 3, are true positives. -
No difference on a 33-function file (containers, compound assignments, inter-procedural calls, loops), nor on
lib/programmemory.cpp,lib/checkother.cpp,lib/astutils.cpp,externals/tinyxml2/tinyxml2.cppandtest/cfg/{std.c,std.cpp,posix.c,gnu.c}(3830 identical output lines). -
Performance: a generated function with N/2 repetitions of
if (x == i) return; if (y > i) { if (x < i+1000) p = &v; }took the following times.N main PR 400 1.1 s 2.9 s 800 2.1 s 5.8 s 1600 4.2 s 11.4 s That is about 2.7× slower but linear. A file with 60 nested
x != kconditions was faster with the PR (3.6 s against 4.5 s).
Not tested: the GUI, and a UBSan build.
Automated review of 0f05eab by Claude. The pull request was built and tested locally.
| if (rhs) { | ||
| value.intvalue = intval - value.intvalue; | ||
| else | ||
| // c - x >= a <=> x <= c - a |
There was a problem hiding this comment.
AI review
Inverting the bound is right for signed x, but c - x wraps when x is unsigned, so c - x > a does not imply x < c - a. This now gives known results that are wrong:
int r3(unsigned used) { int *p = 0; int x = 0; if (64 - used > 8) { if (used > 64) p = &x; return *p; } return 0; }PR: Condition 'used>64' is always false
Null pointer dereference: p
main: Possible null pointer dereference: p
used = 100 gives 64 - 100 = 4294967260 > 8, so used > 64 is reachable. Likewise if (10 - u >= 4) { if (u == 20) {} } now reports u==20 as always false, where main was silent.
Main was also wrong for unsigned operands here, in the other direction: for if (16 - len > 8) { if (len >= 8) ... } it said "always true" and the PR says "always false". So this is not a new class of problem, but the PR turns a "possible" null pointer warning into a definite one.
Suggest not solving a bounded value through c - x when the expression is unsigned, as mayWrap() does in the executor. Only an impossible point is safe there. A test with unsigned operands would help.
| const ValueFlow::Value& v = then ? truevalue : falsevalue; | ||
| pm.setValue(vartok, impossible ? asImpossible(v) : v); | ||
| ValueFlow::Value& v = then ? truevalue : falsevalue; | ||
| // A value with a bound is a range: record it as the range of impossible values so that it |
There was a problem hiding this comment.
AI review
Recording x > N as a pure constraint loses the cases where the type leaves only one value. Main got these right by accident:
int n1(unsigned char c) { int d = 0; if (c > 254) { if (c == 255) d = 1; return 10 / d; } return 0; }PR: f.cpp:3:84: warning: Division by zero. [zerodivcond]
main: (nothing)
The lower end works (unsigned u; if (u < 1) { if (u == 0) ... is "always true"), presumably because the token already has the impossible value -1. The upper limit of the type is not combined with the recorded lower bound. This is a narrow case, but it is a new false positive. It could be fixed by collapsing the constraint to a value when getMinMaxValues() shows that the range has one element.
sounds great to me! |
|
Test results for commit 0f05eab (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8899/ 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. |
My understanding is that some regressions are new false positives. Some of the "Regressions" indicated are false negatives I can live with those. In the future the false negatives are not indicated as "regressions". |
|
I fixed some of the false positives in the report, but some of the other ones require larger changes so I opened trac tickets for them: https://trac.cppcheck.net/ticket/15151 |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new false positive, with a reproducer, that is not in the earlier comments. Clamping a variable inside an enclosing range condition. Take if (len >= 0) { if (len > MAX) len = MAX; if (len <= MAX) p = buf; return *p; }. The PR reports Possible null pointer dereference: p, but len <= MAX always holds there. Main is silent. Main gets this right only because it records len >= 0 as len == 0: the same code without the outer condition gives the false positive on both main and the PR. Still, the PR turns a pattern that used to be quiet into a warning. The clamp-then-check pattern (if (x > N) x = N;) is common. A while (x > 0) x--; loop inside if (x >= 0) does the same.
There is also an unrelated change in the diff. It removes 4788 <location filename="build/Skrivbord-Felsu00f6k/..."> lines from 14 gui/*.ts translation files. That looks like a stray regeneration and would be easier to review as its own PR.
Apart from that I found no new problems. The overflow, compound-assignment, unsigned-wrap, unsigned char and cap issues are already in comments.md, so I left them out.
Testing
Build: make -j8 cppcheck testrunner in pr/ succeeded. base/ did not compile as it is: lib/vf_analyzers.cpp:361 uses settings.platform on a RefThunk. The PR fixes that line. I applied the same one-line fix in base so I could compare.
Unit tests (PR): ./testrunner for TestProgramMemory, TestValueFlow, TestCondition, TestNullPointer, TestType and TestToken all reported Tests failed: 0. The PR touches no Python tests in test/cli.
Own tests: about 45 small functions run through both binaries. They covered unary minus on unsigned ranges, casts of ranges, function calls returning constraints, * and / on ranges, container size and empty ranges, ++ and x = x - 1 on ranges, for/while loops and pointer casts. Apart from the regression below, output was identical to main, or the PR's new warnings were correct. For example, x > 3 then x / 2 == 2 now correctly gives a possible null dereference.
New false positive (regression against main):
#define MAX 16
int h1(int len, int *buf) {
int *p = 0;
if (len >= 0) {
if (len > MAX)
len = MAX;
if (len <= MAX)
p = buf;
return *p;
}
return 0;
}cppcheck -q --template='{line}:{severity}:{id}:{message}' h.c
- main: (no output)
- PR:
9:warning:nullPointer:Possible null pointer dereference: p
More variants, all silent on main and zerodivcond: Division by zero. on the PR:
int d1(int x) { int d = 0; if (x >= 0) { if (x > 0) x = 0; if (x <= 0) d = 1; return 10 / d; } return 0; }
int d3(int x) { int d = 0; if (x >= 0) { if (x > 5) x = 0; if (x <= 5) d = 1; return 10 / d; } return 0; }
int c2(int x) { int d = 0; if (x >= 0) { while (x > 0) { x--; } if (x == 0) d = 1; return 10 / d; } return 0; }Without the outer if (x >= 0), both main and the PR warn, for example int d4(int x) { int d = 0; if (x > 5) x = 0; if (x <= 5) d = 1; return 10 / d; }. So the root cause is an older analyzer limitation at the join after a branch that changes the variable. Main hid it inside a range condition by treating x >= 0 as x == 0, and the PR no longer hides it.
Automated review of 356a9dc by Claude. The pull request was built and tested locally.
| ValueFlow::Value& v = then ? truevalue : falsevalue; | ||
| // A value with a bound is a range: record it as the range of impossible values so that it | ||
| // constrains the expression instead of standing in for its value. | ||
| if (impossible || v.bound != ValueFlow::Value::Bound::Point) |
There was a problem hiding this comment.
AI review
Recording the bound as a constraint, not as a stand-in value, exposes an older false positive in the clamp pattern whenever there is an enclosing range condition. Main is silent here, and the PR warns:
int h1(int len, int *buf) {
int *p = 0;
if (len >= 0) {
if (len > 16)
len = 16;
if (len <= 16)
p = buf;
return *p; // PR: Possible null pointer dereference: p
}
return 0;
}The same thing happens with if (x >= 0) { while (x > 0) x--; if (x == 0) d = 1; return 10 / d; }, which gives zerodivcond. Without the outer if (len >= 0), main warns too. The real gap is that after if (len > 16) len = 16; the analyzer does not combine the assigned value with the len <= 16 constraint from the false branch. Main hid that gap because it took len >= 0 to mean len == 0. Since clamping is common, it would be worth adding a test for this pattern, or a TODO test if the fix belongs in a follow-up.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false negative. A variable assigned in a then-only branch loses its old value inside a later range condition on the same variable, when that range overlaps both paths. In int x = 0; if (n > 5) { x = 1; } if (n > 3) return 10 / x;, n == 4 or n == 5 gives a division by zero. Main reports zerodivcond. The PR reports nothing, and --debug shows that x at line 4 now has only possible 1 instead of possible {0,1}. The null-pointer version (p = 0; if (n > 5) p = q; if (n > 3) *p;) is lost the same way. This is a simple and common pattern. It fails for every overlapping condition I tried: n > 3, n >= 0, n > -100, n == 5, !(n < 3) and n - 3 > 0. It still works for conditions on other variables and for conditions that the else-path implies (n <= 5). No test in the PR covers it.
I could not find the cause in the time I had. It is not the new else-path assume(..., Quiet | Absolute) in lib/forwardanalyzer.cpp:815: dropping Absolute, or restoring main's "assume only when the then-branch is dead", did not bring the value back. So it is probably on the ProgramMemory or condition-evaluation side.
Apart from that I found no new problems. The other differences I saw against main were removed false positives. Issues already raised in comments.md are left out.
Testing
Build: make cppcheck testrunner in pr/ succeeded. base/ does not compile as it is (lib/vf_analyzers.cpp:361, settings.platform on a RefThunk). I changed it to settings().platform, as the PR does, so I could compare.
Unit tests (PR): ./testrunner for TestProgramMemory, TestValueFlow, TestCondition, TestNullPointer, TestType, TestToken and TestOther all ended with Tests failed: 0. The PR touches no Python tests.
Own tests: about 25 functions that exercise the reworked condition fork in forwardanalyzer.cpp, run through both binaries. They covered then-blocks that change the condition variable (n = 0, n--, n *= 2, reset(&n)), else-if chains, if/else assignments, loops and clamping. The PR removes several false positives from main. For example, if (n > 5) n = 0; else if (n > 2) n = 1; if (n <= 2) d = 1; return 10/d; no longer gives zerodivcond, and p = 0; if (n > 5) n = 0; if (n > 5) *p = 1; no longer gives nullPointer. Two false positives that main already had remain: if (n > 5) n++; else n = 0; if (n > 6 || n == 0) d = 1; and if (n > 5) n = 0; if (n > 5) d = 0;.
Regression (false negative):
int f(int n) {
int x = 0;
if (n > 5) { x = 1; }
if (n > 3) return 10 / x;
return 0;
}
int g(int n, int *q) {
int *p = 0;
if (n > 5) { p = q; }
if (n > 3) return *p;
return 0;
}cppcheck -q --template='{line}:{id}:{message}' reg.c
- main:
10:nullPointer:Possible null pointer dereference: p 4:zerodivcond:Division by zero. - PR: no output
--debug at line 4: main has x possible {0,1}, the PR has x possible 1.
I replaced the second condition in f and compared the outputs:
- Main warns and the PR is silent:
n > 3,n >= 0,n > -100,n >= 5,n > 4,n == 5,!(n < 3),n - 3 > 0. - Both warn:
n <= 5,n != 9,1.
Cause: I rebuilt the PR twice with changes to forwardanalyzer.cpp. Once I dropped Assume::Absolute from the new else-path assume at line 818. Once I restored main's rule that the else-path assumes only when the then-branch is dead. Neither brought the warning back, so the cause is somewhere else in the PR.
Automated review of db2b2f2 by Claude. The pull request was built and tested locally.
| ASSERT_EQUALS(true, testValueOfX(code, 6U, 1)); | ||
| ASSERT_EQUALS(true, testValueOfX(code, 7U, 1)); | ||
|
|
||
| // a value changed in one branch is possible on both paths |
There was a problem hiding this comment.
AI review
These tests do not cover the most common shape: a then-only branch that changes the value, followed by a range condition on the same variable that overlaps both paths. That shape regresses with this PR:
int f(int n) {
int x = 0;
if (n > 5) { x = 1; }
if (n > 3) return 10 / x; // main: zerodivcond, PR: nothing
return 0;
}--debug shows x possible {0,1} at line 4 on main and only x possible 1 with the PR, although n == 4 reaches it with x == 0. The null-pointer version p = 0; if (n > 5) p = q; if (n > 3) return *p; is lost the same way. It also fails for n >= 0, n == 5, n - 3 > 0 and !(n < 3). It still works for n <= 5 and for conditions on other variables. Something like ASSERT_EQUALS(true, testValueOfX(code, 4U, 0)); for this code would catch it.
This refactors
ProgramMemoryso it can store a list of values.A condition such as
x > 3was recorded as the possible value 4 with a lower bound, and the executor then used it as ifxwere exactly 4. Nested conditions were decided wrongly, so branches were skipped and values leaked past or were lost before them. This what lead to the FP in 15042.Now it can understand these constraints better. As new values are added, it is resolved similar to
Token::addValue.