Skip to content

Commit a10f94a

Browse files
MathiasVPMathiasVP
authored
Merge pull request #14810 from MathiasVP/fix-ref-deref-duplication
C++: Fix dataflow duplication from `ReferenceDereference` expressions
2 parents c5d2866 + d25c24b commit a10f94a

File tree

9 files changed

+490
-128
lines changed

9 files changed

+490
-128
lines changed

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

Lines changed: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1273,31 +1273,90 @@ abstract private class IndirectExprNodeBase extends Node {
12731273
}
12741274
}
12751275

1276-
private class IndirectOperandIndirectExprNode extends IndirectExprNodeBase instanceof IndirectOperand
1277-
{
1278-
IndirectOperandIndirectExprNode() {
1276+
/** A signature for converting an indirect node to an expression. */
1277+
private signature module IndirectNodeToIndirectExprSig {
1278+
/** The indirect node class to be converted to an expression */
1279+
class IndirectNode;
1280+
1281+
/**
1282+
* Holds if the indirect expression at indirection index `indirectionIndex`
1283+
* of `node` is `e`. The integer `n` specifies how many conversions has been
1284+
* applied to `node`.
1285+
*/
1286+
predicate indirectNodeHasIndirectExpr(IndirectNode node, Expr e, int n, int indirectionIndex);
1287+
}
1288+
1289+
/**
1290+
* A module that implements the logic for deciding whether an indirect node
1291+
* should be an `IndirectExprNode`.
1292+
*/
1293+
private module IndirectNodeToIndirectExpr<IndirectNodeToIndirectExprSig Sig> {
1294+
import Sig
1295+
1296+
/**
1297+
* This predicate shifts the indirection index by one when `conv` is a
1298+
* `ReferenceDereferenceExpr`.
1299+
*
1300+
* This is necessary because `ReferenceDereferenceExpr` is a conversion
1301+
* in the AST, but appears as a `LoadInstruction` in the IR.
1302+
*/
1303+
bindingset[e, indirectionIndex]
1304+
private predicate adjustForReference(
1305+
Expr e, int indirectionIndex, Expr conv, int adjustedIndirectionIndex
1306+
) {
1307+
conv.(ReferenceDereferenceExpr).getExpr() = e and
1308+
adjustedIndirectionIndex = indirectionIndex - 1
1309+
or
1310+
not conv instanceof ReferenceDereferenceExpr and
1311+
conv = e and
1312+
adjustedIndirectionIndex = indirectionIndex
1313+
}
1314+
1315+
/** Holds if `node` should be an `IndirectExprNode`. */
1316+
predicate charpred(IndirectNode node) {
12791317
exists(Expr e, int n, int indirectionIndex |
1280-
indirectExprNodeShouldBeIndirectOperand(this, e, n, indirectionIndex) and
1281-
not indirectExprNodeShouldBeIndirectOperand(_, e, n + 1, indirectionIndex)
1318+
indirectNodeHasIndirectExpr(node, e, n, indirectionIndex) and
1319+
not exists(Expr conv, int adjustedIndirectionIndex |
1320+
adjustForReference(e, indirectionIndex, conv, adjustedIndirectionIndex) and
1321+
indirectNodeHasIndirectExpr(_, conv, n + 1, adjustedIndirectionIndex)
1322+
)
12821323
)
12831324
}
1325+
}
1326+
1327+
private module IndirectOperandIndirectExprNodeImpl implements IndirectNodeToIndirectExprSig {
1328+
class IndirectNode = IndirectOperand;
1329+
1330+
predicate indirectNodeHasIndirectExpr = indirectExprNodeShouldBeIndirectOperand/4;
1331+
}
1332+
1333+
module IndirectOperandToIndirectExpr =
1334+
IndirectNodeToIndirectExpr<IndirectOperandIndirectExprNodeImpl>;
1335+
1336+
private class IndirectOperandIndirectExprNode extends IndirectExprNodeBase instanceof IndirectOperand
1337+
{
1338+
IndirectOperandIndirectExprNode() { IndirectOperandToIndirectExpr::charpred(this) }
12841339

12851340
final override Expr getConvertedExpr(int n, int index) {
1286-
indirectExprNodeShouldBeIndirectOperand(this, result, n, index)
1341+
IndirectOperandToIndirectExpr::indirectNodeHasIndirectExpr(this, result, n, index)
12871342
}
12881343
}
12891344

1345+
private module IndirectInstructionIndirectExprNodeImpl implements IndirectNodeToIndirectExprSig {
1346+
class IndirectNode = IndirectInstruction;
1347+
1348+
predicate indirectNodeHasIndirectExpr = indirectExprNodeShouldBeIndirectInstruction/4;
1349+
}
1350+
1351+
module IndirectInstructionToIndirectExpr =
1352+
IndirectNodeToIndirectExpr<IndirectInstructionIndirectExprNodeImpl>;
1353+
12901354
private class IndirectInstructionIndirectExprNode extends IndirectExprNodeBase instanceof IndirectInstruction
12911355
{
1292-
IndirectInstructionIndirectExprNode() {
1293-
exists(Expr e, int n, int indirectionIndex |
1294-
indirectExprNodeShouldBeIndirectInstruction(this, e, n, indirectionIndex) and
1295-
not indirectExprNodeShouldBeIndirectInstruction(_, e, n + 1, indirectionIndex)
1296-
)
1297-
}
1356+
IndirectInstructionIndirectExprNode() { IndirectInstructionToIndirectExpr::charpred(this) }
12981357

12991358
final override Expr getConvertedExpr(int n, int index) {
1300-
indirectExprNodeShouldBeIndirectInstruction(this, result, n, index)
1359+
IndirectInstructionToIndirectExpr::indirectNodeHasIndirectExpr(this, result, n, index)
13011360
}
13021361
}
13031362

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

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
module AstTest {
2+
import semmle.code.cpp.dataflow.DataFlow
3+
private import semmle.code.cpp.controlflow.Guards
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+
predicate testBarrierGuard(GuardCondition g, Expr checked, boolean isTrue) {
11+
g.(FunctionCall).getTarget().getName() = "guarded" and
12+
checked = g.(FunctionCall).getArgument(0) and
13+
isTrue = true
14+
}
15+
16+
/** Common data flow configuration to be used by tests. */
17+
module AstTestAllocationConfig implements DataFlow::ConfigSig {
18+
predicate isSource(DataFlow::Node source) {
19+
source.asExpr().(FunctionCall).getTarget().getName() = "source"
20+
or
21+
source.asParameter().getName().matches("source%")
22+
or
23+
source.asExpr().(FunctionCall).getTarget().getName() = "indirect_source"
24+
or
25+
source.(DataFlow::DefinitionByReferenceNode).getParameter().getName().matches("ref_source%")
26+
or
27+
// Track uninitialized variables
28+
exists(source.asUninitialized())
29+
}
30+
31+
predicate isSink(DataFlow::Node sink) {
32+
exists(FunctionCall call |
33+
call.getTarget().getName() = ["sink", "indirect_sink"] and
34+
sink.asExpr() = call.getAnArgument()
35+
)
36+
}
37+
38+
predicate isBarrier(DataFlow::Node barrier) {
39+
barrier.asExpr().(VariableAccess).getTarget().hasName("barrier") or
40+
barrier = DataFlow::BarrierGuard<testBarrierGuard/3>::getABarrierNode()
41+
}
42+
}
43+
44+
module AstFlow = DataFlow::Global<AstTestAllocationConfig>;
45+
}
46+
47+
module IRTest {
48+
private import cpp
49+
import semmle.code.cpp.ir.dataflow.DataFlow
50+
private import semmle.code.cpp.ir.IR
51+
private import semmle.code.cpp.controlflow.IRGuards
52+
53+
/**
54+
* A `BarrierGuard` that stops flow to all occurrences of `x` within statement
55+
* S in `if (guarded(x)) S`.
56+
*/
57+
// This is tested in `BarrierGuard.cpp`.
58+
predicate testBarrierGuard(IRGuardCondition g, Expr checked, boolean isTrue) {
59+
exists(Call call |
60+
call = g.getUnconvertedResultExpression() and
61+
call.getTarget().hasName("guarded") and
62+
checked = call.getArgument(0) and
63+
isTrue = true
64+
)
65+
}
66+
67+
/** Common data flow configuration to be used by tests. */
68+
module IRTestAllocationConfig implements DataFlow::ConfigSig {
69+
predicate isSource(DataFlow::Node source) {
70+
source.asExpr().(FunctionCall).getTarget().getName() = "source"
71+
or
72+
source.asIndirectExpr(1).(FunctionCall).getTarget().getName() = "indirect_source"
73+
or
74+
source.asParameter().getName().matches("source%")
75+
or
76+
source.(DataFlow::DefinitionByReferenceNode).getParameter().getName().matches("ref_source%")
77+
or
78+
exists(source.asUninitialized())
79+
}
80+
81+
predicate isSink(DataFlow::Node sink) {
82+
exists(FunctionCall call, Expr e | e = call.getAnArgument() |
83+
call.getTarget().getName() = "sink" and
84+
sink.asExpr() = e
85+
or
86+
call.getTarget().getName() = "indirect_sink" and
87+
sink.asIndirectExpr() = e
88+
)
89+
}
90+
91+
predicate isBarrier(DataFlow::Node barrier) {
92+
exists(Expr barrierExpr | barrierExpr in [barrier.asExpr(), barrier.asIndirectExpr()] |
93+
barrierExpr.(VariableAccess).getTarget().hasName("barrier")
94+
)
95+
or
96+
barrier = DataFlow::BarrierGuard<testBarrierGuard/3>::getABarrierNode()
97+
or
98+
barrier = DataFlow::BarrierGuard<testBarrierGuard/3>::getAnIndirectBarrierNode()
99+
}
100+
}
101+
102+
module IRFlow = DataFlow::Global<IRTestAllocationConfig>;
103+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ argHasPostUpdate
2424
| lambdas.cpp:45:2:45:2 | e | ArgumentNode is missing PostUpdateNode. |
2525
| test.cpp:67:29:67:35 | source1 | ArgumentNode is missing PostUpdateNode. |
2626
| test.cpp:813:19:813:35 | * ... | ArgumentNode is missing PostUpdateNode. |
27+
| test.cpp:848:23:848:25 | rpx | ArgumentNode is missing PostUpdateNode. |
2728
postWithInFlow
2829
| BarrierGuard.cpp:49:6:49:6 | x [post update] | PostUpdateNode should not be the target of local flow. |
2930
| BarrierGuard.cpp:60:7:60:7 | x [post update] | PostUpdateNode should not be the target of local flow. |

0 commit comments

Comments
 (0)