| Dec | JAN | Feb |
| 11 | ||
| 2023 | 2024 | 2025 |
COLLECTED BY
Collection: Save Page Now Outlinks
To see all available qualifiers, see our documentation.
Sign in /SimpleRangeAnalysis::getGuardedUpperBound may calculate wrong upper bounds when overflows are involved.
This PR change the said predicate to not require that no overflows exists in the guard expression.
Sorry, something went wrong.
andersfugmann added 2commits
C++: Add test case exposing problem with overflows for upperBound pre…
d7afd86
…dicate
C++: only use upperbound if there are no overflows in the guard
3437cf2
| @@ -1549,7 +1549,8 @@ private float getGuardedUpperBound(VariableAccess guardedAccess) { | |||
| // that there is one predecessor, albeit somewhat conservative. | |||
| exists(unique(BasicBlock b | b = def.(BasicBlock).getAPredecessor())) and | |||
| guardedAccess = def.getAUse(v) and | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) and | |||
| not exists(Expr e | e = guard.getAChild+() | convertedExprMightOverflow(e)) | |||
guard including expressions that does not affect guardVa.
I was considering extending this to:
not exists(Expr e | e = guard.getAChild+() and e = guardVa.getParent*() | convertedExprMightOverflow(e))
but I was unable to construct an expression where this had any effect.
Sorry, something went wrong.
ContributorupperBoundFromGuard somehow. Is the real problem that we are overflowing in linearAccessImpl?
Sorry, something went wrong.
ContributorSorry, something went wrong.
Contributor AuthorgetTruncatedUpperBounds does handle the overflows correctly, but the addition of predicate getGuardedUpperBound was add specifically to handle cases where getTruncatedUpperBounds would widen the result inside a loop and hence miss obvious cases.
Sorry, something went wrong.
C++: Update test results in SimpleRangeAnalysis
c9c4125
jbj
reviewed
| @@ -1549,7 +1549,8 @@ private float getGuardedUpperBound(VariableAccess guardedAccess) { | |||
| // that there is one predecessor, albeit somewhat conservative. | |||
| exists(unique(BasicBlock b | b = def.(BasicBlock).getAPredecessor())) and | |||
| guardedAccess = def.getAUse(v) and | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) and | |||
| not exists(Expr e | e = guard.getAChild+() | convertedExprMightOverflow(e)) | |||
Sorry, something went wrong.
ContributorboundedFastTC with a good restriction on the starting points. It produces 34M (compressed) rows in 7MB, which is not alarming, and DCA shows no measurable impact.
Sorry, something went wrong.
cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll Outdated| @@ -1549,7 +1549,8 @@ private float getGuardedUpperBound(VariableAccess guardedAccess) { | |||
| // that there is one predecessor, albeit somewhat conservative. | |||
| exists(unique(BasicBlock b | b = def.(BasicBlock).getAPredecessor())) and | |||
| guardedAccess = def.getAUse(v) and | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) | |||
| result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch)) and | |||
| not exists(Expr e | e = guard.getAChild+() | convertedExprMightOverflow(e)) | |||
Sorry, something went wrong.
cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll Outdated Show resolved Hide resolved
" data-pjax="true" class="Link--secondary markdown-title" href="/web/20240111220421/https://github.com/github/codeql/pull/6745/commits/aebde189f80fce0895d7da037d1415b4f9c940a4">C++: Apply peer review suggestion
aebde18
Co-authored-by: Jonas Jensen <jbj@github.com>Contributor
|
I was able to construct a test case to show that the unsound treatment of overflow was not introduced with #6568 but existed already: void test_overflow() {
unsigned int x = 0xffFFffFF;
if ((x + 256) <= 512) {
out(x); // range analysis says upper bound is 256
}
}
The existing problem is much harder to solve than the amplification of it that got introduced with #6568. That's because we haven't calculated overflow information yet at the point where we determine what to do with guard phi nodes. The only general solution I can think of is to make |
Sorry, something went wrong.
jbj approved these changesrc/3.3. It's been validated with DCA and by looking at tuple counts, and all alternatives we've considered would either involve a lot more code or would remove a lot more good results.
Sorry, something went wrong.
06b36f7
into
github:main
MathiasVP
jbj
Successfully merging this pull request may close these issues.
None yet
3 participants 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.