Skip to content

Commit 5aa1242

Browse files
committed
Shared: use a call bit when tracking reachability to/from a discriminator
1 parent 0eb543e commit 5aa1242

File tree

2 files changed

+100
-30
lines changed

2 files changed

+100
-30
lines changed

java/ql/test/library-tests/dataflow/deduplicate-path-graph/test.expected

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,27 @@ nodes
77
| A.java:15:29:15:29 | x : String | semmle.label | x : String |
88
| A.java:17:20:17:27 | source(...) : String | semmle.label | source(...) : String |
99
| A.java:18:20:18:38 | apply(...) : String | semmle.label | apply(...) : String |
10+
| A.java:18:20:18:38 | apply(...) : String | semmle.label | apply(...) : String |
1011
| A.java:18:33:18:37 | step0 : String | semmle.label | step0 : String |
1112
| A.java:19:20:19:38 | apply(...) : String | semmle.label | apply(...) : String |
1213
| A.java:19:33:19:37 | step1 : String | semmle.label | step1 : String |
14+
| A.java:19:33:19:37 | step1 : String | semmle.label | step1 : String |
1315
| A.java:21:10:21:14 | step2 | semmle.label | step2 |
1416
| A.java:26:8:26:8 | x : String | semmle.label | x : String |
17+
| A.java:26:8:26:8 | x : String | semmle.label | x : String |
18+
| A.java:26:13:26:81 | ...?...:... : String | semmle.label | ...?...:... : String |
1519
| A.java:26:13:26:81 | ...?...:... : String | semmle.label | ...?...:... : String |
1620
| A.java:26:35:26:56 | propagateState(...) : String | semmle.label | propagateState(...) : String |
1721
| A.java:26:50:26:50 | x : String | semmle.label | x : String |
1822
| A.java:26:60:26:81 | propagateState(...) : String | semmle.label | propagateState(...) : String |
1923
| A.java:26:75:26:75 | x : String | semmle.label | x : String |
2024
| A.java:28:20:28:27 | source(...) : String | semmle.label | source(...) : String |
2125
| A.java:29:20:29:38 | apply(...) : String | semmle.label | apply(...) : String |
26+
| A.java:29:20:29:38 | apply(...) : String | semmle.label | apply(...) : String |
2227
| A.java:29:33:29:37 | step0 : String | semmle.label | step0 : String |
2328
| A.java:30:20:30:38 | apply(...) : String | semmle.label | apply(...) : String |
2429
| A.java:30:33:30:37 | step1 : String | semmle.label | step1 : String |
30+
| A.java:30:33:30:37 | step1 : String | semmle.label | step1 : String |
2531
| A.java:32:10:32:14 | step2 | semmle.label | step2 |
2632
edges
2733
| A.java:14:9:14:9 | x : String | A.java:14:29:14:29 | x : String |
@@ -30,13 +36,16 @@ edges
3036
| A.java:15:29:15:29 | x : String | A.java:15:14:15:35 | propagateState(...) : String |
3137
| A.java:17:20:17:27 | source(...) : String | A.java:18:33:18:37 | step0 : String |
3238
| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String |
39+
| A.java:18:20:18:38 | apply(...) : String | A.java:19:33:19:37 | step1 : String |
3340
| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String |
3441
| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String |
3542
| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String |
43+
| A.java:18:33:18:37 | step0 : String | A.java:18:20:18:38 | apply(...) : String |
3644
| A.java:19:20:19:38 | apply(...) : String | A.java:21:10:21:14 | step2 |
3745
| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String |
3846
| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String |
3947
| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String |
48+
| A.java:19:33:19:37 | step1 : String | A.java:19:20:19:38 | apply(...) : String |
4049
| A.java:26:8:26:8 | x : String | A.java:26:50:26:50 | x : String |
4150
| A.java:26:8:26:8 | x : String | A.java:26:75:26:75 | x : String |
4251
| A.java:26:35:26:56 | propagateState(...) : String | A.java:26:13:26:81 | ...?...:... : String |
@@ -45,23 +54,26 @@ edges
4554
| A.java:26:75:26:75 | x : String | A.java:26:60:26:81 | propagateState(...) : String |
4655
| A.java:28:20:28:27 | source(...) : String | A.java:29:33:29:37 | step0 : String |
4756
| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String |
57+
| A.java:29:20:29:38 | apply(...) : String | A.java:30:33:30:37 | step1 : String |
4858
| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String |
59+
| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String |
60+
| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String |
4961
| A.java:29:33:29:37 | step0 : String | A.java:29:20:29:38 | apply(...) : String |
5062
| A.java:30:20:30:38 | apply(...) : String | A.java:32:10:32:14 | step2 |
5163
| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String |
64+
| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String |
65+
| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String |
5266
| A.java:30:33:30:37 | step1 : String | A.java:30:20:30:38 | apply(...) : String |
5367
subpaths
5468
| A.java:18:33:18:37 | step0 : String | A.java:14:9:14:9 | x : String | A.java:14:14:14:35 | propagateState(...) : String | A.java:18:20:18:38 | apply(...) : String |
5569
| A.java:18:33:18:37 | step0 : String | A.java:15:9:15:9 | x : String | A.java:15:14:15:35 | propagateState(...) : String | A.java:18:20:18:38 | apply(...) : String |
5670
| A.java:19:33:19:37 | step1 : String | A.java:14:9:14:9 | x : String | A.java:14:14:14:35 | propagateState(...) : String | A.java:19:20:19:38 | apply(...) : String |
5771
| A.java:19:33:19:37 | step1 : String | A.java:15:9:15:9 | x : String | A.java:15:14:15:35 | propagateState(...) : String | A.java:19:20:19:38 | apply(...) : String |
5872
| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:29:20:29:38 | apply(...) : String |
73+
| A.java:29:33:29:37 | step0 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:29:20:29:38 | apply(...) : String |
74+
| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:30:20:30:38 | apply(...) : String |
5975
| A.java:30:33:30:37 | step1 : String | A.java:26:8:26:8 | x : String | A.java:26:13:26:81 | ...?...:... : String | A.java:30:20:30:38 | apply(...) : String |
6076
spuriousFlow
61-
| A.java:14:14:14:35 | propagateState(...) : String | B | A |
62-
| A.java:15:14:15:35 | propagateState(...) : String | A | B |
63-
| A.java:26:35:26:56 | propagateState(...) : String | B | A |
64-
| A.java:26:60:26:81 | propagateState(...) : String | A | B |
6577
#select
6678
| A.java:17:20:17:27 | source(...) : String | A.java:17:20:17:27 | source(...) : String | A.java:21:10:21:14 | step2 | Flow |
6779
| A.java:28:20:28:27 | source(...) : String | A.java:28:20:28:27 | source(...) : String | A.java:32:10:32:14 | step2 | Flow |

shared/dataflow/codeql/dataflow/DataFlow.qll

Lines changed: 84 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -861,22 +861,6 @@ module DataFlowMake<LocationSig Location, InputSig<Location> Lang> {
861861
// non-sink PathNode with the same `(node, toString)` value and the same successors, but is transitively
862862
// reachable from a different set of PathNodes. (And conversely for sources).
863863
//
864-
/**
865-
* Gets a successor of `node`, taking `subpaths` into account.
866-
*/
867-
private InputPathNode getASuccessorLike(InputPathNode node) {
868-
Graph::edges(node, result)
869-
or
870-
Graph::subpaths(node, _, _, result) // arg -> out
871-
//
872-
// Note that there is no case for `arg -> param` or `ret -> out` for subpaths.
873-
// It is OK to collapse nodes inside a subpath while calls to that subpaths aren't collapsed and vice versa.
874-
}
875-
876-
private InputPathNode getAPredecessorLike(InputPathNode node) {
877-
node = getASuccessorLike(result)
878-
}
879-
880864
pragma[nomagic]
881865
private InputPathNode getAPathNode(Node node, string toString) {
882866
result.getNode() = node and
@@ -885,7 +869,11 @@ module DataFlowMake<LocationSig Location, InputSig<Location> Lang> {
885869

886870
private signature predicate collapseCandidateSig(Node node, string toString);
887871

888-
private signature InputPathNode stepSig(InputPathNode node);
872+
private signature predicate stepSig(InputPathNode node1, InputPathNode node2);
873+
874+
private signature predicate subpathStepSig(
875+
InputPathNode arg, InputPathNode param, InputPathNode ret, InputPathNode out
876+
);
889877

890878
/**
891879
* Performs a forward or backward pass computing which `(node, toString)` pairs can subsume their corresponding
@@ -898,12 +886,14 @@ module DataFlowMake<LocationSig Location, InputSig<Location> Lang> {
898886
* Comments are written as if this checks for outgoing edges and propagates backward, though the module is also
899887
* used to perform the opposite direction.
900888
*/
901-
private module MakeDiscriminatorPass<collapseCandidateSig/2 collapseCandidate, stepSig/1 step> {
889+
private module MakeDiscriminatorPass<
890+
collapseCandidateSig/2 collapseCandidate, stepSig/2 step, subpathStepSig/4 subpathStep>
891+
{
902892
/**
903893
* Gets the number of `(node, toString)` pairs reachable in one step from `pathNode`.
904894
*/
905895
private int getOutDegreeFromPathNode(InputPathNode pathNode) {
906-
result = count(Node node, string toString | step(pathNode) = getAPathNode(node, toString))
896+
result = count(Node node, string toString | step(pathNode, getAPathNode(node, toString)))
907897
}
908898

909899
/**
@@ -912,13 +902,46 @@ module DataFlowMake<LocationSig Location, InputSig<Location> Lang> {
912902
private int getOutDegreeFromNode(Node node, string toString) {
913903
result =
914904
strictcount(Node node2, string toString2 |
915-
step(getAPathNode(node, toString)) = getAPathNode(node2, toString2)
905+
step(getAPathNode(node, toString), getAPathNode(node2, toString2))
916906
)
917907
}
918908

909+
/**
910+
* Like `getOutDegreeFromPathNode` except counts `subpath` tuples.
911+
*/
912+
private int getSubpathOutDegreeFromPathNode(InputPathNode pathNode) {
913+
result =
914+
count(Node n1, string s1, Node n2, string s2, Node n3, string s3 |
915+
subpathStep(pathNode, getAPathNode(n1, s1), getAPathNode(n2, s2), getAPathNode(n3, s3))
916+
)
917+
}
918+
919+
/**
920+
* Like `getOutDegreeFromNode` except counts `subpath` tuples.
921+
*/
922+
private int getSubpathOutDegreeFromNode(Node node, string toString) {
923+
result =
924+
strictcount(Node n1, string s1, Node n2, string s2, Node n3, string s3 |
925+
subpathStep(getAPathNode(node, toString), getAPathNode(n1, s1), getAPathNode(n2, s2),
926+
getAPathNode(n3, s3))
927+
)
928+
}
929+
930+
/** Gets a successor of `node` including subpath flow-through. */
931+
InputPathNode stepEx(InputPathNode node) {
932+
step(node, result)
933+
or
934+
subpathStep(node, _, _, result) // assuming the input is pruned properly, all subpaths have flow-through
935+
}
936+
937+
InputPathNode enterSubpathStep(InputPathNode node) { subpathStep(node, result, _, _) }
938+
939+
InputPathNode exitSubpathStep(InputPathNode node) { subpathStep(_, _, node, result) }
940+
919941
/** Holds if `(node, toString)` cannot be collapsed (but was a candidate for being collapsed). */
920-
predicate discriminatedPair(Node node, string toString) {
942+
predicate discriminatedPair(Node node, string toString, boolean hasEnter) {
921943
collapseCandidate(node, toString) and
944+
hasEnter = false and
922945
(
923946
// Check if all corresponding PathNodes have the same successor sets when projected to `(node, toString)`.
924947
// To do this, we check that each successor set has the same size as the union of the succesor sets.
@@ -928,27 +951,62 @@ module DataFlowMake<LocationSig Location, InputSig<Location> Lang> {
928951
getOutDegreeFromPathNode(getAPathNode(node, toString)) <
929952
getOutDegreeFromNode(node, toString)
930953
or
954+
// Same as above but counting associated subpath triples instead
955+
getSubpathOutDegreeFromPathNode(getAPathNode(node, toString)) <
956+
getSubpathOutDegreeFromNode(node, toString)
957+
)
958+
or
959+
collapseCandidate(node, toString) and
960+
(
931961
// Retain flow state if one of the successors requires it to be retained
932-
discriminatedPathNode(step(getAPathNode(node, toString)))
962+
discriminatedPathNode(stepEx(getAPathNode(node, toString)), hasEnter)
963+
or
964+
// Enter a subpath
965+
discriminatedPathNode(enterSubpathStep(getAPathNode(node, toString)), _) and
966+
hasEnter = true
967+
or
968+
// Exit a subpath
969+
discriminatedPathNode(exitSubpathStep(getAPathNode(node, toString)), false) and
970+
hasEnter = false
933971
)
934972
}
935973

936974
/** Holds if `pathNode` cannot be collapsed. */
937-
predicate discriminatedPathNode(InputPathNode pathNode) {
975+
private predicate discriminatedPathNode(InputPathNode pathNode, boolean hasEnter) {
938976
exists(Node node, string toString |
939-
discriminatedPair(node, toString) and
977+
discriminatedPair(node, toString, hasEnter) and
940978
getAPathNode(node, toString) = pathNode
941979
)
942980
}
981+
982+
/** Holds if `(node, toString)` cannot be collapsed (but was a candidate for being collapsed). */
983+
predicate discriminatedPair(Node node, string toString) {
984+
discriminatedPair(node, toString, _)
985+
}
986+
987+
/** Holds if `pathNode` cannot be collapsed. */
988+
predicate discriminatedPathNode(InputPathNode pathNode) { discriminatedPathNode(pathNode, _) }
943989
}
944990

945991
private predicate initialCandidate(Node node, string toString) {
946992
exists(getAPathNode(node, toString))
947993
}
948994

949-
private module Pass1 = MakeDiscriminatorPass<initialCandidate/2, getASuccessorLike/1>;
995+
private module Pass1 =
996+
MakeDiscriminatorPass<initialCandidate/2, Graph::edges/2, Graph::subpaths/4>;
997+
998+
private predicate edgesRev(InputPathNode node1, InputPathNode node2) {
999+
Graph::edges(node2, node1)
1000+
}
1001+
1002+
private predicate subpathsRev(
1003+
InputPathNode n1, InputPathNode n2, InputPathNode n3, InputPathNode n4
1004+
) {
1005+
Graph::subpaths(n4, n3, n2, n1)
1006+
}
9501007

951-
private module Pass2 = MakeDiscriminatorPass<Pass1::discriminatedPair/2, getAPredecessorLike/1>;
1008+
private module Pass2 =
1009+
MakeDiscriminatorPass<Pass1::discriminatedPair/2, edgesRev/2, subpathsRev/4>;
9521010

9531011
private newtype TPathNode =
9541012
TPreservedPathNode(InputPathNode node) { Pass2::discriminatedPathNode(node) } or

0 commit comments

Comments
 (0)