Skip to content

Commit 1c298e6

Browse files
MathiasVPMathiasVP
committed
Swift: Fix 'parameter' -> 'argument' flow into closures.
1 parent 310ebe4 commit 1c298e6

File tree

12 files changed

+90
-26
lines changed

12 files changed

+90
-26
lines changed

‎swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ class ApplyExprCfgNode extends ExprCfgNode {
173173

174174
Callable getStaticTarget() { result = e.getStaticTarget() }
175175

176-
Expr getFunction() { result = e.getFunction() }
176+
CfgNode getFunction() { result.getAst() = e.getFunction() }
177177
}
178178

179179
class CallExprCfgNode extends ApplyExprCfgNode {

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,12 +293,14 @@ private module Cached {
293293
newtype TArgumentPosition =
294294
TThisArgument() or
295295
// we rely on default exprs generated in the caller for ordering
296-
TPositionalArgument(int n) { n = any(Argument arg).getIndex() }
296+
TPositionalArgument(int n) { n = any(Argument arg).getIndex() } or
297+
TClosureSelfArgument()
297298

298299
cached
299300
newtype TParameterPosition =
300301
TThisParameter() or
301-
TPositionalParameter(int n) { n = any(Argument arg).getIndex() }
302+
TPositionalParameter(int n) { n = any(Argument arg).getIndex() } or
303+
TClosureSelfParameter()
302304
}
303305

304306
import Cached
@@ -331,6 +333,10 @@ class ThisParameterPosition extends ParameterPosition, TThisParameter {
331333
override string toString() { result = "this" }
332334
}
333335

336+
class ClosureSelfParameter extends ParameterPosition, TClosureSelfParameter {
337+
override string toString() { result = "{ ... }" }
338+
}
339+
334340
/** An argument position. */
335341
class ArgumentPosition extends TArgumentPosition {
336342
/** Gets a textual representation of this position. */
@@ -347,11 +353,18 @@ class ThisArgumentPosition extends ArgumentPosition, TThisArgument {
347353
override string toString() { result = "this" }
348354
}
349355

356+
class ClosureSelfArgument extends ArgumentPosition, TClosureSelfArgument {
357+
override string toString() { result = "{ ... }" }
358+
}
359+
350360
/** Holds if arguments at position `apos` match parameters at position `ppos`. */
351361
pragma[inline]
352362
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
353363
ppos instanceof TThisParameter and
354364
apos instanceof TThisArgument
355365
or
356366
ppos.(PositionalParameterPosition).getIndex() = apos.(PositionalArgumentPosition).getIndex()
367+
or
368+
ppos instanceof TClosureSelfParameter and
369+
apos instanceof TClosureSelfArgument
357370
}

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

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,12 @@ private class DictionarySubscriptNode extends NodeImpl, TDictionarySubscriptNode
382382
SubscriptExpr getExpr() { result = expr }
383383
}
384384

385+
private class ClosureSelfReferenceNode extends ExprNodeImpl {
386+
override ClosureExpr expr;
387+
388+
ClosureExpr getClosure() { result = expr }
389+
}
390+
385391
private module ParameterNodes {
386392
abstract class ParameterNodeImpl extends NodeImpl {
387393
predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { none() }
@@ -412,6 +418,13 @@ private module ParameterNodes {
412418
override ParamDecl getParameter() { result = param }
413419
}
414420

421+
class ClosureSelfParameterNode extends ParameterNodeImpl, ClosureSelfReferenceNode {
422+
override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
423+
c.asSourceCallable() = this.getClosure() and
424+
pos instanceof ClosureSelfParameter
425+
}
426+
}
427+
415428
class SummaryParameterNode extends ParameterNodeImpl, FlowSummaryNode {
416429
SummaryParameterNode() {
417430
FlowSummaryImpl::Private::summaryParameterNode(this.getSummaryNode(), _)
@@ -626,6 +639,18 @@ private module ArgumentNodes {
626639
pos = TPositionalArgument(0)
627640
}
628641
}
642+
643+
class SelfClosureArgumentNode extends ExprNode, ArgumentNode {
644+
ApplyExprCfgNode apply;
645+
646+
SelfClosureArgumentNode() { n = apply.getFunction() }
647+
648+
override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
649+
apply = call.asCall() and
650+
not exists(apply.getStaticTarget()) and
651+
pos instanceof ClosureSelfArgument
652+
}
653+
}
629654
}
630655

631656
import ArgumentNodes
@@ -878,8 +903,8 @@ private module CaptureInput implements VariableCapture::InputSig {
878903
source = a.getSource()
879904
)
880905
or
881-
exists(S::PatternBindingDecl pbd, S::NamedPattern np | this = pbd and pbd.getAPattern() = np |
882-
np.getVarDecl() = variable and
906+
exists(S::NamedPattern np | this = np |
907+
variable = np.getVarDecl() and
883908
source = np.getMatchingExpr()
884909
)
885910
// TODO: support multiple variables in LHS of =, in both of above cases.
@@ -918,13 +943,23 @@ class CapturedParameter = CaptureInput::CapturedParameter;
918943
module CaptureFlow = VariableCapture::Flow<CaptureInput>;
919944

920945
private CaptureFlow::ClosureNode asClosureNode(Node n) {
921-
result = n.(CaptureNode).getSynthesizedCaptureNode() or
922-
result.(CaptureFlow::ExprNode).getExpr() = n.asExpr() or
946+
result = n.(CaptureNode).getSynthesizedCaptureNode()
947+
or
948+
result.(CaptureFlow::ExprNode).getExpr() = n.asExpr()
949+
or
923950
result.(CaptureFlow::ExprPostUpdateNode).getExpr() =
924-
n.(PostUpdateNode).getPreUpdateNode().asExpr() or
925-
result.(CaptureFlow::ParameterNode).getParameter() = n.getParameter() or
926-
result.(CaptureFlow::ThisParameterNode).getCallable().getSelfParam() = n.getParameter() or
951+
n.(PostUpdateNode).getPreUpdateNode().asExpr()
952+
or
953+
result.(CaptureFlow::ParameterNode).getParameter() = n.getParameter()
954+
or
955+
result.(CaptureFlow::ThisParameterNode).getCallable() = n.(ClosureSelfParameterNode).getClosure()
956+
or
927957
result.(CaptureFlow::MallocNode).getClosureExpr() = n.getCfgNode().getNode().asAstNode() // TODO: figure out why the java version had PostUpdateNode logic here
958+
or
959+
exists(CaptureInput::VariableWrite write |
960+
result.(CaptureFlow::VariableWriteSourceNode).getVariableWrite() = write and
961+
n.asExpr() = write.getSource()
962+
)
928963
}
929964

930965
private predicate captureStoreStep(Node node1, Content::CapturedVariableContent c, Node node2) {

‎swift/ql/test/library-tests/dataflow/capture/closures.swift

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,27 +11,27 @@ func captureList() {
1111
let y: Int = source("captureList", 123);
1212
{ [x = hello()] () in
1313
sink(x) // $ MISSING: hasValueFlow=hello
14-
sink(y) // $ MISSING: hasValueFlow=captureList
14+
sink(y) // $ hasValueFlow=captureList
1515
}()
1616
}
1717

1818
func setAndCallEscape() {
1919
let x = source("setAndCallEscape", 0)
2020

2121
let escape = {
22-
sink(x) // $ MISSING: hasValueFlow=setAndCallEscape
22+
sink(x) // $ hasValueFlow=setAndCallEscape
2323
return x + 1
2424
}
2525

26-
sink(escape()) // $ MISSING: hasTaintFlow=setAndCallEscape
26+
sink(escape()) // $ hasTaintFlow=setAndCallEscape
2727
}
2828

2929
var escape: (() -> Int)? = nil
3030

3131
func setEscape() {
3232
let x = source("setEscape", 0)
3333
escape = {
34-
sink(x) // $ MISSING: hasValueFlow=setEscape
34+
sink(x) // $ hasValueFlow=setEscape
3535
return x + 1
3636
}
3737
}
@@ -73,7 +73,7 @@ func foo() -> Int {
7373
let f = { y in x += y }
7474
x = source("foo", 41)
7575
let r = { x }
76-
sink(r()) // $ MISSING: hasValueFlow=foo
76+
sink(r()) // $ hasValueFlow=foo
7777
f(1)
7878
return r()
7979
}
@@ -138,12 +138,12 @@ func sharedCaptureMultipleWriters() {
138138

139139
func taintCollections(array: inout Array<Int>) {
140140
array[0] = source("array", 0)
141-
sink(array)
141+
sink(array) // $ hasTaintFlow=array
142142
sink(array[0]) // $ hasValueFlow=array
143143
array.withContiguousStorageIfAvailable({
144144
buffer in
145-
sink(array)
146-
sink(array[0]) // $ MISSING: hasValueFlow=array
145+
sink(array) // $ hasTaintFlow=array
146+
sink(array[0]) // $ hasValueFlow=array
147147
})
148148
}
149149

‎swift/ql/test/library-tests/dataflow/capture/inlinetest.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
failures
21
testFailures
2+
failures

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ edges
4747
| test.swift:113:14:113:19 | arg | test.swift:114:19:114:19 | arg |
4848
| test.swift:113:14:113:19 | arg | test.swift:114:19:114:19 | arg |
4949
| test.swift:114:19:114:19 | arg | test.swift:109:9:109:14 | arg |
50+
| test.swift:114:19:114:19 | arg | test.swift:109:9:109:14 | arg |
5051
| test.swift:114:19:114:19 | arg | test.swift:114:12:114:22 | call to ... |
5152
| test.swift:114:19:114:19 | arg | test.swift:114:12:114:22 | call to ... |
5253
| test.swift:114:19:114:19 | arg | test.swift:123:10:123:13 | i |
@@ -58,6 +59,9 @@ edges
5859
| test.swift:122:31:122:38 | call to source() | test.swift:113:14:113:19 | arg |
5960
| test.swift:122:31:122:38 | call to source() | test.swift:122:18:125:6 | call to forward(arg:lambda:) |
6061
| test.swift:123:10:123:13 | i | test.swift:124:16:124:16 | i |
62+
| test.swift:128:22:131:6 | call to forward(arg:lambda:) | test.swift:132:15:132:15 | clean |
63+
| test.swift:128:35:128:42 | call to source() | test.swift:113:14:113:19 | arg |
64+
| test.swift:128:35:128:42 | call to source() | test.swift:128:22:131:6 | call to forward(arg:lambda:) |
6165
| test.swift:142:10:142:13 | i | test.swift:143:16:143:16 | i |
6266
| test.swift:145:23:145:30 | call to source() | test.swift:142:10:142:13 | i |
6367
| test.swift:145:23:145:30 | call to source() | test.swift:145:15:145:31 | call to ... |
@@ -693,6 +697,9 @@ nodes
693697
| test.swift:123:10:123:13 | i | semmle.label | i |
694698
| test.swift:124:16:124:16 | i | semmle.label | i |
695699
| test.swift:126:15:126:15 | z | semmle.label | z |
700+
| test.swift:128:22:131:6 | call to forward(arg:lambda:) | semmle.label | call to forward(arg:lambda:) |
701+
| test.swift:128:35:128:42 | call to source() | semmle.label | call to source() |
702+
| test.swift:132:15:132:15 | clean | semmle.label | clean |
696703
| test.swift:138:19:138:26 | call to source() | semmle.label | call to source() |
697704
| test.swift:142:10:142:13 | i | semmle.label | i |
698705
| test.swift:143:16:143:16 | i | semmle.label | i |
@@ -1257,9 +1264,11 @@ nodes
12571264
subpaths
12581265
| 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 |
12591266
| 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 ... |
1267+
| 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 ... |
12601268
| test.swift:114:19:114:19 | arg | test.swift:123:10:123:13 | i | test.swift:124:16:124:16 | i | test.swift:114:12:114:22 | call to ... |
12611269
| test.swift:119:31:119:31 | x | test.swift:113:14:113:19 | arg | test.swift:114:12:114:22 | call to ... | test.swift:119:18:119:44 | call to forward(arg:lambda:) |
12621270
| test.swift:122:31:122:38 | call to source() | test.swift:113:14:113:19 | arg | test.swift:114:12:114:22 | call to ... | test.swift:122:18:125:6 | call to forward(arg:lambda:) |
1271+
| test.swift:128:35:128:42 | call to source() | test.swift:113:14:113:19 | arg | test.swift:114:12:114:22 | call to ... | test.swift:128:22:131:6 | call to forward(arg:lambda:) |
12631272
| test.swift:145:23:145:30 | call to source() | test.swift:142:10:142:13 | i | test.swift:143:16:143:16 | i | test.swift:145:15:145:31 | call to ... |
12641273
| test.swift:170:9:170:9 | value | test.swift:163:7:163:7 | value | file://:0:0:0:0 | [post] self [x] | test.swift:170:5:170:5 | [post] self [x] |
12651274
| test.swift:174:12:174:12 | self [x] | test.swift:163:7:163:7 | self [x] | file://:0:0:0:0 | .x | test.swift:174:12:174:12 | .x |
@@ -1338,6 +1347,7 @@ subpaths
13381347
| test.swift:105:19:105:19 | x | test.swift:89:15:89:22 | call to source() | test.swift:105:19:105:19 | x | result |
13391348
| test.swift:120:15:120:15 | y | test.swift:118:18:118:25 | call to source() | test.swift:120:15:120:15 | y | result |
13401349
| test.swift:126:15:126:15 | z | test.swift:122:31:122:38 | call to source() | test.swift:126:15:126:15 | z | result |
1350+
| test.swift:132:15:132:15 | clean | test.swift:128:35:128:42 | call to source() | test.swift:132:15:132:15 | clean | result |
13411351
| test.swift:138:19:138:26 | call to source() | test.swift:138:19:138:26 | call to source() | test.swift:138:19:138:26 | call to source() | result |
13421352
| test.swift:145:15:145:31 | call to ... | test.swift:145:23:145:30 | call to source() | test.swift:145:15:145:31 | call to ... | result |
13431353
| test.swift:151:15:151:28 | call to ... | test.swift:149:16:149:23 | call to source() | test.swift:151:15:151:28 | call to ... | result |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
failures
21
testFailures
2+
failures

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,12 +1131,18 @@
11311131
| test.swift:888:9:888:9 | SSA def(stream) | test.swift:898:24:898:24 | stream |
11321132
| test.swift:888:9:888:9 | stream | test.swift:888:9:888:9 | SSA def(stream) |
11331133
| test.swift:888:18:896:6 | call to AsyncStream<Element>.init(_:bufferingPolicy:_:) | test.swift:888:9:888:9 | stream |
1134+
| test.swift:889:9:889:9 | continuation | test.swift:890:27:895:13 | continuation |
1135+
| test.swift:890:27:895:13 | { ... } | test.swift:891:17:891:17 | phi(this) |
11341136
| test.swift:891:17:891:17 | $generator | test.swift:891:17:891:17 | &... |
11351137
| test.swift:891:17:891:17 | &... | test.swift:891:17:891:17 | $generator |
11361138
| test.swift:891:17:891:17 | [post] $generator | test.swift:891:17:891:17 | &... |
1139+
| test.swift:891:17:891:17 | phi(this) | test.swift:892:21:892:21 | this |
1140+
| test.swift:891:17:891:17 | phi(this) | test.swift:894:17:894:17 | this |
11371141
| test.swift:891:26:891:26 | $generator | test.swift:891:26:891:26 | SSA def($generator) |
11381142
| test.swift:891:26:891:26 | SSA def($generator) | test.swift:891:17:891:17 | $generator |
11391143
| test.swift:891:26:891:30 | call to makeIterator() | test.swift:891:26:891:26 | $generator |
1144+
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
1145+
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
11401146
| test.swift:898:5:898:5 | $i$generator | test.swift:898:5:898:5 | &... |
11411147
| test.swift:898:5:898:5 | &... | test.swift:898:5:898:5 | $i$generator |
11421148
| test.swift:898:5:898:5 | [post] $i$generator | test.swift:898:5:898:5 | &... |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func forwarder() {
129129
(i: Int) -> Int in
130130
return 0
131131
})
132-
sink(arg: clean)
132+
sink(arg: clean) // $ SPURIOUS: flow=128
133133
}
134134

135135
func lambdaFlows() {

‎swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
failures
21
testFailures
2+
failures

0 commit comments

Comments
 (0)