Skip to content

Commit a0018c9

Browse files
MathiasVPMathiasVP
authored
Merge pull request #14193 from MathiasVP/fully-converted-expressions-for-flow-after-free
C++: Use fully converted expressions for `cpp/use-after-free` and `cpp/double-free`
2 parents bd1d6e1 + 0508092 commit a0018c9

6 files changed

Lines changed: 20 additions & 26 deletions

File tree

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,13 @@ private module GetConvertedResultExpression {
11131113
result = tas.getExtent().getExpr() and
11141114
instr = tas.getInstruction(any(AllocationExtentConvertTag tag))
11151115
)
1116+
or
1117+
// There's no instruction that returns `ParenthesisExpr`, but some queries
1118+
// expect this
1119+
exists(TranslatedTransparentConversion ttc |
1120+
result = ttc.getExpr().(ParenthesisExpr) and
1121+
instr = ttc.getResult()
1122+
)
11161123
}
11171124

11181125
private Expr getConvertedResultExpressionImpl(Instruction instr) {

‎cpp/ql/src/Critical/FlowAfterFree.qll‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,11 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
9898
* is being freed by a deallocation expression `dealloc`.
9999
*/
100100
predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
101-
e = dealloc.getFreedExpr() and
102-
e = n.asExpr() and
101+
exists(Expr conv |
102+
e = conv.getUnconverted() and
103+
conv = dealloc.getFreedExpr().getFullyConverted() and
104+
conv = n.asConvertedExpr()
105+
) and
103106
// Ignore realloc functions
104107
not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())
105108
}

‎cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ deprecated class PossibleYearArithmeticOperationCheckConfiguration extends Taint
296296
}
297297

298298
override predicate isSource(DataFlow::Node source) {
299-
exists(Operation op | op = source.asConvertedExpr() |
299+
exists(Operation op | op = source.asExpr() |
300300
op.getAChild*().getValue().toInt() = 365 and
301301
(
302302
not op.getParent() instanceof Expr or
@@ -321,7 +321,7 @@ deprecated class PossibleYearArithmeticOperationCheckConfiguration extends Taint
321321

322322
override predicate isSink(DataFlow::Node sink) {
323323
exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr |
324-
aexpr.getRValue() = sink.asConvertedExpr()
324+
aexpr.getRValue() = sink.asExpr()
325325
|
326326
(dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and
327327
fa.getQualifier().getUnderlyingType() = dds and
@@ -336,7 +336,7 @@ deprecated class PossibleYearArithmeticOperationCheckConfiguration extends Taint
336336
*/
337337
private module PossibleYearArithmeticOperationCheckConfig implements DataFlow::ConfigSig {
338338
predicate isSource(DataFlow::Node source) {
339-
exists(Operation op | op = source.asConvertedExpr() |
339+
exists(Operation op | op = source.asExpr() |
340340
op.getAChild*().getValue().toInt() = 365 and
341341
(
342342
not op.getParent() instanceof Expr or
@@ -361,7 +361,7 @@ private module PossibleYearArithmeticOperationCheckConfig implements DataFlow::C
361361

362362
predicate isSink(DataFlow::Node sink) {
363363
exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr |
364-
aexpr.getRValue() = sink.asConvertedExpr()
364+
aexpr.getRValue() = sink.asExpr()
365365
|
366366
(dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and
367367
fa.getQualifier().getUnderlyingType() = dds and

‎cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ edges
1111
| test_free.cpp:128:10:128:11 | * ... | test_free.cpp:129:10:129:11 | * ... |
1212
| test_free.cpp:152:27:152:27 | a | test_free.cpp:154:10:154:10 | a |
1313
| test_free.cpp:207:10:207:10 | a | test_free.cpp:209:10:209:10 | a |
14-
| test_free.cpp:252:7:252:7 | p | test_free.cpp:255:10:255:10 | p |
15-
| test_free.cpp:260:9:260:9 | p | test_free.cpp:263:12:263:12 | p |
1614
nodes
1715
| test_free.cpp:11:10:11:10 | a | semmle.label | a |
1816
| test_free.cpp:14:10:14:10 | a | semmle.label | a |
@@ -38,10 +36,6 @@ nodes
3836
| test_free.cpp:154:10:154:10 | a | semmle.label | a |
3937
| test_free.cpp:207:10:207:10 | a | semmle.label | a |
4038
| test_free.cpp:209:10:209:10 | a | semmle.label | a |
41-
| test_free.cpp:252:7:252:7 | p | semmle.label | p |
42-
| test_free.cpp:255:10:255:10 | p | semmle.label | p |
43-
| test_free.cpp:260:9:260:9 | p | semmle.label | p |
44-
| test_free.cpp:263:12:263:12 | p | semmle.label | p |
4539
subpaths
4640
#select
4741
| test_free.cpp:14:10:14:10 | a | test_free.cpp:11:10:11:10 | a | test_free.cpp:14:10:14:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:11:5:11:8 | call to free | call to free |
@@ -56,5 +50,3 @@ subpaths
5650
| test_free.cpp:129:10:129:11 | * ... | test_free.cpp:128:10:128:11 | * ... | test_free.cpp:129:10:129:11 | * ... | Memory pointed to by '* ...' may already have been freed by $@. | test_free.cpp:128:5:128:8 | call to free | call to free |
5751
| test_free.cpp:154:10:154:10 | a | test_free.cpp:152:27:152:27 | a | test_free.cpp:154:10:154:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:152:22:152:25 | call to free | call to free |
5852
| test_free.cpp:209:10:209:10 | a | test_free.cpp:207:10:207:10 | a | test_free.cpp:209:10:209:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:207:5:207:8 | call to free | call to free |
59-
| test_free.cpp:255:10:255:10 | p | test_free.cpp:252:7:252:7 | p | test_free.cpp:255:10:255:10 | p | Memory pointed to by 'p' may already have been freed by $@. | test_free.cpp:252:2:252:5 | call to free | call to free |
60-
| test_free.cpp:263:12:263:12 | p | test_free.cpp:260:9:260:9 | p | test_free.cpp:263:12:263:12 | p | Memory pointed to by 'p' may already have been freed by $@. | test_free.cpp:260:2:260:9 | delete | delete |

‎cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ edges
1212
| test_free.cpp:233:14:233:15 | * ... | test_free.cpp:236:9:236:10 | * ... |
1313
| test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... |
1414
| test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... |
15-
| test_free.cpp:252:7:252:7 | p | test_free.cpp:254:6:254:6 | p |
16-
| test_free.cpp:260:9:260:9 | p | test_free.cpp:262:6:262:6 | p |
1715
nodes
1816
| test_free.cpp:11:10:11:10 | a | semmle.label | a |
1917
| test_free.cpp:12:5:12:5 | a | semmle.label | a |
@@ -40,10 +38,6 @@ nodes
4038
| test_free.cpp:241:9:241:10 | * ... | semmle.label | * ... |
4139
| test_free.cpp:245:10:245:11 | * ... | semmle.label | * ... |
4240
| test_free.cpp:246:9:246:10 | * ... | semmle.label | * ... |
43-
| test_free.cpp:252:7:252:7 | p | semmle.label | p |
44-
| test_free.cpp:254:6:254:6 | p | semmle.label | p |
45-
| test_free.cpp:260:9:260:9 | p | semmle.label | p |
46-
| test_free.cpp:262:6:262:6 | p | semmle.label | p |
4741
subpaths
4842
#select
4943
| test_free.cpp:12:5:12:5 | a | test_free.cpp:11:10:11:10 | a | test_free.cpp:12:5:12:5 | a | Memory may have been previously freed by $@. | test_free.cpp:11:5:11:8 | call to free | call to free |
@@ -59,5 +53,3 @@ subpaths
5953
| test_free.cpp:236:9:236:10 | * ... | test_free.cpp:233:14:233:15 | * ... | test_free.cpp:236:9:236:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:233:9:233:12 | call to free | call to free |
6054
| test_free.cpp:241:9:241:10 | * ... | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free |
6155
| test_free.cpp:246:9:246:10 | * ... | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:245:5:245:8 | call to free | call to free |
62-
| test_free.cpp:254:6:254:6 | p | test_free.cpp:252:7:252:7 | p | test_free.cpp:254:6:254:6 | p | Memory may have been previously freed by $@. | test_free.cpp:252:2:252:5 | call to free | call to free |
63-
| test_free.cpp:262:6:262:6 | p | test_free.cpp:260:9:260:9 | p | test_free.cpp:262:6:262:6 | p | Memory may have been previously freed by $@. | test_free.cpp:260:2:260:9 | delete | delete |

‎cpp/ql/test/query-tests/Critical/MemoryFreed/test_free.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -251,14 +251,14 @@ void test_deref(char **a) {
251251
void test_ref(char *&p) {
252252
free(p);
253253
p = (char *)malloc(sizeof(char)*10);
254-
use(p); // GOOD [FALSE POSITIVE]
255-
free(p); // GOOD [FALSE POSITIVE]
254+
use(p); // GOOD
255+
free(p); // GOOD
256256
}
257257

258258

259259
void test_ref_delete(int *&p) {
260260
delete p;
261261
p = new int;
262-
use(p); // GOOD [FALSE POSITIVE]
263-
delete p; // GOOD [FALSE POSITIVE]
262+
use(p); // GOOD
263+
delete p; // GOOD
264264
}

0 commit comments

Comments
 (0)