Repository navigation
Conversation
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 |
| 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
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! |
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.