Skip to content

Commit 6ffeaf8

Browse files
hvitvedhvitved
committed
C#: Adjust flow into phi nodes
1 parent 38b0f74 commit 6ffeaf8

File tree

5 files changed

+70
-41
lines changed

5 files changed

+70
-41
lines changed

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -266,16 +266,15 @@ module LocalFlow {
266266
}
267267

268268
/**
269-
* Holds if `nodeFrom` is a last node referencing SSA definition `def`.
270-
* Either an SSA definition node for `def` when there is no read of `def`,
271-
* or a last read of `def`.
269+
* Holds if `nodeFrom` is a last node referencing SSA definition `def`, which
270+
* can reach `next`.
272271
*/
273-
private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def) {
274-
def = nodeFrom.(SsaDefinitionNode).getDefinition() and
275-
not exists(def.getARead())
276-
or
277-
exists(AssignableRead read, ControlFlow::Node cfn | read = nodeFrom.asExprAtNode(cfn) |
278-
def.getALastReadAtNode(cfn) = read
272+
private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
273+
exists(ControlFlow::BasicBlock bb, int i | SsaImpl::lastRefBeforeRedef(def, bb, i, next) |
274+
def = nodeFrom.(SsaDefinitionNode).getDefinition() and
275+
def.definesAt(_, bb, i)
276+
or
277+
nodeFrom.asExprAtNode(bb.getNode(i)) instanceof AssignableRead
279278
)
280279
}
281280

@@ -302,14 +301,14 @@ module LocalFlow {
302301
or
303302
// Flow into phi node
304303
exists(Ssa::PhiNode phi |
305-
localFlowSsaInput(nodeFrom, def) and
304+
localFlowSsaInput(nodeFrom, def, phi) and
306305
phi = nodeTo.(SsaDefinitionNode).getDefinition() and
307306
def = phi.getAnInput()
308307
)
309308
or
310309
// Flow into uncertain SSA definition
311310
exists(LocalFlow::UncertainExplicitSsaDefinition uncertain |
312-
localFlowSsaInput(nodeFrom, def) and
311+
localFlowSsaInput(nodeFrom, def, uncertain) and
313312
uncertain = nodeTo.(SsaDefinitionNode).getDefinition() and
314313
def = uncertain.getPriorDefinition()
315314
)

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/SsaImpl.qll‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import csharp
66
import SsaImplCommon
77

88
/** A classification of variable reads. */
9-
newtype TReadKind =
9+
private newtype TReadKind =
1010
/** An actual read. */
1111
ActualRead() or
1212
/**
@@ -49,7 +49,7 @@ newtype TReadKind =
4949
RefReadBeforeWrite()
5050

5151
/** A classification of variable reads. */
52-
class ReadKind extends TReadKind {
52+
private class ReadKind extends TReadKind {
5353
string toString() {
5454
this = ActualRead() and
5555
result = "ActualRead"
@@ -199,7 +199,7 @@ private module SourceVariableImpl {
199199
rk = RefReadBeforeWrite()
200200
}
201201

202-
private predicate outRefExitRead(ControlFlow::BasicBlock bb, int i, LocalScopeSourceVariable v) {
202+
predicate outRefExitRead(ControlFlow::BasicBlock bb, int i, LocalScopeSourceVariable v) {
203203
exists(ControlFlow::Nodes::AnnotatedExitNode exit |
204204
exit.isNormal() and
205205
exists(LocalScopeVariable lsv |
@@ -1130,7 +1130,6 @@ private module Cached {
11301130
) {
11311131
exists(Ssa::Definition def0, ControlFlow::BasicBlock bb, int i |
11321132
def = def0.getAnUltimateDefinition() and
1133-
lastRef(def0, bb, i) and
11341133
capturedReadOut(bb, i, def0.getSourceVariable(), cdef.getSourceVariable(), cdef.getCall(),
11351134
additionalCalls)
11361135
)
@@ -1199,12 +1198,28 @@ private module Cached {
11991198
variableRead(bb2, i2, _, any(ReadKind rk | rk.isPseudo()))
12001199
}
12011200

1201+
private predicate reachesLastRefRedef(
1202+
Definition def, ControlFlow::BasicBlock bb, int i, Definition next
1203+
) {
1204+
lastRefRedef(def, bb, i, next)
1205+
or
1206+
exists(ControlFlow::BasicBlock bb0, int i0 |
1207+
reachesLastRefRedef(def, bb0, i0, next) and
1208+
adjacentDefPseudoRead(def, bb, i, bb0, i0)
1209+
)
1210+
}
1211+
1212+
cached
1213+
predicate lastRefBeforeRedef(Definition def, ControlFlow::BasicBlock bb, int i, Definition next) {
1214+
reachesLastRefRedef(def, bb, i, next) and
1215+
not variableRead(bb, i, def.getSourceVariable(), any(ReadKind rk | rk.isPseudo()))
1216+
}
1217+
12021218
private predicate reachesLastRef(Definition def, ControlFlow::BasicBlock bb, int i) {
12031219
lastRef(def, bb, i)
12041220
or
12051221
exists(ControlFlow::BasicBlock bb0, int i0 |
12061222
reachesLastRef(def, bb0, i0) and
1207-
variableRead(bb0, i0, _, any(ReadKind rk | rk.isPseudo())) and
12081223
adjacentDefPseudoRead(def, bb, i, bb0, i0)
12091224
)
12101225
}
@@ -1221,11 +1236,12 @@ private module Cached {
12211236
cached
12221237
predicate isLiveOutRefParameterDefinition(Ssa::Definition def, Parameter p) {
12231238
p.isOutOrRef() and
1224-
exists(Ssa::Definition def0, ControlFlow::BasicBlock bb, int i |
1239+
exists(Ssa::SourceVariable v, Ssa::Definition def0, ControlFlow::BasicBlock bb, int i |
1240+
v = def.getSourceVariable() and
1241+
p = v.getAssignable() and
12251242
def = def0.getAnUltimateDefinition() and
1226-
reachesLastRef(def0, bb, i) and
1227-
variableRead(bb, i, def0.getSourceVariable(), OutRefExitRead()) and
1228-
p = def0.getSourceVariable().getAssignable()
1243+
ssaDefReachesRead(_, def0, bb, i) and
1244+
outRefExitRead(bb, i, v)
12291245
)
12301246
}
12311247
}

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,28 @@ private module Cached {
440440
defAdjacentRead(def, bb1, bb2, i2)
441441
}
442442

443+
/**
444+
* Holds if the node at index `i` in `bb` is a last reference to SSA definition
445+
* `def`. The reference is last because it can reach another write `next`,
446+
* without passing through another read or write.
447+
*/
448+
cached
449+
predicate lastRefRedef(Definition def, BasicBlock bb, int i, Definition next) {
450+
exists(int rnk, SourceVariable v, int j | rnk = ssaDefRank(def, v, bb, i, _) |
451+
// Next reference to `v` inside `bb` is a write
452+
next.definesAt(v, bb, j) and
453+
rnk + 1 = ssaRefRank(bb, j, v, SsaDef())
454+
or
455+
// Can reach a write using one or more steps
456+
rnk = maxSsaRefRank(bb, v) and
457+
exists(BasicBlock bb2 |
458+
varBlockReaches(def, bb, bb2) and
459+
next.definesAt(v, bb2, j) and
460+
1 = ssaRefRank(bb2, j, v, SsaDef())
461+
)
462+
)
463+
}
464+
443465
/**
444466
* Holds if the node at index `i` in `bb` is a last reference to SSA
445467
* definition `def`.
@@ -450,24 +472,16 @@ private module Cached {
450472
*/
451473
cached
452474
predicate lastRef(Definition def, BasicBlock bb, int i) {
453-
exists(int rnk, SourceVariable v | rnk = ssaDefRank(def, v, bb, i, _) |
454-
// Next reference to `v` inside `bb` is a write
455-
rnk + 1 = ssaRefRank(bb, _, v, SsaDef())
475+
lastRefRedef(def, bb, i, _)
476+
or
477+
exists(SourceVariable v | ssaDefRank(def, v, bb, i, _) = maxSsaRefRank(bb, v) |
478+
// Can reach exit directly
479+
bb instanceof ExitBasicBlock
456480
or
457-
// No more references to `v` inside `bb`
458-
rnk = maxSsaRefRank(bb, v) and
459-
(
460-
// Can reach exit directly
461-
bb instanceof ExitBasicBlock
462-
or
463-
exists(BasicBlock bb2 | varBlockReaches(def, bb, bb2) |
464-
// Can reach a write using one or more steps
465-
1 = ssaRefRank(bb2, _, def.getSourceVariable(), SsaDef())
466-
or
467-
// Can reach a block using one or more steps, where `def` is no longer live
468-
not defOccursInBlock(def, bb2, _) and
469-
not ssaDefReachesEndOfBlock(bb2, def, _)
470-
)
481+
// Can reach a block using one or more steps, where `def` is no longer live
482+
exists(BasicBlock bb2 | varBlockReaches(def, bb, bb2) |
483+
not defOccursInBlock(def, bb2, _) and
484+
not ssaDefReachesEndOfBlock(bb2, def, _)
471485
)
472486
)
473487
}

‎csharp/ql/test/library-tests/dataflow/local/DataFlowStep.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,8 @@
417417
| LocalDataFlow.cs:367:23:367:24 | b1 | LocalDataFlow.cs:371:13:371:14 | access to parameter b1 |
418418
| LocalDataFlow.cs:367:32:367:33 | b2 | LocalDataFlow.cs:374:17:374:18 | access to parameter b2 |
419419
| LocalDataFlow.cs:373:13:373:25 | SSA def(x) | LocalDataFlow.cs:376:35:376:35 | access to local variable x |
420+
| LocalDataFlow.cs:373:13:373:25 | SSA def(x) | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
420421
| LocalDataFlow.cs:373:17:373:25 | "tainted" | LocalDataFlow.cs:373:13:373:25 | SSA def(x) |
421-
| LocalDataFlow.cs:376:35:376:35 | access to local variable x | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
422422
| LocalDataFlow.cs:381:13:381:29 | SSA def(x) | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
423423
| LocalDataFlow.cs:381:17:381:29 | "not tainted" | LocalDataFlow.cs:381:13:381:29 | SSA def(x) |
424424
| LocalDataFlow.cs:382:9:382:17 | SSA phi(x) | LocalDataFlow.cs:382:15:382:15 | access to local variable x |
@@ -702,14 +702,14 @@
702702
| SSA.cs:154:13:154:13 | access to parameter t | SSA.cs:154:13:154:13 | (...) ... |
703703
| SSA.cs:154:13:154:13 | access to parameter t | SSA.cs:155:25:155:25 | access to parameter t |
704704
| SSA.cs:155:25:155:25 | SSA def(t) | SSA.cs:152:17:152:28 | SSA phi(t) |
705-
| SSA.cs:155:25:155:25 | access to parameter t | SSA.cs:152:17:152:28 | SSA phi(t) |
706705
| SSA.cs:166:10:166:13 | this | SSA.cs:166:19:166:22 | this access |
707706
| SSA.cs:168:22:168:28 | tainted | SSA.cs:173:24:173:30 | access to parameter tainted |
708707
| SSA.cs:168:35:168:35 | i | SSA.cs:171:13:171:13 | access to parameter i |
709708
| SSA.cs:170:16:170:28 | SSA def(ssaSink5) | SSA.cs:180:9:180:24 | SSA phi(ssaSink5) |
710709
| SSA.cs:170:27:170:28 | "" | SSA.cs:170:16:170:28 | SSA def(ssaSink5) |
711710
| SSA.cs:171:13:171:15 | SSA def(i) | SSA.cs:174:20:174:20 | SSA phi(i) |
712711
| SSA.cs:173:13:173:30 | SSA def(ssaSink5) | SSA.cs:176:21:176:28 | access to local variable ssaSink5 |
712+
| SSA.cs:173:13:173:30 | SSA def(ssaSink5) | SSA.cs:180:9:180:24 | SSA phi(ssaSink5) |
713713
| SSA.cs:173:24:173:30 | access to parameter tainted | SSA.cs:173:13:173:30 | SSA def(ssaSink5) |
714714
| SSA.cs:174:20:174:20 | SSA phi(i) | SSA.cs:174:20:174:20 | access to parameter i |
715715
| SSA.cs:174:20:174:22 | SSA def(i) | SSA.cs:174:20:174:20 | SSA phi(i) |

‎csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -527,8 +527,8 @@
527527
| LocalDataFlow.cs:367:23:367:24 | b1 | LocalDataFlow.cs:371:13:371:14 | access to parameter b1 |
528528
| LocalDataFlow.cs:367:32:367:33 | b2 | LocalDataFlow.cs:374:17:374:18 | access to parameter b2 |
529529
| LocalDataFlow.cs:373:13:373:25 | SSA def(x) | LocalDataFlow.cs:376:35:376:35 | access to local variable x |
530+
| LocalDataFlow.cs:373:13:373:25 | SSA def(x) | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
530531
| LocalDataFlow.cs:373:17:373:25 | "tainted" | LocalDataFlow.cs:373:13:373:25 | SSA def(x) |
531-
| LocalDataFlow.cs:376:35:376:35 | access to local variable x | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
532532
| LocalDataFlow.cs:381:13:381:29 | SSA def(x) | LocalDataFlow.cs:382:9:382:17 | SSA phi(x) |
533533
| LocalDataFlow.cs:381:17:381:29 | "not tainted" | LocalDataFlow.cs:381:13:381:29 | SSA def(x) |
534534
| LocalDataFlow.cs:382:9:382:17 | SSA phi(x) | LocalDataFlow.cs:382:15:382:15 | access to local variable x |
@@ -828,7 +828,6 @@
828828
| SSA.cs:154:13:154:13 | access to parameter t | SSA.cs:154:13:154:13 | (...) ... |
829829
| SSA.cs:154:13:154:13 | access to parameter t | SSA.cs:155:25:155:25 | access to parameter t |
830830
| SSA.cs:155:25:155:25 | SSA def(t) | SSA.cs:152:17:152:28 | SSA phi(t) |
831-
| SSA.cs:155:25:155:25 | access to parameter t | SSA.cs:152:17:152:28 | SSA phi(t) |
832831
| SSA.cs:166:10:166:13 | this | SSA.cs:166:19:166:22 | this access |
833832
| SSA.cs:168:22:168:28 | tainted | SSA.cs:173:24:173:30 | access to parameter tainted |
834833
| SSA.cs:168:35:168:35 | i | SSA.cs:171:13:171:13 | access to parameter i |
@@ -837,6 +836,7 @@
837836
| SSA.cs:171:13:171:15 | ...-- | SSA.cs:171:13:171:19 | ... > ... |
838837
| SSA.cs:171:13:171:15 | SSA def(i) | SSA.cs:174:20:174:20 | SSA phi(i) |
839838
| SSA.cs:173:13:173:30 | SSA def(ssaSink5) | SSA.cs:176:21:176:28 | access to local variable ssaSink5 |
839+
| SSA.cs:173:13:173:30 | SSA def(ssaSink5) | SSA.cs:180:9:180:24 | SSA phi(ssaSink5) |
840840
| SSA.cs:173:24:173:30 | access to parameter tainted | SSA.cs:173:13:173:30 | SSA def(ssaSink5) |
841841
| SSA.cs:174:20:174:20 | SSA phi(i) | SSA.cs:174:20:174:20 | access to parameter i |
842842
| SSA.cs:174:20:174:22 | ...-- | SSA.cs:174:20:174:26 | ... > ... |

0 commit comments

Comments
 (0)