-
Notifications
You must be signed in to change notification settings - Fork 67
Implement IntegerOverflow
package
#263
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.
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
Changes from 32 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
23065e4
IntegerOverflow: Create an overflow library
lcartey 0ffa4b1
IntegerOverflow: Add package files.
lcartey 03cb601
IntegerOverflow: Add query for INT30-C
lcartey c4ff2de
IntegerOverflow: Implement INT33-C
lcartey 195cc3f
IntegerOverflow: Implement Rule 12.4.
lcartey 9580891
Fix typo.
lcartey e9c0c18
IntegerOverflow: Do not exclude casted expressions
lcartey d64b125
Macro: Add classes for library vs user macro.
lcartey a845f38
IntegerOverflow: Exclude macro results
lcartey 3f2db7a
IntegerOverflow: clarify valid post-check
lcartey 37df9a6
IntegerOverflow: Implement INT32-C
lcartey 6dd30df
IntegerOverflow: Support MulExpr
lcartey 5b9e572
INT32-C: Report issues in guards
lcartey b51dc68
IntegerOverflow: Implement INT31-C
lcartey 9dfc200
INT31-C: Exclude stdbool.h
lcartey 7f672bf
INT31-C: Add better message for typedefs
lcartey 9afa81e
INT31-C: Improve error message to include range
lcartey 417514c
IntegerOverflow: Implement INT35-C
lcartey f7951fb
IntegerOverflow: Improve descriptions.
lcartey 6d471f2
IntegerOverflow: Update help
lcartey f5bfa52
Regenerate help files to resolve format issue
3cede45
Revert "Regenerate help files to resolve format issue"
1bdbdcd
Fix rule help format issue
57cf6ec
Merge branch 'main' into lcartey/integer-overflow
697e2e2
IntegerOverflow: Expand supported operations
lcartey bf37105
IntegerOverflow: Support divide and remainder
lcartey d3dc330
IntegerOverflow: Further restrictions
lcartey 6490705
INT32-C: Add tests for ++,--
lcartey 8a67704
IntegerOverflow: INT30-C to medium
lcartey cb38c60
Merge branch 'main' into lcartey/integer-overflow
lcartey e4b6417
INT33-C: Include assign div/mod
lcartey d5b7fe5
Fix typo.
lcartey b455965
Merge branch 'main' into lcartey/integer-overflow
lcartey 3a33a22
IntegerOverflow: recognise safe crements
lcartey ba3bf2b
IntgerOverflow: Expose getting a valid post check.
lcartey 68117b7
IntegerOverflow: Support for more guards
lcartey a2c8fe3
Add change note
lcartey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
249 changes: 249 additions & 0 deletions
249
c/cert/src/rules/INT30-C/UnsignedIntegerOperationsWrapAround.md
Large diffs are not rendered by default.
Oops, something went wrong.
38 changes: 38 additions & 0 deletions
38
c/cert/src/rules/INT30-C/UnsignedIntegerOperationsWrapAround.ql
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/** | ||
* @id c/cert/unsigned-integer-operations-wrap-around | ||
* @name INT30-C: Ensure that unsigned integer operations do not wrap | ||
* @description Unsigned integer expressions do not strictly overflow, but instead wrap around in a | ||
* modular way. If the size of the type is not sufficient, this can happen | ||
* unexpectedly. | ||
* @kind problem | ||
* @precision medium | ||
* @problem.severity error | ||
* @tags external/cert/id/int30-c | ||
* correctness | ||
* security | ||
* external/cert/obligation/rule | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.cert | ||
import codingstandards.cpp.Overflow | ||
import semmle.code.cpp.controlflow.Guards | ||
import semmle.code.cpp.valuenumbering.GlobalValueNumbering | ||
|
||
from InterestingOverflowingOperation op | ||
where | ||
not isExcluded(op, IntegerOverflowPackage::unsignedIntegerOperationsWrapAroundQuery()) and | ||
op.getType().getUnderlyingType().(IntegralType).isUnsigned() and | ||
// Not within a guard condition | ||
not exists(GuardCondition gc | gc.getAChild*() = op) and | ||
// Not guarded by a check, where the check is not an invalid overflow check | ||
not op.hasValidPreCheck() and | ||
// Is not checked after the operation | ||
not op.hasValidPostCheck() and | ||
// Permitted by exception 3 | ||
not op instanceof LShiftExpr and | ||
// Permitted by exception 2 - zero case is handled in separate query | ||
not op instanceof DivExpr and | ||
not op instanceof RemExpr | ||
select op, | ||
"Operation " + op.getOperator() + " of type " + op.getType().getUnderlyingType() + " may wrap." |
361 changes: 361 additions & 0 deletions
361
c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.md
Large diffs are not rendered by default.
Oops, something went wrong.
107 changes: 107 additions & 0 deletions
107
c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.ql
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
/** | ||
* @id c/cert/integer-conversion-causes-data-loss | ||
* @name INT31-C: Ensure that integer conversions do not result in lost or misinterpreted data | ||
* @description Converting an integer value to another integer type with a different sign or size | ||
* can lead to data loss or misinterpretation of the value. | ||
* @kind problem | ||
* @precision medium | ||
* @problem.severity error | ||
* @tags external/cert/id/int31-c | ||
* correctness | ||
* external/cert/obligation/rule | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.cert | ||
|
||
class IntegerConversion extends Expr { | ||
private IntegralType castedToType; | ||
private Expr preConversionExpr; | ||
|
||
IntegerConversion() { | ||
// This is an explicit cast | ||
castedToType = this.(Cast).getType().getUnspecifiedType() and | ||
preConversionExpr = this.(Cast).getExpr() | ||
or | ||
// Functions that internally cast an argument to unsigned char | ||
castedToType instanceof UnsignedCharType and | ||
this = preConversionExpr and | ||
exists(FunctionCall call, string name | call.getTarget().hasGlobalOrStdName(name) | | ||
name = ["ungetc", "fputc"] and | ||
this = call.getArgument(0) | ||
or | ||
name = ["memset", "memchr"] and | ||
this = call.getArgument(1) | ||
or | ||
name = "memset_s" and | ||
this = call.getArgument(2) | ||
) | ||
} | ||
|
||
Expr getPreConversionExpr() { result = preConversionExpr } | ||
|
||
Type getCastedToType() { result = castedToType } | ||
} | ||
|
||
bindingset[value] | ||
predicate withinIntegralRange(IntegralType typ, float value) { | ||
exists(float lb, float ub, float limit | | ||
limit = 2.pow(8 * typ.getSize()) and | ||
( | ||
if typ.isUnsigned() | ||
then ( | ||
lb = 0 and ub = limit - 1 | ||
) else ( | ||
lb = -limit / 2 and | ||
ub = (limit / 2) - 1 | ||
) | ||
) and | ||
value >= lb and | ||
value <= ub | ||
) | ||
} | ||
|
||
from | ||
IntegerConversion c, Expr preConversionExpr, Type castedToType, Type castedFromType, | ||
IntegralType unspecifiedCastedFromType, string typeFromMessage, float preConversionLowerBound, | ||
float preConversionUpperBound, float typeLowerBound, float typeUpperBound | ||
where | ||
not isExcluded(c, IntegerOverflowPackage::integerConversionCausesDataLossQuery()) and | ||
preConversionExpr = c.getPreConversionExpr() and | ||
castedFromType = preConversionExpr.getType() and | ||
// Casting from an integral type | ||
unspecifiedCastedFromType = castedFromType.getUnspecifiedType() and | ||
// Casting to an integral type | ||
castedToType = c.getCastedToType() and | ||
// Get the upper/lower bound of the pre-conversion expression | ||
preConversionLowerBound = lowerBound(preConversionExpr) and | ||
preConversionUpperBound = upperBound(preConversionExpr) and | ||
// Get the upper/lower bound of the target type | ||
typeLowerBound = typeLowerBound(castedToType) and | ||
typeUpperBound = typeUpperBound(castedToType) and | ||
// Where the result is not within the range of the target type | ||
( | ||
not withinIntegralRange(castedToType, preConversionLowerBound) or | ||
not withinIntegralRange(castedToType, preConversionUpperBound) | ||
) and | ||
// A conversion of `-1` to `time_t` is permitted by the standard | ||
not ( | ||
c.getType().getUnspecifiedType().hasName("time_t") and | ||
preConversionExpr.getValue() = "-1" | ||
) and | ||
// Conversion to unsigned char is permitted from the range [SCHAR_MIN..UCHAR_MAX], as those can | ||
// legitimately represent characters | ||
not ( | ||
c.getType().getUnspecifiedType() instanceof UnsignedCharType and | ||
lowerBound(preConversionExpr) >= typeLowerBound(any(SignedCharType s)) and | ||
upperBound(preConversionExpr) <= typeUpperBound(any(UnsignedCharType s)) | ||
) and | ||
not castedToType instanceof BoolType and | ||
// Create a helpful message | ||
if castedFromType = unspecifiedCastedFromType | ||
then typeFromMessage = castedFromType.toString() | ||
else typeFromMessage = castedFromType + " (" + unspecifiedCastedFromType + ")" | ||
select c, | ||
"Conversion from " + typeFromMessage + " to " + castedToType + | ||
" may cause data loss (casting from range " + preConversionLowerBound + "..." + | ||
preConversionUpperBound + " to range " + typeLowerBound + "..." + typeUpperBound + ")." |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.