Skip to content

Commit b51dc68

Browse files
lcarteylcartey
committed
IntegerOverflow: Implement INT31-C
Adds a query to detect conversions which could potentially lead to data loss. This covers both explicit/implicit casts, and also calls to functions which internal convert values.
1 parent 5b9e572 commit b51dc68

File tree

5 files changed

+228
-0
lines changed

5 files changed

+228
-0
lines changed

‎c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.md

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# INT31-C: Ensure that integer conversions do not result in lost or misinterpreted data
2+
3+
This query implements the CERT-C rule INT31-C:
4+
5+
> Ensure that integer conversions do not result in lost or misinterpreted data
6+
7+
8+
## CERT
9+
10+
** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` **
11+
12+
## Implementation notes
13+
14+
None
15+
16+
## References
17+
18+
* CERT-C: [INT31-C: Ensure that integer conversions do not result in lost or misinterpreted data](https://wiki.sei.cmu.edu/confluence/display/c)

‎c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.ql

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/**
2+
* @id c/cert/integer-conversion-causes-data-loss
3+
* @name INT31-C: Ensure that integer conversions do not result in lost or misinterpreted data
4+
* @description
5+
* @kind problem
6+
* @precision high
7+
* @problem.severity error
8+
* @tags external/cert/id/int31-c
9+
* correctness
10+
* external/cert/obligation/rule
11+
*/
12+
13+
import cpp
14+
import codingstandards.c.cert
15+
16+
class IntegerConversion extends Expr {
17+
private IntegralType castedToType;
18+
private Expr preConversionExpr;
19+
20+
IntegerConversion() {
21+
// This is an explicit cast
22+
castedToType = this.(Cast).getActualType() and
23+
preConversionExpr = this.(Cast).getExpr()
24+
or
25+
// Functions that internally cast an argument to unsigned char
26+
castedToType instanceof UnsignedCharType and
27+
this = preConversionExpr and
28+
exists(FunctionCall call, string name | call.getTarget().hasGlobalOrStdName(name) |
29+
name = ["ungetc", "fputc"] and
30+
this = call.getArgument(0)
31+
or
32+
name = ["memset", "memchr"] and
33+
this = call.getArgument(1)
34+
or
35+
name = "memset_s" and
36+
this = call.getArgument(2)
37+
)
38+
}
39+
40+
Expr getPreConversionExpr() { result = preConversionExpr }
41+
42+
Type getCastedToType() { result = castedToType }
43+
}
44+
45+
bindingset[value]
46+
predicate withinIntegralRange(IntegralType typ, float value) {
47+
exists(float lb, float ub, float limit |
48+
limit = 2.pow(8 * typ.getSize()) and
49+
(
50+
if typ.isUnsigned()
51+
then (
52+
lb = 0 and ub = limit - 1
53+
) else (
54+
lb = -limit / 2 and
55+
ub = (limit / 2) - 1
56+
)
57+
) and
58+
value >= lb and
59+
value <= ub
60+
)
61+
}
62+
63+
from IntegerConversion c, Expr preConversionExpr
64+
where
65+
not isExcluded(c, IntegerOverflowPackage::integerConversionCausesDataLossQuery()) and
66+
preConversionExpr = c.getPreConversionExpr() and
67+
// Casting from an integral type
68+
preConversionExpr.getType().getUnspecifiedType() instanceof IntegralType and
69+
// Where the result is not within the range of the target type
70+
(
71+
not withinIntegralRange(c.getCastedToType(), lowerBound(preConversionExpr)) or
72+
not withinIntegralRange(c.getCastedToType(), upperBound(preConversionExpr))
73+
) and
74+
// A conversion of `-1` to `time_t` is permitted by the standard
75+
not (
76+
c.getType().getUnspecifiedType().hasName("time_t") and
77+
preConversionExpr.getValue() = "-1"
78+
) and
79+
// Conversion to unsigned char is permitted from the range [SCHAR_MIN..UCHAR_MAX], as those can
80+
// legitimately represent characters
81+
not (
82+
c.getType().getUnspecifiedType() instanceof UnsignedCharType and
83+
lowerBound(preConversionExpr) >= typeLowerBound(any(SignedCharType s)) and
84+
upperBound(preConversionExpr) <= typeUpperBound(any(UnsignedCharType s))
85+
)
86+
select c,
87+
"Conversion from " + c.getPreConversionExpr().getType() + " to " + c.getCastedToType() +
88+
" may cause data loss."

‎c/cert/test/rules/INT31-C/IntegerConversionCausesDataLoss.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
WARNING: Unused predicate test (/home/luke/git/codeql-coding-standards/c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.ql:63,11-15)
2+
WARNING: Unused predicate withinIntegralRange (/home/luke/git/codeql-coding-standards/c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.ql:65,11-30)
3+
| test.c:7:3:7:15 | (signed int)... | Conversion from unsigned int to signed int may cause data loss. |
4+
| test.c:17:3:17:17 | (unsigned int)... | Conversion from signed int to unsigned int may cause data loss. |
5+
| test.c:34:3:34:17 | (signed short)... | Conversion from signed int to signed short may cause data loss. |
6+
| test.c:51:3:51:19 | (unsigned short)... | Conversion from unsigned int to unsigned short may cause data loss. |
7+
| test.c:89:3:89:19 | (unsigned char)... | Conversion from signed int to unsigned char may cause data loss. |
8+
| test.c:92:3:92:19 | (unsigned char)... | Conversion from signed int to unsigned char may cause data loss. |
9+
| test.c:93:3:93:19 | (unsigned char)... | Conversion from signed int to unsigned char may cause data loss. |
10+
| test.c:97:9:97:12 | 4096 | Conversion from int to unsigned char may cause data loss. |
11+
| test.c:99:10:99:13 | 4096 | Conversion from int to unsigned char may cause data loss. |
12+
| test.c:101:13:101:16 | 4096 | Conversion from int to unsigned char may cause data loss. |
13+
| test.c:103:13:103:16 | 4096 | Conversion from int to unsigned char may cause data loss. |

‎c/cert/test/rules/INT31-C/IntegerConversionCausesDataLoss.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
rules/INT31-C/IntegerConversionCausesDataLoss.ql

‎c/cert/test/rules/INT31-C/test.c

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
#include <limits.h>
2+
#include <stddef.h>
3+
#include <stdio.h>
4+
#include <string.h>
5+
6+
void test_unsigned_to_signed(unsigned int x) {
7+
(signed int)x; // NON_COMPLIANT - not larger enough to represent all
8+
}
9+
10+
void test_unsigned_to_signed_check(unsigned int x) {
11+
if (x <= INT_MAX) {
12+
(signed int)x; // COMPLIANT
13+
}
14+
}
15+
16+
void test_signed_to_unsigned(signed int x) {
17+
(unsigned int)x; // NON_COMPLIANT - not large enough to represent all
18+
}
19+
20+
void test_signed_to_unsigned_check(signed int x) {
21+
if (x >= 0) {
22+
(unsigned int)x; // COMPLIANT
23+
}
24+
}
25+
26+
void test_signed_to_unsigned_check2(signed int x) {
27+
if (x < 0) {
28+
} else {
29+
(unsigned int)x; // COMPLIANT
30+
}
31+
}
32+
33+
void test_signed_loss_of_precision(signed int x) {
34+
(signed short)x; // NON_COMPLIANT - not large enough to represent all
35+
}
36+
37+
void test_signed_loss_of_precision_check(signed int x) {
38+
if (x >= SHRT_MIN && x <= SHRT_MAX) {
39+
(signed short)x; // COMPLIANT
40+
}
41+
}
42+
43+
void test_signed_loss_of_precision_check2(signed int x) {
44+
if (x < SHRT_MIN || x > SHRT_MAX) {
45+
} else {
46+
(signed short)x; // COMPLIANT
47+
}
48+
}
49+
50+
void test_unsigned_loss_of_precision(unsigned int x) {
51+
(unsigned short)x; // NON_COMPLIANT - not large enough to represent all
52+
}
53+
54+
void test_unsigned_loss_of_precision_check(unsigned int x) {
55+
if (x <= USHRT_MAX) {
56+
(unsigned short)x; // COMPLIANT
57+
}
58+
}
59+
60+
void test_unsigned_loss_of_precision_check2(unsigned int x) {
61+
if (x > USHRT_MAX) {
62+
} else {
63+
(unsigned short)x; // COMPLIANT
64+
}
65+
}
66+
67+
// We create a fake stub here to test the case
68+
// that time_t is an unsigned type.
69+
typedef unsigned int time_t;
70+
time_t time(time_t *seconds);
71+
72+
void test_time_t_check_against_zero(time_t x) {
73+
time_t now = time(0);
74+
if (now != -1) { // NON_COMPLIANT[FALSE_NEGATIVE] - there is no conversion
75+
// here in our model
76+
}
77+
if (now != (time_t)-1) { // COMPLIANT
78+
}
79+
}
80+
81+
void test_chars() {
82+
signed int i1 = 'A';
83+
signed int i2 = 100000;
84+
signed int i3 = -128;
85+
signed int i4 = 255;
86+
signed int i5 = -129;
87+
signed int i6 = 256;
88+
(unsigned char)i1; // COMPLIANT
89+
(unsigned char)i2; // NON_COMPLIANT
90+
(unsigned char)i3; // COMPLIANT
91+
(unsigned char)i4; // COMPLIANT
92+
(unsigned char)i5; // NON_COMPLIANT
93+
(unsigned char)i6; // NON_COMPLIANT
94+
}
95+
96+
void test_funcs(int *a, size_t n) {
97+
fputc(4096, stdout); // NON_COMPLIANT
98+
fputc('A', stdout); // COMPLIANT
99+
ungetc(4096, stdin); // NON_COMPLIANT
100+
ungetc('A', stdin); // COMPLIANT
101+
memchr(a, 4096, n); // NON_COMPLIANT
102+
memchr(a, 'A', n); // COMPLIANT
103+
memset(a, 4096, n); // NON_COMPLIANT
104+
memset(a, 0, n); // COMPLIANT
105+
// not supported in our stdlib, or in any of the compilers
106+
// memset_s(a, rn, 4096, n); // NON_COMPLIANT
107+
// memset_s(a, rn, 0, n); // COMPLIANT
108+
}

0 commit comments

Comments
 (0)