Skip to content

Commit 163ecd9

Browse files
author
Dave Bartolomeo
authored
Merge pull request github#3277 from geoffw0/rangeshift
C++: Support for & and >> in SimpleRangeAnalysis
2 parents 1ecfa2e + 2e39251 commit 163ecd9

File tree

7 files changed

+264
-98
lines changed

7 files changed

+264
-98
lines changed

‎cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

Lines changed: 102 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -118,36 +118,79 @@ private string getValue(Expr e) {
118118
)
119119
}
120120

121+
/**
122+
* A bitwise `&` expression in which both operands are unsigned, or are effectively
123+
* unsigned due to being a non-negative constant.
124+
*/
125+
private class UnsignedBitwiseAndExpr extends BitwiseAndExpr {
126+
UnsignedBitwiseAndExpr() {
127+
(
128+
getLeftOperand().getFullyConverted().getType().getUnderlyingType().(IntegralType).isUnsigned() or
129+
getLeftOperand().getFullyConverted().getValue().toInt() >= 0
130+
) and
131+
(
132+
getRightOperand()
133+
.getFullyConverted()
134+
.getType()
135+
.getUnderlyingType()
136+
.(IntegralType)
137+
.isUnsigned() or
138+
getRightOperand().getFullyConverted().getValue().toInt() >= 0
139+
)
140+
}
141+
}
142+
121143
/** Set of expressions which we know how to analyze. */
122144
private predicate analyzableExpr(Expr e) {
123145
// The type of the expression must be arithmetic. We reuse the logic in
124146
// `exprMinVal` to check this.
125147
exists(exprMinVal(e)) and
126148
(
127-
exists(getValue(e).toFloat()) or
128-
e instanceof UnaryPlusExpr or
129-
e instanceof UnaryMinusExpr or
130-
e instanceof MinExpr or
131-
e instanceof MaxExpr or
132-
e instanceof ConditionalExpr or
133-
e instanceof AddExpr or
134-
e instanceof SubExpr or
135-
e instanceof AssignExpr or
136-
e instanceof AssignAddExpr or
137-
e instanceof AssignSubExpr or
138-
e instanceof CrementOperation or
139-
e instanceof RemExpr or
140-
e instanceof CommaExpr or
141-
e instanceof StmtExpr or
149+
exists(getValue(e).toFloat())
150+
or
151+
e instanceof UnaryPlusExpr
152+
or
153+
e instanceof UnaryMinusExpr
154+
or
155+
e instanceof MinExpr
156+
or
157+
e instanceof MaxExpr
158+
or
159+
e instanceof ConditionalExpr
160+
or
161+
e instanceof AddExpr
162+
or
163+
e instanceof SubExpr
164+
or
165+
e instanceof AssignExpr
166+
or
167+
e instanceof AssignAddExpr
168+
or
169+
e instanceof AssignSubExpr
170+
or
171+
e instanceof CrementOperation
172+
or
173+
e instanceof RemExpr
174+
or
175+
e instanceof CommaExpr
176+
or
177+
e instanceof StmtExpr
178+
or
142179
// A conversion is analyzable, provided that its child has an arithmetic
143180
// type. (Sometimes the child is a reference type, and so does not get
144181
// any bounds.) Rather than checking whether the type of the child is
145182
// arithmetic, we reuse the logic that is already encoded in
146183
// `exprMinVal`.
147-
exists(exprMinVal(e.(Conversion).getExpr())) or
184+
exists(exprMinVal(e.(Conversion).getExpr()))
185+
or
148186
// Also allow variable accesses, provided that they have SSA
149187
// information.
150188
exists(RangeSsaDefinition def, StackVariable v | e = def.getAUse(v))
189+
or
190+
e instanceof UnsignedBitwiseAndExpr
191+
or
192+
// `>>` by a constant
193+
exists(e.(RShiftExpr).getRightOperand().getValue())
151194
)
152195
}
153196

@@ -245,6 +288,19 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
245288
or
246289
exists(Conversion convExpr | e = convExpr | exprDependsOnDef(convExpr.getExpr(), srcDef, srcVar))
247290
or
291+
// unsigned `&`
292+
exists(UnsignedBitwiseAndExpr andExpr |
293+
andExpr = e and
294+
exprDependsOnDef(andExpr.getAnOperand(), srcDef, srcVar)
295+
)
296+
or
297+
// `>>` by a constant
298+
exists(RShiftExpr rs |
299+
rs = e and
300+
exists(rs.getRightOperand().getValue()) and
301+
exprDependsOnDef(rs.getLeftOperand(), srcDef, srcVar)
302+
)
303+
or
248304
e = srcDef.getAUse(srcVar)
249305
}
250306

@@ -641,6 +697,20 @@ private float getLowerBoundsImpl(Expr expr) {
641697
exists(RangeSsaDefinition def, StackVariable v | expr = def.getAUse(v) |
642698
result = getDefLowerBounds(def, v)
643699
)
700+
or
701+
// unsigned `&` (tighter bounds may exist)
702+
exists(UnsignedBitwiseAndExpr andExpr |
703+
andExpr = expr and
704+
result = 0.0
705+
)
706+
or
707+
// `>>` by a constant
708+
exists(RShiftExpr rsExpr, float left, int right |
709+
rsExpr = expr and
710+
left = getFullyConvertedLowerBounds(rsExpr.getLeftOperand()) and
711+
right = rsExpr.getRightOperand().getValue().toInt() and
712+
result = left / 2.pow(right)
713+
)
644714
}
645715

646716
/** Only to be called by `getTruncatedUpperBounds`. */
@@ -794,6 +864,22 @@ private float getUpperBoundsImpl(Expr expr) {
794864
exists(RangeSsaDefinition def, StackVariable v | expr = def.getAUse(v) |
795865
result = getDefUpperBounds(def, v)
796866
)
867+
or
868+
// unsigned `&` (tighter bounds may exist)
869+
exists(UnsignedBitwiseAndExpr andExpr, float left, float right |
870+
andExpr = expr and
871+
left = getFullyConvertedUpperBounds(andExpr.getLeftOperand()) and
872+
right = getFullyConvertedUpperBounds(andExpr.getRightOperand()) and
873+
result = left.minimum(right)
874+
)
875+
or
876+
// `>>` by a constant
877+
exists(RShiftExpr rsExpr, float left, int right |
878+
rsExpr = expr and
879+
left = getFullyConvertedUpperBounds(rsExpr.getLeftOperand()) and
880+
right = rsExpr.getRightOperand().getValue().toInt() and
881+
result = left / 2.pow(right)
882+
)
797883
}
798884

799885
/**

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.expected

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
| test.c:4:14:4:18 | ... < ... | Comparison between $@ of type char and $@ of wider type int. | test.c:3:7:3:7 | c | c | test.c:2:17:2:17 | x | x |
2+
| test.c:9:14:9:18 | ... > ... | Comparison between $@ of type char and $@ of wider type int. | test.c:8:7:8:7 | c | c | test.c:7:17:7:17 | x | x |
3+
| test.c:14:14:14:18 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:13:8:13:8 | s | s | test.c:12:17:12:17 | x | x |
4+
| test.c:65:14:65:18 | ... < ... | Comparison between $@ of type short and $@ of wider type int. | test.c:64:8:64:8 | s | s | test.c:63:17:63:17 | x | x |
5+
| test.c:87:14:87:18 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:84:15:84:15 | x | x |
6+
| test.c:91:14:91:23 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type int. | test.c:83:16:83:16 | c | c | test.c:91:18:91:23 | 65280 | 65280 |
7+
| test.c:93:14:93:25 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type int. | test.c:83:16:83:16 | c | c | test.c:93:18:93:25 | 16711680 | 16711680 |
8+
| test.c:95:14:95:27 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:95:18:95:27 | 4278190080 | 4278190080 |
9+
| test.c:99:14:99:29 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:99:19:99:28 | ... & ... | ... & ... |
10+
| test.c:101:14:101:31 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:101:19:101:30 | ... & ... | ... & ... |
11+
| test.c:103:14:103:33 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:103:19:103:32 | ... & ... | ... & ... |
12+
| test.c:105:14:105:25 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:105:19:105:24 | ... >> ... | ... >> ... |
13+
| test.c:107:14:107:26 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:83:16:83:16 | c | c | test.c:107:19:107:25 | ... >> ... | ... >> ... |
14+
| test.c:128:15:128:21 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:121:16:121:17 | uc | uc | test.c:123:19:123:20 | sz | sz |
15+
| test.c:139:15:139:21 | ... < ... | Comparison between $@ of type unsigned char and $@ of wider type unsigned int. | test.c:121:16:121:17 | uc | uc | test.c:123:19:123:20 | sz | sz |

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/wider_type/ComparisonWithWiderType.qlref renamed to ‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/ComparisonWithWiderType.qlref

File renamed without changes.

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test.c

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
2+
void test1 (int x) {
3+
char c;
4+
for (c = 0; c < x; c++) {} //BAD
5+
}
6+
7+
void test2 (int x) {
8+
char c;
9+
for (c = 0; x > c; c++) {} // BAD
10+
}
11+
12+
void test3 (int x) {
13+
short s;
14+
for (s = 0; s < x; s++) {} //BAD
15+
}
16+
17+
void runner() { // get range analysis to give large values to x in tests
18+
test1(65536);
19+
test2(65536);
20+
test3(655360);
21+
test7((unsigned long long)1<<48);
22+
test8(65536);
23+
test9(65536);
24+
test10(65536);
25+
26+
}
27+
28+
void test4 () {
29+
short s1;
30+
short s2 = 200;
31+
for (s1 = 0; s1 < s2; s1++) {}
32+
}
33+
34+
void test5 () {
35+
short s1;
36+
int x = 65536;
37+
s1 < x;
38+
}
39+
40+
void test6() {
41+
short s1;
42+
for (s1 = 0; s1 < 0x0000ffff; s1++) {}
43+
}
44+
45+
void test7(long long l) {
46+
int i;
47+
for (i = 0; i < l; i++) {}
48+
}
49+
50+
void test8(int x) {
51+
short s;
52+
for (s = 256; s < x; x--) {}
53+
}
54+
55+
56+
void test9(int x) {
57+
short s;
58+
for (s = 256; s < x; ) {
59+
x--;
60+
}
61+
}
62+
63+
void test10(int x) {
64+
short s;
65+
for (s = 0; s < x; ) { // BAD
66+
do
67+
{
68+
s++;
69+
} while (0);
70+
}
71+
}
72+
73+
extern const int const256;
74+
75+
void test11() {
76+
short s;
77+
for(s = 0; s < const256; ++s) {}
78+
}
79+
80+
unsigned int get_a_uint();
81+
82+
void test12() {
83+
unsigned char c;
84+
unsigned int x;
85+
86+
x = get_a_uint();
87+
for (c = 0; c < x; c++) {} // BAD
88+
x = get_a_uint();
89+
for (c = 0; c < 0xFF; c++) {} // GOOD
90+
x = get_a_uint();
91+
for (c = 0; c < 0xFF00; c++) {} // BAD
92+
x = get_a_uint();
93+
for (c = 0; c < 0xFF0000; c++) {} // BAD
94+
x = get_a_uint();
95+
for (c = 0; c < 0xFF000000; c++) {} // BAD
96+
x = get_a_uint();
97+
for (c = 0; c < (x & 0xFF); c++) {} // GOOD
98+
x = get_a_uint();
99+
for (c = 0; c < (x & 0xFF00); c++) {} // BAD
100+
x = get_a_uint();
101+
for (c = 0; c < (x & 0xFF0000); c++) {} // BAD
102+
x = get_a_uint();
103+
for (c = 0; c < (x & 0xFF000000); c++) {} // BAD
104+
x = get_a_uint();
105+
for (c = 0; c < (x >> 8); c++) {} // BAD
106+
x = get_a_uint();
107+
for (c = 0; c < (x >> 16); c++) {} // BAD
108+
x = get_a_uint();
109+
for (c = 0; c < (x >> 24); c++) {} // GOOD (assuming 32-bit ints)
110+
x = get_a_uint();
111+
for (c = 0; c < ((x & 0xFF00) >> 8); c++) {} // GOOD
112+
x = get_a_uint();
113+
for (c = 0; c < ((x & 0xFF0000) >> 16); c++) {} // GOOD
114+
x = get_a_uint();
115+
for (c = 0; c < ((x & 0xFF000000) >> 24); c++) {} // GOOD
116+
}
117+
118+
int get_an_int();
119+
120+
void test13() {
121+
unsigned char uc;
122+
int sx, sy;
123+
unsigned ux, uy, sz;
124+
125+
ux = get_a_uint();
126+
uy = get_a_uint();
127+
sz = ux & uy;
128+
for (uc = 0; uc < sz; uc++) {} // BAD
129+
130+
ux = get_a_uint();
131+
uy = get_a_uint();
132+
if (ux > 128) {ux = 128;}
133+
sz = ux & uy;
134+
for (uc = 0; uc < sz; uc++) {} // GOOD
135+
136+
sx = get_an_int();
137+
sy = get_an_int();
138+
sz = (unsigned)sx & (unsigned)sy;
139+
for (uc = 0; uc < sz; uc++) {} // BAD
140+
141+
sx = get_an_int();
142+
sy = get_an_int();
143+
if (sx < 0) {sx = 0;}
144+
if (sx > 128) {sx = 128;}
145+
sz = (unsigned)sx & (unsigned)sy;
146+
for (uc = 0; uc < sz; uc++) {} // GOOD
147+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/wider_type/test2.c renamed to ‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ComparisonWithWiderType/test2.c

File renamed without changes.

‎cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/wider_type/ComparisonWithWiderType.expected

Lines changed: 0 additions & 4 deletions
This file was deleted.

0 commit comments

Comments
 (0)