Skip to content

Commit 39dafd6

Browse files
MathiasVPMathiasVP
authored
C++: Suggestions to github#15343 (#39)
* C++: Change the interface of 'FlowAfterFree' so that the module it takes a single module as a parameter. * C++: Add another predicate to the module signature. * C++: Convert the use-after-free and double-free libraries to use new interface. * C++: Accept test changes.
1 parent 9a0e2e5 commit 39dafd6

File tree

5 files changed

+69
-74
lines changed

5 files changed

+69
-74
lines changed

‎cpp/ql/src/Critical/DoubleFree.ql

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
import cpp
1515
import semmle.code.cpp.dataflow.new.DataFlow
16-
import semmle.code.cpp.ir.IR
1716
import FlowAfterFree
1817
import DoubleFree::PathGraph
1918

@@ -42,28 +41,21 @@ predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
4241
)
4342
}
4443

45-
module DoubleFree = FlowFromFree<isFree/2, isExcludeFreePair/2>;
44+
module DoubleFreeParam implements FlowFromFreeParamSig {
45+
predicate isSink = isFree/2;
4646

47-
/*
48-
* In order to reduce false positives, the set of sinks is restricted to only those
49-
* that satisfy at least one of the following two criteria:
50-
* 1. The source dominates the sink, or
51-
* 2. The sink post-dominates the source.
52-
*/
47+
predicate isExcluded = isExcludeFreePair/2;
48+
49+
predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2;
50+
}
5351

54-
from
55-
DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2,
56-
DataFlow::Node srcNode, DataFlow::Node sinkNode
52+
module DoubleFree = FlowFromFree<DoubleFreeParam>;
53+
54+
from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2
5755
where
5856
DoubleFree::flowPath(source, sink) and
59-
source.getNode() = srcNode and
60-
sink.getNode() = sinkNode and
61-
isFree(srcNode, _, _, dealloc) and
62-
isFree(sinkNode, e2) and
63-
(
64-
sinkStrictlyPostDominatesSource(srcNode, sinkNode) or
65-
sourceStrictlyDominatesSink(srcNode, sinkNode)
66-
)
67-
select sinkNode, source, sink,
57+
isFree(source.getNode(), _, _, dealloc) and
58+
isFree(sink.getNode(), e2)
59+
select sink.getNode(), source, sink,
6860
"Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc,
6961
dealloc.toString()

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

Lines changed: 46 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,6 @@ import cpp
22
import semmle.code.cpp.dataflow.new.DataFlow
33
private import semmle.code.cpp.ir.IR
44

5-
/**
6-
* Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in
7-
* the `FlowFromFreeConfig` module.
8-
*/
9-
private signature predicate isSinkSig(DataFlow::Node n, Expr e);
10-
11-
/**
12-
* Holds if `dealloc` is a deallocation expression and `e` is an expression such
13-
* that `isFree(_, e)` holds for some `isFree` predicate satisfying `isSinkSig`,
14-
* and this source-sink pair should be excluded from the analysis.
15-
*/
16-
bindingset[dealloc, e]
17-
private signature predicate isExcludedSig(DeallocationExpr dealloc, Expr e);
18-
195
/**
206
* Holds if `(b1, i1)` strictly post-dominates `(b2, i2)`
217
*/
@@ -38,27 +24,38 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
3824
b1.strictlyDominates(b2)
3925
}
4026

41-
predicate sinkStrictlyPostDominatesSource(DataFlow::Node source, DataFlow::Node sink) {
42-
exists(IRBlock b1, int i1, IRBlock b2, int i2 |
43-
source.hasIndexInBlock(b1, i1) and
44-
sink.hasIndexInBlock(b2, i2) and
45-
strictlyPostDominates(b2, i2, b1, i1)
46-
)
47-
}
27+
signature module FlowFromFreeParamSig {
28+
/**
29+
* Signature for a predicate that holds if `n.asExpr() = e` and `n` is a sink in
30+
* the `FlowFromFreeConfig` module.
31+
*/
32+
predicate isSink(DataFlow::Node n, Expr e);
4833

49-
predicate sourceStrictlyDominatesSink(DataFlow::Node source, DataFlow::Node sink) {
50-
exists(IRBlock b1, int i1, IRBlock b2, int i2 |
51-
source.hasIndexInBlock(b1, i1) and
52-
sink.hasIndexInBlock(b2, i2) and
53-
strictlyDominates(b1, i1, b2, i2)
54-
)
34+
/**
35+
* Holds if `dealloc` is a deallocation expression and `e` is an expression such
36+
* that `isFree(_, e)` holds for some `isFree` predicate satisfying `isSinkSig`,
37+
* and this source-sink pair should be excluded from the analysis.
38+
*/
39+
bindingset[dealloc, e]
40+
predicate isExcluded(DeallocationExpr dealloc, Expr e);
41+
42+
/**
43+
* Holds if `sink` should be considered a `sink` when the source of flow is `source`.
44+
*/
45+
bindingset[source, sink]
46+
default predicate sourceSinkIsRelated(DataFlow::Node source, DataFlow::Node sink) { any() }
5547
}
5648

5749
/**
5850
* Constructs a `FlowFromFreeConfig` module that can be used to find flow between
5951
* a pointer being freed by some deallocation function, and a user-specified sink.
52+
*
53+
* In order to reduce false positives, the set of sinks is restricted to only those
54+
* that satisfy at least one of the following two criteria:
55+
* 1. The source dominates the sink, or
56+
* 2. The sink post-dominates the source.
6057
*/
61-
module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
58+
module FlowFromFree<FlowFromFreeParamSig P> {
6259
module FlowFromFreeConfig implements DataFlow::StateConfigSig {
6360
class FlowState instanceof Expr {
6461
FlowState() { isFree(_, _, this, _) }
@@ -70,11 +67,12 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
7067

7168
pragma[inline]
7269
predicate isSink(DataFlow::Node sink, FlowState state) {
73-
exists(Expr e, DeallocationExpr dealloc |
74-
isASink(sink, e) and
75-
isFree(_, _, state, dealloc) and
70+
exists(Expr e, DataFlow::Node source, DeallocationExpr dealloc |
71+
P::isSink(sink, e) and
72+
isFree(source, _, state, dealloc) and
7673
e != state and
77-
not isExcluded(dealloc, e)
74+
not P::isExcluded(dealloc, e) and
75+
P::sourceSinkIsRelated(source, sink)
7876
)
7977
}
8078

@@ -129,3 +127,19 @@ predicate isExFreePoolCall(FunctionCall fc, Expr e) {
129127
fc.getTarget().hasGlobalName("ExFreePool")
130128
)
131129
}
130+
131+
/**
132+
* Holds if either `source` strictly dominates `sink`, or `sink` strictly
133+
* post-dominates `source`.
134+
*/
135+
bindingset[source, sink]
136+
predicate defaultSourceSinkIsRelated(DataFlow::Node source, DataFlow::Node sink) {
137+
exists(IRBlock b1, int i1, IRBlock b2, int i2 |
138+
source.hasIndexInBlock(b1, i1) and
139+
sink.hasIndexInBlock(b2, i2)
140+
|
141+
strictlyDominates(b1, i1, b2, i2)
142+
or
143+
strictlyPostDominates(b2, i2, b1, i1)
144+
)
145+
}

‎cpp/ql/src/Critical/UseAfterFree.ql

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -173,26 +173,19 @@ predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) {
173173
isExFreePoolCall(_, e)
174174
}
175175

176-
module UseAfterFree = FlowFromFree<isUse/2, isExcludeFreeUsePair/2>;
176+
module UseAfterFreeParam implements FlowFromFreeParamSig {
177+
predicate isSink = isUse/2;
177178

178-
/*
179-
* In order to reduce false positives, the set of sinks is restricted to only those
180-
* that satisfy at least one of the following two criteria:
181-
* 1. The source dominates the sink, or
182-
* 2. The sink post-dominates the source.
183-
*/
179+
predicate isExcluded = isExcludeFreeUsePair/2;
180+
181+
predicate sourceSinkIsRelated = defaultSourceSinkIsRelated/2;
182+
}
184183

185-
from
186-
UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc,
187-
DataFlow::Node srcNode, DataFlow::Node sinkNode
184+
module UseAfterFree = FlowFromFree<UseAfterFreeParam>;
185+
186+
from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc
188187
where
189188
UseAfterFree::flowPath(source, sink) and
190-
source.getNode() = srcNode and
191-
sink.getNode() = sinkNode and
192-
isFree(srcNode, _, _, dealloc) and
193-
(
194-
sinkStrictlyPostDominatesSource(srcNode, sinkNode) or
195-
sourceStrictlyDominatesSink(srcNode, sinkNode)
196-
)
197-
select sinkNode, source, sink, "Memory may have been previously freed by $@.", dealloc,
189+
isFree(source.getNode(), _, _, dealloc)
190+
select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc,
198191
dealloc.toString()

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ edges
22
| test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:14:10:14:10 | a |
33
| test_free.cpp:30:10:30:10 | pointer to free output argument | test_free.cpp:31:27:31:27 | a |
44
| test_free.cpp:35:10:35:10 | pointer to free output argument | test_free.cpp:37:27:37:27 | a |
5-
| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:44:27:44:27 | a |
65
| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:46:10:46:10 | a |
76
| test_free.cpp:44:27:44:27 | pointer to free output argument | test_free.cpp:46:10:46:10 | a |
87
| test_free.cpp:50:27:50:27 | pointer to free output argument | test_free.cpp:51:10:51:10 | a |
@@ -20,7 +19,6 @@ nodes
2019
| test_free.cpp:35:10:35:10 | pointer to free output argument | semmle.label | pointer to free output argument |
2120
| test_free.cpp:37:27:37:27 | a | semmle.label | a |
2221
| test_free.cpp:42:27:42:27 | pointer to free output argument | semmle.label | pointer to free output argument |
23-
| test_free.cpp:44:27:44:27 | a | semmle.label | a |
2422
| test_free.cpp:44:27:44:27 | pointer to free output argument | semmle.label | pointer to free output argument |
2523
| test_free.cpp:46:10:46:10 | a | semmle.label | a |
2624
| test_free.cpp:46:10:46:10 | a | semmle.label | a |

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
edges
22
| test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:12:5:12:5 | a |
33
| test_free.cpp:11:10:11:10 | pointer to free output argument | test_free.cpp:13:5:13:6 | * ... |
4-
| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:43:22:43:22 | a |
54
| test_free.cpp:42:27:42:27 | pointer to free output argument | test_free.cpp:45:5:45:5 | a |
65
| test_free.cpp:44:27:44:27 | pointer to free output argument | test_free.cpp:45:5:45:5 | a |
76
| test_free.cpp:69:10:69:10 | pointer to free output argument | test_free.cpp:71:9:71:9 | a |
@@ -28,7 +27,6 @@ nodes
2827
| test_free.cpp:12:5:12:5 | a | semmle.label | a |
2928
| test_free.cpp:13:5:13:6 | * ... | semmle.label | * ... |
3029
| test_free.cpp:42:27:42:27 | pointer to free output argument | semmle.label | pointer to free output argument |
31-
| test_free.cpp:43:22:43:22 | a | semmle.label | a |
3230
| test_free.cpp:44:27:44:27 | pointer to free output argument | semmle.label | pointer to free output argument |
3331
| test_free.cpp:45:5:45:5 | a | semmle.label | a |
3432
| test_free.cpp:45:5:45:5 | a | semmle.label | a |

0 commit comments

Comments
 (0)