Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Shifts: calculate() rejects a negative or too large shift and a negative value | ||
| bool error = false; | ||
| result = calculate(op, edge, k, &error); | ||
| if (!rangeIsLhs || error || (op == "<<" && (result >> k) != edge)) |
| // Shifts: calculate() rejects a negative or too large shift and a negative value | ||
| bool error = false; | ||
| result = calculate(op, edge, k, &error); | ||
| if (!rangeIsLhs || error || (op == "<<" && (result >> k) != edge)) |
| // known value and does not depend on a tracked value | ||
| const Values* getStoredValues(const Token* expr) const | ||
| { | ||
| if (expr->exprId() == 0) |
| const ValueFlow::Value& v = utils::as_const(*pm).at(expr->exprId()); | ||
| if (v.isIntValue()) | ||
| return v; | ||
| if (const ValueFlow::Value* v = pm->getValue(expr->exprId(), /*impossible*/ true)) { |
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.
Comment on lines
+879
to
+883
| if (op == "+") { | ||
| result = edge + k; | ||
| } else if (op == "-") { | ||
| increasing = rangeIsLhs; | ||
| result = rangeIsLhs ? edge - k : k - edge; |
Comment on lines
+186
to
+187
| values.push_back(value); | ||
| Token::removeContradictions(values); |
Comment on lines
1807
to
+1811
| const ValueFlow::Value* sizeValue = pm->getValue(value.tokvalue->exprId()); | ||
| if (sizeValue && sizeValue->isContainerSizeValue()) | ||
| return *sizeValue; | ||
| if (sizeValue && sizeValue->isContainerSizeValue()) { | ||
| sizes.push_back(*sizeValue); | ||
| break; | ||
| } |
Comment on lines
+1905
to
+1910
| // 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--; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.