Skip to content

C++: Cleanup missingGuardAgainstOverflow #5780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 27, 2021
Merged

C++: Cleanup missingGuardAgainstOverflow #5780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05d693e
C++: Also include the assignment versions in exprThatCanOverflow.
MathiasVP Apr 27, 2021
a41e905
C++: Delete the fix that was introduced in bb447d7174141dc518e6ce9036…
MathiasVP Apr 27, 2021
04a785b
C++: Accept test changes.
MathiasVP Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,7 +1626,9 @@ private module SimpleRangeAnalysisCached {
private predicate exprThatCanOverflow(Expr e) {
e instanceof UnaryArithmeticOperation or
e instanceof BinaryArithmeticOperation or
e instanceof LShiftExpr
e instanceof AssignArithmeticOperation or
e instanceof LShiftExpr or
e instanceof AssignLShiftExpr
}

/**
Expand Down
22 changes: 10 additions & 12 deletions cpp/ql/src/semmle/code/cpp/security/Overflow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,11 @@ VariableAccess varUse(LocalScopeVariable v) { result = v.getAnAccess() }
* Holds if `e` potentially overflows and `use` is an operand of `e` that is not guarded.
*/
predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
(
convertedExprMightOverflowPositively(e)
or
// Ensure that the predicate holds when range analysis cannot determine an upper bound
upperBound(e.getFullyConverted()) = exprMaxVal(e.getFullyConverted())
) and
// Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or
// an `AssignArithmeticOperation` by the other constraints in this predicate, we know that
// `convertedExprMightOverflowPositively` will have a result even when `e` is not analyzable
// by `SimpleRangeAnalysis`.
convertedExprMightOverflowPositively(e) and
use = e.getAnOperand() and
exists(LocalScopeVariable v | use.getTarget() = v |
// overflow possible if large
Expand All @@ -126,12 +125,11 @@ predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
* Holds if `e` potentially underflows and `use` is an operand of `e` that is not guarded.
*/
predicate missingGuardAgainstUnderflow(Operation e, VariableAccess use) {
(
convertedExprMightOverflowNegatively(e)
or
// Ensure that the predicate holds when range analysis cannot determine a lower bound
lowerBound(e.getFullyConverted()) = exprMinVal(e.getFullyConverted())
) and
// Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or
// an `AssignArithmeticOperation` by the other constraints in this predicate, we know that
// `convertedExprMightOverflowNegatively` will have a result even when `e` is not analyzable
// by `SimpleRangeAnalysis`.
convertedExprMightOverflowNegatively(e) and
use = e.getAnOperand() and
exists(LocalScopeVariable v | use.getTarget() = v |
// underflow possible if use is left operand and small
Expand Down
1 change: 0 additions & 1 deletion ...test/query-tests/Security/CWE/CWE-190/semmle/extreme/ArithmeticWithExtremeValues.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@
| test.c:50:3:50:5 | sc3 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:49:9:49:16 | 127 | Extreme value |
| test.c:59:3:59:5 | sc6 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:58:9:58:16 | 127 | Extreme value |
| test.c:63:3:63:5 | sc8 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:62:9:62:16 | - ... | Extreme value |
| test.c:75:3:75:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value |
| test.c:124:9:124:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:17:118:23 | 2147483647 | Extreme value |
2 changes: 1 addition & 1 deletion cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/extreme/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void test_negatives() {
signed char sc1, sc2, sc3, sc4, sc5, sc6, sc7, sc8;

sc1 = CHAR_MAX;
sc1 += 0; // GOOD [FALSE POSITIVE]
sc1 += 0; // GOOD
sc1 += -1; // GOOD
sc2 = CHAR_MIN;
sc2 += -1; // BAD [NOT DETECTED]
Expand Down