Skip to content

Commit 5c9942f

Browse files
asgerfasgerf
committed
unified: Propagate receiver argument into accessor calls
1 parent 34de910 commit 5c9942f

4 files changed

Lines changed: 49 additions & 4 deletions

File tree

‎unified/ql/lib/codeql/unified/internal/dataflow/DataFlowGraph.qll‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,17 @@ predicate step(BuilderNode node1, Step step, BuilderNode node2) {
9898
(node1.isIncomingValue(access) or node1.isPostUpdate(access)) and
9999
step.storeName(access.getMemberName()) and
100100
node2.isLocalVariablePostUpdate(access, access.getImplicitQualifierVariable())
101+
or
102+
// Pass the receiver along to the getter/setter invoked by this expression
103+
exists(DataFlowCall call | access = [call.asGetterCall(), call.asSetterCall()] |
104+
node1.isLocalVariableRead(access, access.getImplicitQualifierVariable()) and
105+
step.value() and
106+
node2.isReceiverArgumentEx(call)
107+
or
108+
node1.isReceiverPostUpdateEx(call) and
109+
step.value() and
110+
node2.isLocalVariablePostUpdate(access, access.getImplicitQualifierVariable())
111+
)
101112
)
102113
or
103114
exists(StringInterpolationExpr expr |
@@ -124,6 +135,17 @@ predicate step(BuilderNode node1, Step step, BuilderNode node2) {
124135
(node1.isIncomingValue(expr) or node1.isPostUpdate(expr)) and
125136
step.storeName(expr.getMemberName()) and
126137
node2.isPostUpdate(expr.getBase())
138+
or
139+
// Pass the receiver along to the getter/setter invoked by this expression
140+
exists(DataFlowCall call | expr = [call.asGetterCall(), call.asSetterCall()] |
141+
node1.isResultValue(expr.getBase()) and
142+
step.value() and
143+
node2.isReceiverArgumentEx(call)
144+
or
145+
node1.isReceiverPostUpdateEx(call) and
146+
step.value() and
147+
node2.isPostUpdate(expr.getBase())
148+
)
127149
)
128150
or
129151
// Calls and constructor-patterns targeting an enum-case constructor.

‎unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,21 @@ class BuilderNode extends TDataFlowNodeStage1 {
186186
}
187187

188188
/** Holds if this node represents the receiver argument passed to `call`. */
189-
predicate isReceiverArgument(CallExpr call) {
190-
this.isImplicitArgument(toDataFlowCall(call), any(ArgumentPosition p | p.isReceiver()), false)
189+
predicate isReceiverArgumentEx(DataFlowCall call) {
190+
this.isImplicitArgument(call, any(ArgumentPosition p | p.isReceiver()), false)
191191
}
192192

193+
/** Holds if this node represents the receiver argument passed to `call`. */
194+
predicate isReceiverArgument(CallExpr call) { this.isReceiverArgumentEx(toDataFlowCall(call)) }
195+
193196
/** Holds if this node represents the updated state of the receiver of `call` after the call returns. */
194197
predicate isReceiverPostUpdate(CallExpr call) {
195-
this.isImplicitArgument(toDataFlowCall(call), any(ArgumentPosition p | p.isReceiver()), true)
198+
this.isReceiverPostUpdateEx(toDataFlowCall(call))
199+
}
200+
201+
/** Holds if this node represents the updated state of the receiver of `call` after the call returns. */
202+
predicate isReceiverPostUpdateEx(DataFlowCall call) {
203+
this.isImplicitArgument(call, any(ArgumentPosition p | p.isReceiver()), true)
196204
}
197205

198206
/** Holds if this is the canonical representative for the given `callable`. */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func t7() {
7171
var c = C()
7272
sink(c.x) // no flow
7373
c.field = source("t7.1")
74-
sink(c.x) // $ MISSING: hasValueFlow=t7.1
74+
sink(c.x) // $ hasValueFlow=t7.1
7575
}
7676

7777
func t8() {

‎unified/ql/test/library-tests/dataflow/test.expected‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ edges
1212
| accessors_instance.swift:52:11:52:13 | ... .x | accessors_instance.swift:52:5:52:7 | ... .x | provenance | |
1313
| accessors_instance.swift:57:25:57:38 | source(...) | accessors_instance.swift:60:18:60:23 | ... .x | provenance | |
1414
| accessors_instance.swift:57:25:57:38 | source(...) | accessors_instance.swift:61:18:61:18 | x | provenance | |
15+
| accessors_instance.swift:69:25:69:28 | self [field] | accessors_instance.swift:69:25:69:34 | ... .field | provenance | |
16+
| accessors_instance.swift:73:5:73:5 | [post] c [field] | accessors_instance.swift:74:10:74:10 | c [field] | provenance | |
17+
| accessors_instance.swift:73:5:73:11 | ... .field | accessors_instance.swift:73:5:73:5 | [post] c [field] | provenance | |
18+
| accessors_instance.swift:73:15:73:28 | source(...) | accessors_instance.swift:73:5:73:11 | ... .field | provenance | |
19+
| accessors_instance.swift:74:10:74:10 | c [field] | accessors_instance.swift:69:25:69:28 | self [field] | provenance | |
20+
| accessors_instance.swift:74:10:74:10 | c [field] | accessors_instance.swift:74:10:74:12 | ... .x | provenance | |
1521
| accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | provenance | |
1622
| accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | provenance | |
1723
| accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | provenance | |
@@ -518,6 +524,13 @@ nodes
518524
| accessors_instance.swift:57:25:57:38 | source(...) | semmle.label | source(...) |
519525
| accessors_instance.swift:60:18:60:23 | ... .x | semmle.label | ... .x |
520526
| accessors_instance.swift:61:18:61:18 | x | semmle.label | x |
527+
| accessors_instance.swift:69:25:69:28 | self [field] | semmle.label | self [field] |
528+
| accessors_instance.swift:69:25:69:34 | ... .field | semmle.label | ... .field |
529+
| accessors_instance.swift:73:5:73:5 | [post] c [field] | semmle.label | [post] c [field] |
530+
| accessors_instance.swift:73:5:73:11 | ... .field | semmle.label | ... .field |
531+
| accessors_instance.swift:73:15:73:28 | source(...) | semmle.label | source(...) |
532+
| accessors_instance.swift:74:10:74:10 | c [field] | semmle.label | c [field] |
533+
| accessors_instance.swift:74:10:74:12 | ... .x | semmle.label | ... .x |
521534
| accessors_local.swift:7:21:7:34 | source(...) | semmle.label | source(...) |
522535
| accessors_local.swift:8:10:8:10 | x | semmle.label | x |
523536
| accessors_local.swift:15:15:15:28 | source(...) | semmle.label | source(...) |
@@ -1119,6 +1132,7 @@ nodes
11191132
| test.swift:273:30:273:45 | source(...) | semmle.label | source(...) |
11201133
| test.swift:274:10:274:12 | f(...) | semmle.label | f(...) |
11211134
subpaths
1135+
| accessors_instance.swift:74:10:74:10 | c [field] | accessors_instance.swift:69:25:69:28 | self [field] | accessors_instance.swift:69:25:69:34 | ... .field | accessors_instance.swift:74:10:74:12 | ... .x |
11221136
| accessors_local.swift:45:9:45:9 | y | accessors_local.swift:46:21:46:21 | y | accessors_local.swift:46:21:46:21 | y | accessors_local.swift:47:10:47:10 | x |
11231137
| accessors_local.swift:54:5:54:5 | y | accessors_local.swift:52:21:52:21 | y | accessors_local.swift:52:21:52:21 | y | accessors_local.swift:55:10:55:10 | x |
11241138
| calls.swift:31:17:31:30 | source(...) | calls.swift:28:19:28:19 | x | calls.swift:29:16:29:24 | ... + ... | calls.swift:31:10:31:31 | target(...) |
@@ -1146,6 +1160,7 @@ testFailures
11461160
| accessors_instance.swift:48:24:48:31 | newValue | accessors_instance.swift:47:19:47:32 | source(...) | accessors_instance.swift:48:24:48:31 | newValue | $@ | accessors_instance.swift:47:19:47:32 | source(...) | source(...) |
11471161
| accessors_instance.swift:60:18:60:23 | ... .x | accessors_instance.swift:57:25:57:38 | source(...) | accessors_instance.swift:60:18:60:23 | ... .x | $@ | accessors_instance.swift:57:25:57:38 | source(...) | source(...) |
11481162
| accessors_instance.swift:61:18:61:18 | x | accessors_instance.swift:57:25:57:38 | source(...) | accessors_instance.swift:61:18:61:18 | x | $@ | accessors_instance.swift:57:25:57:38 | source(...) | source(...) |
1163+
| accessors_instance.swift:74:10:74:12 | ... .x | accessors_instance.swift:73:15:73:28 | source(...) | accessors_instance.swift:74:10:74:12 | ... .x | $@ | accessors_instance.swift:73:15:73:28 | source(...) | source(...) |
11491164
| accessors_local.swift:8:10:8:10 | x | accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | $@ | accessors_local.swift:7:21:7:34 | source(...) | source(...) |
11501165
| accessors_local.swift:17:10:17:10 | x | accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | $@ | accessors_local.swift:15:15:15:28 | source(...) | source(...) |
11511166
| accessors_local.swift:25:10:25:10 | x | accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | $@ | accessors_local.swift:22:15:22:28 | source(...) | source(...) |

0 commit comments

Comments
 (0)