Skip to content

Commit 5fe942e

Browse files
author
Robert Marsh
committed
Swift: flow through writeable keypaths
1 parent 12a717e commit 5fe942e

File tree

4 files changed

+185
-2
lines changed

4 files changed

+185
-2
lines changed

‎swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,22 @@ private class KeyPathComponentNodeImpl extends TKeyPathComponentNode, NodeImpl {
5757
KeyPathComponent getComponent() { result = component }
5858
}
5959

60+
private class KeyPathComponentPostUpdateNode extends TKeyPathComponentPostUpdateNode, NodeImpl {
61+
KeyPathComponent component;
62+
63+
KeyPathComponentPostUpdateNode() { this = TKeyPathComponentPostUpdateNode(component) }
64+
65+
override Location getLocationImpl() { result = component.getLocation() }
66+
67+
override string toStringImpl() { result = "[post] " + component.toString() }
68+
69+
override DataFlowCallable getEnclosingCallable() {
70+
result.asSourceCallable() = component.getKeyPathExpr()
71+
}
72+
73+
KeyPathComponent getComponent() { result = component }
74+
}
75+
6076
private class PatternNodeImpl extends PatternNode, NodeImpl {
6177
override Location getLocationImpl() { result = pattern.getLocation() }
6278

@@ -96,6 +112,9 @@ private module Cached {
96112
TKeyPathParameterNode(EntryNode entry) { entry.getScope() instanceof KeyPathExpr } or
97113
TKeyPathReturnNode(ExitNode exit) { exit.getScope() instanceof KeyPathExpr } or
98114
TKeyPathComponentNode(KeyPathComponent component) or
115+
TKeyPathParameterPostUpdateNode(EntryNode entry) { entry.getScope() instanceof KeyPathExpr } or
116+
TKeyPathReturnPostUpdateNode(ExitNode exit) { exit.getScope() instanceof KeyPathExpr } or
117+
TKeyPathComponentPostUpdateNode(KeyPathComponent component) or
99118
TExprPostUpdateNode(CfgNode n) {
100119
// Obviously, the base of setters needs a post-update node
101120
n = any(PropertySetterCfgNode setter).getBase()
@@ -105,6 +124,8 @@ private module Cached {
105124
or
106125
n = any(PropertyObserverCfgNode getter).getBase()
107126
or
127+
n = any(KeyPathApplicationExprCfgNode expr).getBase()
128+
or
108129
// Arguments that are `inout` expressions needs a post-update node,
109130
// as well as any class-like argument (since a field can be modified).
110131
// Finally, qualifiers and bases of member reference need post-update nodes to support reverse reads.
@@ -227,6 +248,17 @@ private module Cached {
227248
nodeTo.(KeyPathComponentNodeImpl).getComponent() =
228249
nodeFrom.(KeyPathParameterNode).getComponent(0)
229250
or
251+
nodeFrom.(KeyPathComponentPostUpdateNode).getComponent() =
252+
nodeTo.(KeyPathParameterPostUpdateNode).getComponent(0)
253+
or
254+
// Flow to the result of a keypath assignment
255+
// TODO: is there a cleaner way to do this?
256+
exists(KeyPathApplicationExpr apply, AssignExpr assign |
257+
apply = assign.getDest() and
258+
nodeTo.asExpr() = apply and
259+
nodeFrom.asExpr() = assign.getSource()
260+
)
261+
or
230262
// flow through a flow summary (extension of `SummaryModelCsv`)
231263
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom.(FlowSummaryNode).getSummaryNode(),
232264
nodeTo.(FlowSummaryNode).getSummaryNode(), true)
@@ -383,6 +415,56 @@ class FlowSummaryNode extends NodeImpl, TFlowSummaryNode {
383415
override string toStringImpl() { result = this.getSummaryNode().toString() }
384416
}
385417

418+
class KeyPathParameterPostUpdateNode extends NodeImpl, ReturnNode, PostUpdateNodeImpl,
419+
TKeyPathParameterPostUpdateNode
420+
{
421+
private EntryNode entry;
422+
423+
KeyPathParameterPostUpdateNode() { this = TKeyPathParameterPostUpdateNode(entry) }
424+
425+
override KeyPathParameterNode getPreUpdateNode() {
426+
result.getKeyPathExpr() = this.getKeyPathExpr()
427+
}
428+
429+
override Location getLocationImpl() { result = entry.getLocation() }
430+
431+
override string toStringImpl() { result = "[post] " + entry.toString() }
432+
433+
override DataFlowCallable getEnclosingCallable() { result.asSourceCallable() = entry.getScope() }
434+
435+
KeyPathComponent getComponent(int i) { result = entry.getScope().(KeyPathExpr).getComponent(i) }
436+
437+
KeyPathComponent getAComponent() { result = this.getComponent(_) }
438+
439+
KeyPathExpr getKeyPathExpr() { result = entry.getScope() }
440+
441+
override ReturnKind getKind() { result.(ParamReturnKind).getIndex() = -1 }
442+
}
443+
444+
class KeyPathReturnPostUpdateNode extends NodeImpl, ParameterNodeImpl, PostUpdateNodeImpl,
445+
TKeyPathReturnPostUpdateNode
446+
{
447+
private ExitNode exit;
448+
449+
KeyPathReturnPostUpdateNode() { this = TKeyPathReturnPostUpdateNode(exit) }
450+
451+
override KeyPathParameterNode getPreUpdateNode() {
452+
result.getKeyPathExpr() = this.getKeyPathExpr()
453+
}
454+
455+
override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
456+
c.asSourceCallable() = this.getKeyPathExpr() and pos = TPositionalParameter(0) // TODO: new parameter type?
457+
}
458+
459+
override Location getLocationImpl() { result = exit.getLocation() }
460+
461+
override string toStringImpl() { result = "[post] " + exit.toString() }
462+
463+
override DataFlowCallable getEnclosingCallable() { result.asSourceCallable() = exit.getScope() }
464+
465+
KeyPathExpr getKeyPathExpr() { result = exit.getScope() }
466+
}
467+
386468
/** A data-flow node that represents a call argument. */
387469
abstract class ArgumentNode extends Node {
388470
/** Holds if this argument occurs at the given position in the given call. */
@@ -476,6 +558,20 @@ private module ArgumentNodes {
476558
pos = TThisArgument()
477559
}
478560
}
561+
562+
class KeyPathAssignmentArgumentNode extends ArgumentNode {
563+
private KeyPathApplicationExprCfgNode keyPath;
564+
565+
KeyPathAssignmentArgumentNode() {
566+
keyPath = this.getCfgNode() and
567+
exists(AssignExpr assign | assign.getDest() = keyPath.getNode().asAstNode())
568+
}
569+
570+
override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
571+
call.asKeyPath() = keyPath and
572+
pos = TPositionalArgument(0) // TODO: new parameter type?
573+
}
574+
}
479575
}
480576

481577
import ArgumentNodes
@@ -735,6 +831,24 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
735831
c instanceof OptionalSomeContentSet
736832
)
737833
or
834+
// store through a component in a key-path expression chain
835+
exists(KeyPathComponent component |
836+
component = node2.(KeyPathComponentPostUpdateNode).getComponent() and
837+
(
838+
c.isSingleton(any(Content::FieldContent ct | ct.getField() = component.getDeclRef()))
839+
or
840+
c.isSingleton(any(Content::ArrayContent ac)) and
841+
component.isSubscript()
842+
)
843+
|
844+
// the previous node is either the next element in the chain
845+
node1.(KeyPathComponentPostUpdateNode).getComponent() = component.getNextComponent()
846+
or
847+
// or the return node, if this is the last component in the chain
848+
not exists(component.getNextComponent()) and
849+
node1.(KeyPathReturnPostUpdateNode).getKeyPathExpr() = component.getKeyPathExpr()
850+
)
851+
or
738852
FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c,
739853
node2.(FlowSummaryNode).getSummaryNode())
740854
}

‎swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
edges
22
| file://:0:0:0:0 | KeyPathComponent | test.swift:663:13:663:29 | exit #keyPath(...) [some:0] |
33
| file://:0:0:0:0 | self [a, x] | file://:0:0:0:0 | .a [x] |
4+
| file://:0:0:0:0 | self [s, x] | file://:0:0:0:0 | .s [x] |
45
| file://:0:0:0:0 | self [str] | file://:0:0:0:0 | .str |
56
| file://:0:0:0:0 | self [v2, some:0] | file://:0:0:0:0 | .v2 [some:0] |
67
| file://:0:0:0:0 | self [v2] | file://:0:0:0:0 | .v2 |
78
| file://:0:0:0:0 | self [v3] | file://:0:0:0:0 | .v3 |
89
| file://:0:0:0:0 | self [x, some:0] | file://:0:0:0:0 | .x [some:0] |
910
| file://:0:0:0:0 | self [x] | file://:0:0:0:0 | .x |
11+
| file://:0:0:0:0 | self [x] | file://:0:0:0:0 | .x |
1012
| file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [v2] |
1113
| file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [v3] |
1214
| file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [x] |
@@ -299,6 +301,7 @@ edges
299301
| test.swift:599:24:599:32 | call to source3() | test.swift:599:13:599:33 | call to MyClass.init(s:) [str] |
300302
| test.swift:600:13:600:41 | call to MyClass.init(contentsOfFile:) [str] | test.swift:585:9:585:9 | self [str] |
301303
| test.swift:600:13:600:41 | call to MyClass.init(contentsOfFile:) [str] | test.swift:600:13:600:43 | .str |
304+
| test.swift:615:7:615:7 | self [x] | file://:0:0:0:0 | self [x] |
302305
| test.swift:617:8:617:11 | x | test.swift:618:14:618:14 | x |
303306
| test.swift:618:5:618:5 | [post] self [x] | test.swift:617:3:619:3 | self[return] [x] |
304307
| test.swift:618:14:618:14 | x | test.swift:618:5:618:5 | [post] self [x] |
@@ -314,6 +317,7 @@ edges
314317
| test.swift:627:38:627:38 | KeyPathComponent [x] | test.swift:627:36:627:38 | exit #keyPath(...) |
315318
| test.swift:628:13:628:13 | s [x] | test.swift:627:36:627:38 | enter #keyPath(...) [x] |
316319
| test.swift:628:13:628:13 | s [x] | test.swift:628:13:628:32 | \\...[...] |
320+
| test.swift:632:7:632:7 | self [s, x] | file://:0:0:0:0 | self [s, x] |
317321
| test.swift:634:8:634:11 | s [x] | test.swift:635:14:635:14 | s [x] |
318322
| test.swift:635:5:635:5 | [post] self [s, x] | test.swift:634:3:636:3 | self[return] [s, x] |
319323
| test.swift:635:14:635:14 | s [x] | test.swift:635:5:635:5 | [post] self [s, x] |
@@ -438,13 +442,26 @@ edges
438442
| test.swift:766:29:766:29 | KeyPathComponent [x] | test.swift:766:13:766:29 | exit #keyPath(...) |
439443
| test.swift:767:15:767:15 | s2 [s, some:0, x] | test.swift:766:13:766:29 | enter #keyPath(...) [s, some:0, x] |
440444
| test.swift:767:15:767:15 | s2 [s, some:0, x] | test.swift:767:15:767:28 | \\...[...] |
445+
| test.swift:773:11:773:17 | [post] exit #keyPath(...) | test.swift:773:17:773:17 | [post] KeyPathComponent [x] |
446+
| test.swift:773:15:773:15 | [post] KeyPathComponent [s, x] | test.swift:773:11:773:17 | [post] enter #keyPath(...) [s, x] |
447+
| test.swift:773:17:773:17 | [post] KeyPathComponent [x] | test.swift:773:15:773:15 | [post] KeyPathComponent [s, x] |
448+
| test.swift:774:3:774:3 | [post] s2 [s, x] | test.swift:775:13:775:13 | s2 [s, x] |
449+
| test.swift:774:3:774:16 | \\...[...] | test.swift:773:11:773:17 | [post] exit #keyPath(...) |
450+
| test.swift:774:3:774:16 | \\...[...] | test.swift:774:3:774:3 | [post] s2 [s, x] |
451+
| test.swift:774:20:774:27 | call to source() | test.swift:774:3:774:16 | \\...[...] |
452+
| test.swift:775:13:775:13 | s2 [s, x] | test.swift:632:7:632:7 | self [s, x] |
453+
| test.swift:775:13:775:13 | s2 [s, x] | test.swift:775:13:775:16 | .s [x] |
454+
| test.swift:775:13:775:16 | .s [x] | test.swift:615:7:615:7 | self [x] |
455+
| test.swift:775:13:775:16 | .s [x] | test.swift:775:13:775:18 | .x |
441456
nodes
442457
| file://:0:0:0:0 | .a [x] | semmle.label | .a [x] |
458+
| file://:0:0:0:0 | .s [x] | semmle.label | .s [x] |
443459
| file://:0:0:0:0 | .str | semmle.label | .str |
444460
| file://:0:0:0:0 | .v2 | semmle.label | .v2 |
445461
| file://:0:0:0:0 | .v2 [some:0] | semmle.label | .v2 [some:0] |
446462
| file://:0:0:0:0 | .v3 | semmle.label | .v3 |
447463
| file://:0:0:0:0 | .x | semmle.label | .x |
464+
| file://:0:0:0:0 | .x | semmle.label | .x |
448465
| file://:0:0:0:0 | .x [some:0] | semmle.label | .x [some:0] |
449466
| file://:0:0:0:0 | KeyPathComponent | semmle.label | KeyPathComponent |
450467
| file://:0:0:0:0 | [post] self [v2, some:0] | semmle.label | [post] self [v2, some:0] |
@@ -453,12 +470,14 @@ nodes
453470
| file://:0:0:0:0 | [post] self [x, some:0] | semmle.label | [post] self [x, some:0] |
454471
| file://:0:0:0:0 | [post] self [x] | semmle.label | [post] self [x] |
455472
| file://:0:0:0:0 | self [a, x] | semmle.label | self [a, x] |
473+
| file://:0:0:0:0 | self [s, x] | semmle.label | self [s, x] |
456474
| file://:0:0:0:0 | self [str] | semmle.label | self [str] |
457475
| file://:0:0:0:0 | self [v2, some:0] | semmle.label | self [v2, some:0] |
458476
| file://:0:0:0:0 | self [v2] | semmle.label | self [v2] |
459477
| file://:0:0:0:0 | self [v3] | semmle.label | self [v3] |
460478
| file://:0:0:0:0 | self [x, some:0] | semmle.label | self [x, some:0] |
461479
| file://:0:0:0:0 | self [x] | semmle.label | self [x] |
480+
| file://:0:0:0:0 | self [x] | semmle.label | self [x] |
462481
| file://:0:0:0:0 | value | semmle.label | value |
463482
| file://:0:0:0:0 | value | semmle.label | value |
464483
| file://:0:0:0:0 | value | semmle.label | value |
@@ -769,6 +788,7 @@ nodes
769788
| test.swift:599:24:599:32 | call to source3() | semmle.label | call to source3() |
770789
| test.swift:600:13:600:41 | call to MyClass.init(contentsOfFile:) [str] | semmle.label | call to MyClass.init(contentsOfFile:) [str] |
771790
| test.swift:600:13:600:43 | .str | semmle.label | .str |
791+
| test.swift:615:7:615:7 | self [x] | semmle.label | self [x] |
772792
| test.swift:617:3:619:3 | self[return] [x] | semmle.label | self[return] [x] |
773793
| test.swift:617:8:617:11 | x | semmle.label | x |
774794
| test.swift:618:5:618:5 | [post] self [x] | semmle.label | [post] self [x] |
@@ -785,6 +805,7 @@ nodes
785805
| test.swift:627:38:627:38 | KeyPathComponent [x] | semmle.label | KeyPathComponent [x] |
786806
| test.swift:628:13:628:13 | s [x] | semmle.label | s [x] |
787807
| test.swift:628:13:628:32 | \\...[...] | semmle.label | \\...[...] |
808+
| test.swift:632:7:632:7 | self [s, x] | semmle.label | self [s, x] |
788809
| test.swift:634:3:636:3 | self[return] [s, x] | semmle.label | self[return] [s, x] |
789810
| test.swift:634:8:634:11 | s [x] | semmle.label | s [x] |
790811
| test.swift:635:5:635:5 | [post] self [s, x] | semmle.label | [post] self [s, x] |
@@ -915,6 +936,16 @@ nodes
915936
| test.swift:766:29:766:29 | KeyPathComponent [x] | semmle.label | KeyPathComponent [x] |
916937
| test.swift:767:15:767:15 | s2 [s, some:0, x] | semmle.label | s2 [s, some:0, x] |
917938
| test.swift:767:15:767:28 | \\...[...] | semmle.label | \\...[...] |
939+
| test.swift:773:11:773:17 | [post] enter #keyPath(...) [s, x] | semmle.label | [post] enter #keyPath(...) [s, x] |
940+
| test.swift:773:11:773:17 | [post] exit #keyPath(...) | semmle.label | [post] exit #keyPath(...) |
941+
| test.swift:773:15:773:15 | [post] KeyPathComponent [s, x] | semmle.label | [post] KeyPathComponent [s, x] |
942+
| test.swift:773:17:773:17 | [post] KeyPathComponent [x] | semmle.label | [post] KeyPathComponent [x] |
943+
| test.swift:774:3:774:3 | [post] s2 [s, x] | semmle.label | [post] s2 [s, x] |
944+
| test.swift:774:3:774:16 | \\...[...] | semmle.label | \\...[...] |
945+
| test.swift:774:20:774:27 | call to source() | semmle.label | call to source() |
946+
| test.swift:775:13:775:13 | s2 [s, x] | semmle.label | s2 [s, x] |
947+
| test.swift:775:13:775:16 | .s [x] | semmle.label | .s [x] |
948+
| test.swift:775:13:775:18 | .x | semmle.label | .x |
918949
subpaths
919950
| test.swift:75:22:75:22 | x | test.swift:65:16:65:28 | arg1 | test.swift:65:1:70:1 | arg2[return] | test.swift:75:32:75:32 | [post] y |
920951
| test.swift:114:19:114:19 | arg | test.swift:109:9:109:14 | arg | test.swift:110:12:110:12 | arg | test.swift:114:12:114:22 | call to ... |
@@ -969,6 +1000,9 @@ subpaths
9691000
| test.swift:764:18:764:25 | call to source() | test.swift:617:8:617:11 | x | test.swift:617:3:619:3 | self[return] [x] | test.swift:764:13:764:26 | call to S.init(x:) [x] |
9701001
| test.swift:765:29:765:29 | s [some:0, x] | test.swift:655:8:655:12 | s [some:0, x] | test.swift:655:3:657:3 | self[return] [s, some:0, x] | test.swift:765:14:765:30 | call to S2_Optional.init(s:) [s, some:0, x] |
9711002
| test.swift:767:15:767:15 | s2 [s, some:0, x] | test.swift:766:13:766:29 | enter #keyPath(...) [s, some:0, x] | test.swift:766:13:766:29 | exit #keyPath(...) | test.swift:767:15:767:28 | \\...[...] |
1003+
| test.swift:774:3:774:16 | \\...[...] | test.swift:773:11:773:17 | [post] exit #keyPath(...) | test.swift:773:11:773:17 | [post] enter #keyPath(...) [s, x] | test.swift:774:3:774:3 | [post] s2 [s, x] |
1004+
| test.swift:775:13:775:13 | s2 [s, x] | test.swift:632:7:632:7 | self [s, x] | file://:0:0:0:0 | .s [x] | test.swift:775:13:775:16 | .s [x] |
1005+
| test.swift:775:13:775:16 | .s [x] | test.swift:615:7:615:7 | self [x] | file://:0:0:0:0 | .x | test.swift:775:13:775:18 | .x |
9721006
#select
9731007
| test.swift:7:15:7:15 | t1 | test.swift:6:19:6:26 | call to source() | test.swift:7:15:7:15 | t1 | result |
9741008
| test.swift:9:15:9:15 | t1 | test.swift:6:19:6:26 | call to source() | test.swift:9:15:9:15 | t1 | result |
@@ -1070,3 +1104,4 @@ subpaths
10701104
| test.swift:756:15:756:21 | ...! | test.swift:746:14:746:21 | call to source() | test.swift:756:15:756:21 | ...! | result |
10711105
| test.swift:757:15:757:19 | .v3 | test.swift:747:14:747:21 | call to source() | test.swift:757:15:757:19 | .v3 | result |
10721106
| test.swift:767:15:767:28 | \\...[...] | test.swift:764:18:764:25 | call to source() | test.swift:767:15:767:28 | \\...[...] | result |
1107+
| test.swift:775:13:775:18 | .x | test.swift:774:20:774:27 | call to source() | test.swift:775:13:775:18 | .x | result |

0 commit comments

Comments
 (0)