Skip to content

Commit 3e8b28a

Browse files
geoffw0geoffw0
authored
Merge pull request #2213 from jbj/BarrierGuard
C++: Implement DataFlow::BarrierGuard for AST+IR
2 parents cc7c30d + ff62afb commit 3e8b28a

File tree

10 files changed

+147
-9
lines changed

10 files changed

+147
-9
lines changed

‎change-notes/1.23/analysis-cpp.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
3939
definition of `x` when `x` is a variable of pointer type. It no longer
4040
considers deep paths such as `f(&x.myField)` to be definitions of `x`. These
4141
changes are in line with the user expectations we've observed.
42+
* The data-flow library now makes it easier to specify barriers/sanitizers
43+
arising from guards by overriding the predicate
44+
`isBarrierGuard`/`isSanitizerGuard` on data-flow and taint-tracking
45+
configurations respectively.
4246
* There is now a `DataFlow::localExprFlow` predicate and a
4347
`TaintTracking::localExprTaint` predicate to make it easy to use the most
4448
common case of local data flow and taint: from one `Expr` to another.

‎cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
private import cpp
66
private import semmle.code.cpp.dataflow.internal.FlowVar
77
private import semmle.code.cpp.models.interfaces.DataFlow
8+
private import semmle.code.cpp.controlflow.Guards
9+
private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
810

911
cached
1012
private newtype TNode =
@@ -680,12 +682,16 @@ VariableAccess getAnAccessToAssignedVariable(Expr assign) {
680682
*
681683
* It is important that all extending classes in scope are disjoint.
682684
*/
683-
class BarrierGuard extends Expr {
684-
/** NOT YET SUPPORTED. Holds if this guard validates `e` upon evaluating to `branch`. */
685-
abstract deprecated predicate checks(Expr e, boolean branch);
685+
class BarrierGuard extends GuardCondition {
686+
/** Override this predicate to hold if this guard validates `e` upon evaluating to `b`. */
687+
abstract predicate checks(Expr e, boolean b);
686688

687689
/** Gets a node guarded by this guard. */
688-
final Node getAGuardedNode() {
689-
none() // stub
690+
final ExprNode getAGuardedNode() {
691+
exists(GVN value, boolean branch |
692+
result.getExpr() = value.getAnExpr() and
693+
this.checks(value.getAnExpr(), branch) and
694+
this.controls(result.getExpr().getBasicBlock(), branch)
695+
)
690696
}
691697
}

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
private import cpp
66
private import semmle.code.cpp.ir.IR
77
private import semmle.code.cpp.controlflow.IRGuards
8+
private import semmle.code.cpp.ir.ValueNumbering
89

910
/**
1011
* A newtype wrapper to prevent accidental casts between `Node` and
@@ -220,7 +221,7 @@ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) }
220221
predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) }
221222

222223
/**
223-
* A guard that validates some expression.
224+
* A guard that validates some instruction.
224225
*
225226
* To use this in a configuration, extend the class and provide a
226227
* characteristic predicate precisely specifying the guard, and override
@@ -229,11 +230,15 @@ predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)
229230
* It is important that all extending classes in scope are disjoint.
230231
*/
231232
class BarrierGuard extends IRGuardCondition {
232-
/** NOT YET SUPPORTED. Holds if this guard validates `e` upon evaluating to `b`. */
233-
abstract deprecated predicate checks(Instruction e, boolean b);
233+
/** Override this predicate to hold if this guard validates `instr` upon evaluating to `b`. */
234+
abstract predicate checks(Instruction instr, boolean b);
234235

235236
/** Gets a node guarded by this guard. */
236237
final Node getAGuardedNode() {
237-
none() // stub
238+
exists(ValueNumber value, boolean edge |
239+
result.asInstruction() = value.getAnInstruction() and
240+
this.checks(value.getAnInstruction(), edge) and
241+
this.controls(result.asInstruction().getBlock(), edge)
242+
)
238243
}
239244
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
int source();
2+
void sink(int);
3+
bool guarded(int);
4+
5+
void bg_basic(int source) {
6+
if (guarded(source)) {
7+
sink(source); // no flow
8+
} else {
9+
sink(source); // flow
10+
}
11+
}
12+
13+
void bg_not(int source) {
14+
if (!guarded(source)) {
15+
sink(source); // flow
16+
} else {
17+
sink(source); // no flow
18+
}
19+
}
20+
21+
void bg_and(int source, bool arbitrary) {
22+
if (guarded(source) && arbitrary) {
23+
sink(source); // no flow
24+
} else {
25+
sink(source); // flow
26+
}
27+
}
28+
29+
void bg_or(int source, bool arbitrary) {
30+
if (guarded(source) || arbitrary) {
31+
sink(source); // flow
32+
} else {
33+
sink(source); // flow
34+
}
35+
}
36+
37+
void bg_return(int source) {
38+
if (!guarded(source)) {
39+
return;
40+
}
41+
sink(source); // no flow
42+
}
43+
44+
struct XY {
45+
int x, y;
46+
};
47+
48+
void bg_stackstruct(XY s1, XY s2) {
49+
s1.x = source();
50+
if (guarded(s1.x)) {
51+
sink(s1.x); // no flow
52+
} else if (guarded(s1.y)) {
53+
sink(s1.x); // flow
54+
} else if (guarded(s2.y)) {
55+
sink(s1.x); // flow
56+
}
57+
}
58+
59+
void bg_structptr(XY *p1, XY *p2) {
60+
p1->x = source();
61+
if (guarded(p1->x)) {
62+
sink(p1->x); // no flow [FALSE POSITIVE in AST]
63+
} else if (guarded(p1->y)) {
64+
sink(p1->x); // flow [NOT DETECTED in IR]
65+
} else if (guarded(p2->x)) {
66+
sink(p1->x); // flow [NOT DETECTED in IR]
67+
}
68+
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/DataflowTestCommon.qll‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,20 @@
11
import cpp
22
import semmle.code.cpp.dataflow.DataFlow
33

4+
/**
5+
* A `BarrierGuard` that stops flow to all occurrences of `x` within statement
6+
* S in `if (guarded(x)) S`.
7+
*/
8+
// This is tested in `BarrierGuard.cpp`.
9+
class TestBarrierGuard extends DataFlow::BarrierGuard {
10+
TestBarrierGuard() { this.(FunctionCall).getTarget().getName() = "guarded" }
11+
12+
override predicate checks(Expr checked, boolean isTrue) {
13+
checked = this.(FunctionCall).getArgument(0) and
14+
isTrue = true
15+
}
16+
}
17+
418
/** Common data flow configuration to be used by tests. */
519
class TestAllocationConfig extends DataFlow::Configuration {
620
TestAllocationConfig() { this = "TestAllocationConfig" }
@@ -26,4 +40,6 @@ class TestAllocationConfig extends DataFlow::Configuration {
2640
override predicate isBarrier(DataFlow::Node barrier) {
2741
barrier.asExpr().(VariableAccess).getTarget().hasName("barrier")
2842
}
43+
44+
override predicate isBarrierGuard(DataFlow::BarrierGuard bg) { bg instanceof TestBarrierGuard }
2945
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/IRDataflowTestCommon.qll‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,20 @@
11
import cpp
22
import semmle.code.cpp.ir.dataflow.DataFlow
3+
import semmle.code.cpp.ir.IR
4+
5+
/**
6+
* A `BarrierGuard` that stops flow to all occurrences of `x` within statement
7+
* S in `if (guarded(x)) S`.
8+
*/
9+
// This is tested in `BarrierGuard.cpp`.
10+
class TestBarrierGuard extends DataFlow::BarrierGuard {
11+
TestBarrierGuard() { this.(CallInstruction).getStaticCallTarget().getName() = "guarded" }
12+
13+
override predicate checks(Instruction checked, boolean isTrue) {
14+
checked = this.(CallInstruction).getPositionalArgument(0) and
15+
isTrue = true
16+
}
17+
}
318

419
/** Common data flow configuration to be used by tests. */
520
class TestAllocationConfig extends DataFlow::Configuration {
@@ -24,4 +39,6 @@ class TestAllocationConfig extends DataFlow::Configuration {
2439
override predicate isBarrier(DataFlow::Node barrier) {
2540
barrier.asExpr().(VariableAccess).getTarget().hasName("barrier")
2641
}
42+
43+
override predicate isBarrierGuard(DataFlow::BarrierGuard bg) { bg instanceof TestBarrierGuard }
2744
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
| BarrierGuard.cpp:9:10:9:15 | source | BarrierGuard.cpp:5:19:5:24 | source |
2+
| BarrierGuard.cpp:15:10:15:15 | source | BarrierGuard.cpp:13:17:13:22 | source |
3+
| BarrierGuard.cpp:25:10:25:15 | source | BarrierGuard.cpp:21:17:21:22 | source |
4+
| BarrierGuard.cpp:31:10:31:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
5+
| BarrierGuard.cpp:33:10:33:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
6+
| BarrierGuard.cpp:53:13:53:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
7+
| BarrierGuard.cpp:55:13:55:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
8+
| BarrierGuard.cpp:62:14:62:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source |
9+
| BarrierGuard.cpp:64:14:64:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source |
10+
| BarrierGuard.cpp:66:14:66:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source |
111
| acrossLinkTargets.cpp:12:8:12:8 | x | acrossLinkTargets.cpp:19:27:19:32 | call to source |
212
| clang.cpp:18:8:18:19 | sourceArray1 | clang.cpp:12:9:12:20 | sourceArray1 |
313
| clang.cpp:22:8:22:20 | & ... | clang.cpp:12:9:12:20 | sourceArray1 |

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
| BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only |
2+
| BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:64:14:64:14 | AST only |
3+
| BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:66:14:66:14 | AST only |
14
| clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only |
25
| clang.cpp:28:27:28:32 | clang.cpp:29:27:29:28 | AST only |
36
| clang.cpp:28:27:28:32 | clang.cpp:30:27:30:34 | AST only |

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
| BarrierGuard.cpp:9:10:9:15 | Load: source | BarrierGuard.cpp:5:19:5:24 | InitializeParameter: source |
2+
| BarrierGuard.cpp:15:10:15:15 | Load: source | BarrierGuard.cpp:13:17:13:22 | InitializeParameter: source |
3+
| BarrierGuard.cpp:25:10:25:15 | Load: source | BarrierGuard.cpp:21:17:21:22 | InitializeParameter: source |
4+
| BarrierGuard.cpp:31:10:31:15 | Load: source | BarrierGuard.cpp:29:16:29:21 | InitializeParameter: source |
5+
| BarrierGuard.cpp:33:10:33:15 | Load: source | BarrierGuard.cpp:29:16:29:21 | InitializeParameter: source |
6+
| BarrierGuard.cpp:53:13:53:13 | Load: x | BarrierGuard.cpp:49:10:49:15 | Call: call to source |
7+
| BarrierGuard.cpp:55:13:55:13 | Load: x | BarrierGuard.cpp:49:10:49:15 | Call: call to source |
18
| acrossLinkTargets.cpp:12:8:12:8 | Convert: (int)... | acrossLinkTargets.cpp:19:27:19:32 | Call: call to source |
29
| acrossLinkTargets.cpp:12:8:12:8 | Load: x | acrossLinkTargets.cpp:19:27:19:32 | Call: call to source |
310
| clang.cpp:18:8:18:19 | Convert: (const int *)... | clang.cpp:12:9:12:20 | InitializeParameter: sourceArray1 |

‎docs/language/learn-ql/cpp/dataflow.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ The following predicates are defined in the configuration:
166166
- ``isSource``—defines where data may flow from
167167
- ``isSink``—defines where data may flow to
168168
- ``isBarrier``—optional, restricts the data flow
169+
- ``isBarrierGuard``—optional, restricts the data flow
169170
- ``isAdditionalFlowStep``—optional, adds additional flow steps
170171

171172
The characteristic predicate ``MyDataFlowConfiguration()`` defines the name of the configuration, so ``"MyDataFlowConfiguration"`` should be replaced by the name of your class.
@@ -204,6 +205,7 @@ The following predicates are defined in the configuration:
204205
- ``isSource``—defines where taint may flow from
205206
- ``isSink``—defines where taint may flow to
206207
- ``isSanitizer``—optional, restricts the taint flow
208+
- ``isSanitizerGuard``—optional, restricts the taint flow
207209
- ``isAdditionalTaintStep``—optional, adds additional taint steps
208210

209211
Similar to global data flow, the characteristic predicate ``MyTaintTrackingConfiguration()`` defines the unique name of the configuration, so ``"MyTaintTrackingConfiguration"`` should be replaced by the name of your class.

0 commit comments

Comments
 (0)