Skip to content

Commit 18802a2

Browse files
MathiasVPMathiasVP
authored
Merge pull request #11042 from MathiasVP/simplify-buffer.qll
C++: Simplify `buffer.qll` repair
2 parents 80ef3b3 + 30f1547 commit 18802a2

5 files changed

Lines changed: 47 additions & 108 deletions

File tree

‎cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll‎

Lines changed: 41 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,6 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
2424
exists(ArrayType t | t = v.getUnspecifiedType() | not t.getArraySize() > 1)
2525
}
2626

27-
/**
28-
* Gets the expression associated with `n`. Unlike `n.asExpr()` this also gets the
29-
* expression underlying an indirect dataflow node.
30-
*/
31-
private Expr getExpr(DataFlow::Node n, boolean isIndirect) {
32-
result = n.asExpr() and isIndirect = false
33-
or
34-
result = n.asIndirectExpr() and isIndirect = true
35-
}
36-
37-
private DataFlow::Node exprNode(Expr e, boolean isIndirect) { e = getExpr(result, isIndirect) }
38-
3927
/**
4028
* Holds if `bufferExpr` is an allocation-like expression.
4129
*
@@ -85,77 +73,19 @@ private int isSource(Expr bufferExpr, Element why) {
8573
)
8674
}
8775

88-
/** Holds if the value of `n2 + delta` may be equal to the value of `n1`. */
89-
private predicate localFlowIncrStep(DataFlow::Node n1, DataFlow::Node n2, int delta) {
90-
DataFlow::localFlowStep(n1, n2) and
91-
(
92-
exists(IncrementOperation incr |
93-
n1.asIndirectExpr() = incr.getOperand() and
94-
delta = -1
95-
)
96-
or
97-
exists(DecrementOperation decr |
98-
n1.asIndirectExpr() = decr.getOperand() and
99-
delta = 1
100-
)
101-
or
102-
exists(AddExpr add, Expr e1, Expr e2 |
103-
add.hasOperands(e1, e2) and
104-
n1.asIndirectExpr() = e1 and
105-
delta = -e2.getValue().toInt()
106-
)
107-
or
108-
exists(SubExpr add, Expr e1, Expr e2 |
109-
add.hasOperands(e1, e2) and
110-
n1.asIndirectExpr() = e1 and
111-
delta = e2.getValue().toInt()
112-
)
113-
)
114-
}
115-
116-
/**
117-
* Holds if `n1` may flow to `n2` without passing through any back-edges.
118-
*
119-
* Back-edges are excluded to prevent infinite loops on examples like:
120-
* ```
121-
* while(true) { ++n; }
122-
* ```
123-
* which, when used in `localFlowStepRec`, would create infinite loop that continuously
124-
* increments the `delta` parameter.
125-
*/
126-
private predicate localFlowNotIncrStep(DataFlow::Node n1, DataFlow::Node n2) {
127-
not localFlowIncrStep(n1, n2, _) and
128-
DataFlow::localFlowStep(n1, n2) and
129-
not n1 = n2.(DataFlow::SsaPhiNode).getAnInput(true)
130-
}
131-
13276
private predicate localFlowToExprStep(DataFlow::Node n1, DataFlow::Node n2) {
133-
not exists([n1.asExpr(), n1.asIndirectExpr()]) and
134-
localFlowNotIncrStep(n1, n2)
135-
}
136-
137-
/** Holds if `mid2 + delta` may be equal to `n1`. */
138-
private predicate localFlowStepRec0(DataFlow::Node n1, DataFlow::Node mid2, int delta) {
139-
exists(DataFlow::Node mid1, int d1, int d2 |
140-
// Or we take a number of steps that adds `d1` to the pointer
141-
localFlowStepRec(n1, mid1, d1) and
142-
// followed by a step that adds `d2` to the pointer
143-
localFlowIncrStep(mid1, mid2, d2) and
144-
delta = d1 + d2
145-
)
77+
not exists(n2.asExpr()) and
78+
DataFlow::localFlowStep(n1, n2)
14679
}
14780

14881
/** Holds if `n2 + delta` may be equal to `n1`. */
149-
private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int delta) {
150-
// Either we take one or more steps that doesn't modify the size of the buffer
151-
localFlowNotIncrStep+(n1, n2) and
152-
delta = 0
153-
or
154-
exists(DataFlow::Node mid2 |
155-
// Or we step from `n1` to `mid2 + delta`
156-
localFlowStepRec0(n1, mid2, delta) and
157-
// and finally to the next `ExprNode`.
158-
localFlowToExprStep*(mid2, n2)
82+
private predicate localFlowStepToExpr(Expr e1, Expr e2) {
83+
getBufferSizeCand0(e1) and
84+
exists(DataFlow::Node n1, DataFlow::Node mid, DataFlow::Node n2 |
85+
n1.asExpr() = e1 and
86+
localFlowToExprStep*(n1, mid) and
87+
DataFlow::localFlowStep(mid, n2) and
88+
n2.asExpr() = e2
15989
)
16090
}
16191

@@ -165,6 +95,7 @@ private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int del
16595
* expression.
16696
*/
16797
private predicate step(Expr e1, Expr e2, int delta) {
98+
getBufferSizeCand0(e1) and
16899
exists(Variable bufferVar, Class parentClass, VariableAccess parentPtr, int bufferSize |
169100
e1 = parentPtr
170101
|
@@ -181,20 +112,42 @@ private predicate step(Expr e1, Expr e2, int delta) {
181112
delta = bufferSize - parentClass.getSize()
182113
)
183114
or
184-
exists(boolean isIndirect |
185-
localFlowStepRec(exprNode(e1, isIndirect), exprNode(e2, isIndirect), delta)
115+
localFlowStepToExpr(e1, e2) and
116+
delta = 0
117+
}
118+
119+
pragma[nomagic]
120+
private predicate getBufferSizeCand0(Expr e) {
121+
exists(isSource(e, _))
122+
or
123+
exists(Expr e0 |
124+
getBufferSizeCand0(e0) and
125+
step(e0, e, _)
126+
)
127+
}
128+
129+
/**
130+
* Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
131+
*
132+
* NOTE: There can be multiple `(result, why)` for a given `bufferExpr`.
133+
*/
134+
private int getBufferSizeCand(Expr bufferExpr, Element why) {
135+
getBufferSizeCand0(bufferExpr) and
136+
(
137+
result = isSource(bufferExpr, why)
138+
or
139+
exists(Expr e0, int delta, int size |
140+
size = getBufferSizeCand(e0, why) and
141+
step(e0, bufferExpr, delta) and
142+
result = size + delta
143+
)
186144
)
187145
}
188146

189147
/**
190148
* Get the size in bytes of the buffer pointed to by an expression (if this can be determined).
191149
*/
192150
int getBufferSize(Expr bufferExpr, Element why) {
193-
result = isSource(bufferExpr, why)
194-
or
195-
exists(Expr e0, int delta, int size |
196-
size = getBufferSize(e0, why) and
197-
delta = unique(int cand | step(e0, bufferExpr, cand) | cand) and
198-
result = size + delta
199-
)
151+
result = max( | | getBufferSizeCand(bufferExpr, _)) and
152+
result = getBufferSizeCand(bufferExpr, why)
200153
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/SAMATE/VeryLikelyOverrunWrite.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +0,0 @@
1-
| tests.cpp:668:9:668:14 | call to strcpy | This 'call to strcpy' operation requires 11 bytes but the destination is only 10 bytes. |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
| overflowdestination.cpp:46:2:46:7 | call to memcpy | This 'memcpy' operation accesses 128 bytes but the $@ is only 64 bytes. | overflowdestination.cpp:40:7:40:10 | dest | destination buffer |
2-
| tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
32
| tests.cpp:23:2:23:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
43
| tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | destination buffer |
5-
| tests.cpp:25:2:25:7 | call to memcpy | This 'memcpy' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:19:7:19:17 | smallbuffer | source buffer |
64
| tests.cpp:172:23:172:31 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:170:17:170:41 | {...} | array |
75
| tests.cpp:176:23:176:30 | access to array | This array indexing operation accesses byte offset 31 but the $@ is only 24 bytes. | tests.cpp:170:17:170:41 | {...} | array |
86
| tests.cpp:222:3:222:8 | call to memset | This 'memset' operation accesses 33 bytes but the $@ is only 32 bytes. | tests.cpp:214:8:214:14 | buffer1 | destination buffer |
@@ -38,36 +36,26 @@
3836
| tests.cpp:376:3:376:13 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 101 bytes. | tests.cpp:369:47:369:52 | call to malloc | array |
3937
| tests.cpp:446:3:446:24 | access to array | This array indexing operation accesses a negative index -3 on the $@. | tests.cpp:444:7:444:14 | intArray | array |
4038
| tests.cpp:454:3:454:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
41-
| tests.cpp:455:3:455:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
4239
| tests.cpp:456:3:456:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
43-
| tests.cpp:456:3:456:15 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
44-
| tests.cpp:457:3:457:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
4540
| tests.cpp:459:3:459:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
4641
| tests.cpp:461:3:461:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
4742
| tests.cpp:476:2:476:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
4843
| tests.cpp:477:2:477:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
49-
| tests.cpp:479:2:479:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
5044
| tests.cpp:481:2:481:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
5145
| tests.cpp:485:2:485:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
5246
| tests.cpp:487:2:487:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
53-
| tests.cpp:489:2:489:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
5447
| tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
5548
| tests.cpp:495:2:495:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
5649
| tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer |
5750
| tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:510:16:510:21 | call to malloc | destination buffer |
58-
| tests.cpp:520:3:520:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:503:15:503:20 | call to malloc | destination buffer |
5951
| tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
6052
| tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
6153
| tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
62-
| tests.cpp:575:7:575:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
6354
| tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
6455
| tests.cpp:579:6:579:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
6556
| tests.cpp:586:6:586:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
6657
| tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer |
6758
| unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |
68-
| unions.cpp:27:2:27:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
69-
| unions.cpp:29:2:29:7 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
70-
| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 10 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
7159
| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
7260
| unions.cpp:34:2:34:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:16:7:16:11 | large | destination buffer |
7361
| var_size_struct.cpp:71:3:71:8 | call to memset | This 'memset' operation accesses 1025 bytes but the $@ is only 1024 bytes. | var_size_struct.cpp:67:35:67:40 | call to malloc | destination buffer |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
| overflowdestination.cpp:46:20:46:30 | sizeof(<expr>) | Potential buffer-overflow: 'dest' has size 64 not 128. |
2-
| tests.cpp:23:33:23:49 | sizeof(<expr>) | Potential buffer-overflow: 'bigbuffer' has size 10 not 20. |
32
| tests.cpp:25:33:25:49 | sizeof(<expr>) | Potential buffer-overflow: 'smallbuffer' has size 10 not 20. |
43
| tests.cpp:163:3:163:11 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |
54
| tests.cpp:164:8:164:16 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 100 elements. |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ void test1()
2020
char bigbuffer[20];
2121

2222
memcpy(bigbuffer, smallbuffer, sizeof(smallbuffer)); // GOOD
23-
memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read
23+
memcpy(bigbuffer, smallbuffer, sizeof(bigbuffer)); // BAD: over-read [NOT DETECTED]
2424
memcpy(smallbuffer, bigbuffer, sizeof(smallbuffer)); // GOOD
2525
memcpy(smallbuffer, bigbuffer, sizeof(bigbuffer)); // BAD: over-write
2626
}
@@ -454,7 +454,7 @@ void test17(long long *longArray)
454454
multi[-5][5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
455455
multi[5][-5] = 0; // DUBIOUS: underrun write (this one is still within the bounds of the whole array)
456456
multi[-5][-5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
457-
multi[0][-5] = 0; // BAD: underrun write
457+
multi[0][-5] = 0; // BAD: underrun write [NOT DETECTED]
458458

459459
multi[15][5] = 0; // BAD: overrun write
460460
multi[5][15] = 0; // DUBIOUS: overrun write (this one is still within the bounds of the whole array)
@@ -476,7 +476,7 @@ void test18()
476476
p1[-1] = 0; // BAD: underrun write
477477
p2[-1] = 0; // BAD: underrun write
478478
p2++;
479-
p2[-1] = 0; // GOOD [FALSE POSITIVE]
479+
p2[-1] = 0; // GOOD
480480

481481
p3[-1] = 0; // BAD
482482
while (*p3 != 0) {
@@ -486,7 +486,7 @@ void test18()
486486

487487
p4[-1] = 0; // BAD: underrun write
488488
p4++;
489-
p4[-1] = 0; // GOOD [FALSE POSITIVE]
489+
p4[-1] = 0; // GOOD
490490

491491
p5[-1] = 0; // BAD
492492
while (*p5 != 0) {
@@ -517,7 +517,7 @@ void test19(bool b)
517517
if (b)
518518
{
519519
memset(p1, 0, 20); // BAD
520-
memset(p2, 0, 20); // GOOD [FALSE POSITIVE]
520+
memset(p2, 0, 20); // GOOD
521521
memset(p3, 0, 20); // GOOD
522522
}
523523
}
@@ -572,7 +572,7 @@ void test21(bool cond)
572572
if (cond)
573573
{
574574
ptr++;
575-
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0] [FALSE POSITIVE]
575+
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0]
576576
} else {
577577
if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1]
578578
}

0 commit comments

Comments
 (0)