Skip to content

Commit ed70ef4

Browse files
asgerfasgerf
committed
unified: Support setter calls from writebacks
If `p` is a getter/setter pair for a value type, then `p.f = x` ends with a call to p's setter with the updated value (called "writebacks" in Swift).
1 parent 4a510f7 commit ed70ef4

8 files changed

Lines changed: 102 additions & 12 deletions

File tree

‎unified/ql/consistency-queries/DataFlowConsistency.ql‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ module ConsistencyInput implements InputSig<Location, DataFlowInput> {
66
predicate argHasPostUpdateExclude(DataFlowInput::ArgumentNode n) {
77
not exists(n.getBasicBlock()) // ignore unreachable data flow nodes
88
or
9-
// The RHS value passed into a setter call technically needs a post-update node
9+
// The value passed into a setter call technically needs a post-update node
1010
// because it's an argument to a call, but this rarely matters in practice.
11-
n.isIncomingValue(any(DataFlowCall c).asSetterCall())
11+
n.(BuilderNode).isSetterValueArgument(_)
1212
}
1313

1414
predicate reverseReadExclude(DataFlow::Node n) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ module CaptureSsaInput implements InputSig<Location, BasicBlock> {
9393
private predicate isEscape(BuilderNode node) {
9494
node.isResultValue(any(Argument arg).getValue())
9595
or
96-
node.isIncomingValue(any(DataFlowCall call).asSetterCall())
96+
node.isSetterValueArgument(_)
9797
or
9898
node.isImplicitArgument(_, _, _)
9999
or

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,12 @@ private newtype TDataFlowCall =
1616
)
1717
} or
1818
TSetterCall(Expr expr) {
19-
isInAssignmentContext(expr, _) and
19+
(
20+
isInAssignmentContext(expr, _)
21+
or
22+
// Write back a value that was mutated in-place, such as `x.p` in `x.p.f = y`
23+
isMutatedByCallOrAssign(expr, _)
24+
) and
2025
(
2126
N::getStaticBindingTargetFromRef(expr) = any(SetterDeclaration g).getNameNode()
2227
or

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -107,11 +107,8 @@ module DataFlowInput implements InputSig<Location> {
107107
or
108108
n.(BuilderNode).isImplicitArgument(call, pos, false)
109109
or
110-
exists(Expr access |
111-
call.asSetterCall() = access and
112-
n.isIncomingValue(access) and
113-
pos.asPositional() = 0
114-
)
110+
n.(BuilderNode).isSetterValueArgument(call) and
111+
pos.asPositional() = 0
115112
or
116113
exists(FlowSummaryOutput::SummaryNode callee |
117114
FlowSummaryOutput::summaryArgumentNode(callee, getSummaryNode(n), pos) and

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

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@ private predicate hasPostUpdateAfterCallOrAssign(Expr expr, ControlFlowNode cfgN
4646
)
4747
}
4848

49+
/**
50+
* Holds if the value of `expr` is mutated at `cfgNode` by a surrounding call or assignment,
51+
* and thus needs to be written back if `expr` refers to a computed value.
52+
*/
53+
predicate isMutatedByCallOrAssign(Expr expr, ControlFlowNode cfgNode) {
54+
hasPostUpdateAfterCallOrAssign(expr, cfgNode)
55+
}
56+
4957
/**
5058
* Holds if `expr` has a post-update at `cfgNode`.
5159
*/
@@ -178,6 +186,22 @@ class BuilderNode extends TDataFlowNodeStage1 {
178186
/** Holds if this represents the updated state of the value returned by `expr` after it has been mutated by the surrounding assignment or call. */
179187
predicate isPostUpdate(Expr expr) { this = TExprPostUpdateNode(expr) }
180188

189+
/**
190+
* Holds if this node represents the new value passed to the setter invoked by `call`.
191+
*
192+
* For a direct assignment, such as `x.p = y`, this is the incoming value of `x.p`.
193+
* When the value of a computed property is mutated in place, such as `x.p.f = y`,
194+
* this is the post-update node of `x.p`, as the mutated value is written back via the setter.
195+
*/
196+
predicate isSetterValueArgument(DataFlowCall call) {
197+
exists(Expr access | access = call.asSetterCall() |
198+
this.isIncomingValue(access)
199+
or
200+
not isInAssignmentContext(access, _) and
201+
this.isPostUpdate(access)
202+
)
203+
}
204+
181205
/** Holds if this node is the canonical representative for the return value of `callable`. */
182206
predicate isReturnNode(Callable callable) { this.isReturnNodeEx(toDataFlowCallable(callable)) }
183207

@@ -387,10 +411,16 @@ class BuilderNode extends TDataFlowNodeStage1 {
387411
)
388412
or
389413
access = call.asSetterCall() and
390-
exists(Assignment assignment | isInAssignmentContext(access, assignment) |
391-
isPost = false and cfgNode.injects(assignment)
414+
(
415+
exists(Assignment assignment | isInAssignmentContext(access, assignment) |
416+
isPost = false and cfgNode.injects(assignment)
417+
or
418+
isPost = true and cfgNode.isAfter(assignment)
419+
)
392420
or
393-
isPost = true and cfgNode.isAfter(assignment)
421+
// Setter invoked to write back a mutated value
422+
not isInAssignmentContext(access, _) and
423+
isMutatedByCallOrAssign(access, cfgNode)
394424
)
395425
)
396426
)

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,29 @@ func t10() {
126126
}
127127
}
128128
}
129+
130+
func t11() {
131+
class C {
132+
var p: (String, String) {
133+
get { ("safe", "safe") }
134+
set { sink(newValue.0) } // $ hasValueFlow=t11.1
135+
}
136+
}
137+
let c = C()
138+
c.p.0 = source("t11.1")
139+
}
140+
141+
func t12() {
142+
struct S {
143+
var field: String = "safe"
144+
mutating func update() { field = source("t12.1") }
145+
}
146+
class C {
147+
var s: S {
148+
get { S() }
149+
set { sink(newValue.field) } // $ MISSING: hasValueFlow=t12.1
150+
}
151+
}
152+
let c = C()
153+
c.s.update() // MISSING: the call to 'update' is not resolved
154+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,11 @@ func t10() {
8484
y = source("t9.1")
8585
closure()
8686
}
87+
88+
func t11() {
89+
var p: (String, String) {
90+
get { ("safe", "safe") }
91+
set { sink(newValue.0) } // $ hasValueFlow=t11.1
92+
}
93+
p.0 = source("t11.1")
94+
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ edges
3333
| accessors_instance.swift:112:10:112:10 | c [field] | accessors_instance.swift:112:10:112:16 | ... .field | provenance | |
3434
| accessors_instance.swift:119:13:119:17 | field | accessors_instance.swift:125:18:125:22 | field | provenance | |
3535
| accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:119:13:119:17 | field | provenance | |
36+
| accessors_instance.swift:134:13:134:15 | newValue [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
37+
| accessors_instance.swift:134:24:134:31 | newValue [0] | accessors_instance.swift:134:24:134:33 | ... .0 | provenance | |
38+
| accessors_instance.swift:138:5:138:7 | [post] ... .p [0] | accessors_instance.swift:134:13:134:15 | newValue [0] | provenance | |
39+
| accessors_instance.swift:138:5:138:9 | ... .0 | accessors_instance.swift:138:5:138:7 | [post] ... .p [0] | provenance | |
40+
| accessors_instance.swift:138:13:138:27 | source(...) | accessors_instance.swift:138:5:138:9 | ... .0 | provenance | |
3641
| accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | provenance | |
3742
| accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | provenance | |
3843
| accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | provenance | |
@@ -54,6 +59,11 @@ edges
5459
| accessors_local.swift:72:9:72:22 | source(...) | accessors_local.swift:72:5:72:5 | y | provenance | |
5560
| accessors_local.swift:84:5:84:5 | y | accessors_local.swift:82:14:82:15 | x3 | provenance | |
5661
| accessors_local.swift:84:9:84:22 | source(...) | accessors_local.swift:84:5:84:5 | y | provenance | |
62+
| accessors_local.swift:91:9:91:11 | newValue [0] | accessors_local.swift:91:20:91:27 | newValue [0] | provenance | |
63+
| accessors_local.swift:91:20:91:27 | newValue [0] | accessors_local.swift:91:20:91:29 | ... .0 | provenance | |
64+
| accessors_local.swift:93:5:93:5 | [post] p [0] | accessors_local.swift:91:9:91:11 | newValue [0] | provenance | |
65+
| accessors_local.swift:93:5:93:7 | ... .0 | accessors_local.swift:93:5:93:5 | [post] p [0] | provenance | |
66+
| accessors_local.swift:93:11:93:25 | source(...) | accessors_local.swift:93:5:93:7 | ... .0 | provenance | |
5767
| accessors_static.swift:8:32:8:45 | source(...) | accessors_static.swift:10:10:10:12 | ... .x | provenance | |
5868
| accessors_static.swift:18:19:18:32 | source(...) | accessors_static.swift:21:10:21:12 | ... .x | provenance | |
5969
| accessors_static.swift:27:19:27:32 | source(...) | accessors_static.swift:31:10:31:12 | ... .x | provenance | |
@@ -556,6 +566,12 @@ nodes
556566
| accessors_instance.swift:119:13:119:17 | field | semmle.label | field |
557567
| accessors_instance.swift:119:21:119:35 | source(...) | semmle.label | source(...) |
558568
| accessors_instance.swift:125:18:125:22 | field | semmle.label | field |
569+
| accessors_instance.swift:134:13:134:15 | newValue [0] | semmle.label | newValue [0] |
570+
| accessors_instance.swift:134:24:134:31 | newValue [0] | semmle.label | newValue [0] |
571+
| accessors_instance.swift:134:24:134:33 | ... .0 | semmle.label | ... .0 |
572+
| accessors_instance.swift:138:5:138:7 | [post] ... .p [0] | semmle.label | [post] ... .p [0] |
573+
| accessors_instance.swift:138:5:138:9 | ... .0 | semmle.label | ... .0 |
574+
| accessors_instance.swift:138:13:138:27 | source(...) | semmle.label | source(...) |
559575
| accessors_local.swift:7:21:7:34 | source(...) | semmle.label | source(...) |
560576
| accessors_local.swift:8:10:8:10 | x | semmle.label | x |
561577
| accessors_local.swift:15:15:15:28 | source(...) | semmle.label | source(...) |
@@ -587,6 +603,12 @@ nodes
587603
| accessors_local.swift:82:14:82:15 | x3 | semmle.label | x3 |
588604
| accessors_local.swift:84:5:84:5 | y | semmle.label | y |
589605
| accessors_local.swift:84:9:84:22 | source(...) | semmle.label | source(...) |
606+
| accessors_local.swift:91:9:91:11 | newValue [0] | semmle.label | newValue [0] |
607+
| accessors_local.swift:91:20:91:27 | newValue [0] | semmle.label | newValue [0] |
608+
| accessors_local.swift:91:20:91:29 | ... .0 | semmle.label | ... .0 |
609+
| accessors_local.swift:93:5:93:5 | [post] p [0] | semmle.label | [post] p [0] |
610+
| accessors_local.swift:93:5:93:7 | ... .0 | semmle.label | ... .0 |
611+
| accessors_local.swift:93:11:93:25 | source(...) | semmle.label | source(...) |
590612
| accessors_static.swift:8:32:8:45 | source(...) | semmle.label | source(...) |
591613
| accessors_static.swift:10:10:10:12 | ... .x | semmle.label | ... .x |
592614
| accessors_static.swift:18:19:18:32 | source(...) | semmle.label | source(...) |
@@ -1185,6 +1207,7 @@ testFailures
11851207
| accessors_instance.swift:99:10:99:21 | ... .otherField | accessors_instance.swift:97:15:97:28 | source(...) | accessors_instance.swift:99:10:99:21 | ... .otherField | $@ | accessors_instance.swift:97:15:97:28 | source(...) | source(...) |
11861208
| accessors_instance.swift:112:10:112:16 | ... .field | accessors_instance.swift:106:21:106:34 | source(...) | accessors_instance.swift:112:10:112:16 | ... .field | $@ | accessors_instance.swift:106:21:106:34 | source(...) | source(...) |
11871209
| accessors_instance.swift:125:18:125:22 | field | accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:125:18:125:22 | field | $@ | accessors_instance.swift:119:21:119:35 | source(...) | source(...) |
1210+
| accessors_instance.swift:134:24:134:33 | ... .0 | accessors_instance.swift:138:13:138:27 | source(...) | accessors_instance.swift:134:24:134:33 | ... .0 | $@ | accessors_instance.swift:138:13:138:27 | source(...) | source(...) |
11881211
| 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(...) |
11891212
| 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(...) |
11901213
| 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(...) |
@@ -1195,6 +1218,7 @@ testFailures
11951218
| accessors_local.swift:61:14:61:14 | x | accessors_local.swift:59:21:59:34 | source(...) | accessors_local.swift:61:14:61:14 | x | $@ | accessors_local.swift:59:21:59:34 | source(...) | source(...) |
11961219
| accessors_local.swift:70:14:70:14 | x | accessors_local.swift:72:9:72:22 | source(...) | accessors_local.swift:70:14:70:14 | x | $@ | accessors_local.swift:72:9:72:22 | source(...) | source(...) |
11971220
| accessors_local.swift:82:14:82:15 | x3 | accessors_local.swift:84:9:84:22 | source(...) | accessors_local.swift:82:14:82:15 | x3 | $@ | accessors_local.swift:84:9:84:22 | source(...) | source(...) |
1221+
| accessors_local.swift:91:20:91:29 | ... .0 | accessors_local.swift:93:11:93:25 | source(...) | accessors_local.swift:91:20:91:29 | ... .0 | $@ | accessors_local.swift:93:11:93:25 | source(...) | source(...) |
11981222
| accessors_static.swift:10:10:10:12 | ... .x | accessors_static.swift:8:32:8:45 | source(...) | accessors_static.swift:10:10:10:12 | ... .x | $@ | accessors_static.swift:8:32:8:45 | source(...) | source(...) |
11991223
| accessors_static.swift:21:10:21:12 | ... .x | accessors_static.swift:18:19:18:32 | source(...) | accessors_static.swift:21:10:21:12 | ... .x | $@ | accessors_static.swift:18:19:18:32 | source(...) | source(...) |
12001224
| accessors_static.swift:31:10:31:12 | ... .x | accessors_static.swift:27:19:27:32 | source(...) | accessors_static.swift:31:10:31:12 | ... .x | $@ | accessors_static.swift:27:19:27:32 | source(...) | source(...) |

0 commit comments

Comments
 (0)