Skip to content

Commit 14468b6

Browse files
MathiasVPMathiasVP
authored
Merge pull request #11924 from atorralba/atorralba/optbinding-getters
Swift: Support more CFG node types in optional binding flow
2 parents 5173f10 + 90517e2 commit 14468b6

File tree

5 files changed

+91
-25
lines changed

5 files changed

+91
-25
lines changed

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

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -91,20 +91,21 @@ class CfgNode extends ControlFlowNode, TElementNode {
9191

9292
/** Gets a split for this control flow node, if any. */
9393
final Split getASplit() { result = splits.getASplit() }
94-
}
9594

96-
private Expr getAst(ControlFlowElement n) {
97-
result = n.asAstNode()
98-
or
99-
result = n.(PropertyGetterElement).getRef()
100-
or
101-
result = n.(PropertySetterElement).getAssignExpr()
102-
or
103-
result = n.(PropertyObserverElement).getAssignExpr()
104-
or
105-
result = n.(ClosureElement).getAst()
106-
or
107-
result = n.(KeyPathElement).getAst()
95+
/** Gets the AST representation of this control flow node, if any. */
96+
Expr getAst() {
97+
result = n.asAstNode()
98+
or
99+
result = n.(PropertyGetterElement).getRef()
100+
or
101+
result = n.(PropertySetterElement).getAssignExpr()
102+
or
103+
result = n.(PropertyObserverElement).getAssignExpr()
104+
or
105+
result = n.(ClosureElement).getAst()
106+
or
107+
result = n.(KeyPathElement).getAst()
108+
}
108109
}
109110

110111
/** A control-flow node that wraps an AST expression. */
@@ -123,7 +124,7 @@ class PropertyGetterCfgNode extends CfgNode {
123124

124125
Expr getRef() { result = n.getRef() }
125126

126-
CfgNode getBase() { getAst(result.getNode()) = n.getBase() }
127+
CfgNode getBase() { result.getAst() = n.getBase() }
127128

128129
AccessorDecl getAccessorDecl() { result = n.getAccessorDecl() }
129130
}
@@ -134,9 +135,9 @@ class PropertySetterCfgNode extends CfgNode {
134135

135136
AssignExpr getAssignExpr() { result = n.getAssignExpr() }
136137

137-
CfgNode getBase() { getAst(result.getNode()) = n.getBase() }
138+
CfgNode getBase() { result.getAst() = n.getBase() }
138139

139-
CfgNode getSource() { getAst(result.getNode()) = n.getAssignExpr().getSource() }
140+
CfgNode getSource() { result.getAst() = n.getAssignExpr().getSource() }
140141

141142
AccessorDecl getAccessorDecl() { result = n.getAccessorDecl() }
142143
}
@@ -146,19 +147,19 @@ class PropertyObserverCfgNode extends CfgNode {
146147

147148
AssignExpr getAssignExpr() { result = n.getAssignExpr() }
148149

149-
CfgNode getBase() { getAst(result.getNode()) = n.getBase() }
150+
CfgNode getBase() { result.getAst() = n.getBase() }
150151

151-
CfgNode getSource() { getAst(result.getNode()) = n.getAssignExpr().getSource() }
152+
CfgNode getSource() { result.getAst() = n.getAssignExpr().getSource() }
152153

153154
AccessorDecl getAccessorDecl() { result = n.getObserver() }
154155
}
155156

156157
class ApplyExprCfgNode extends ExprCfgNode {
157158
override ApplyExpr e;
158159

159-
CfgNode getArgument(int index) { getAst(result.getNode()) = e.getArgument(index).getExpr() }
160+
CfgNode getArgument(int index) { result.getAst() = e.getArgument(index).getExpr() }
160161

161-
CfgNode getQualifier() { getAst(result.getNode()) = e.getQualifier() }
162+
CfgNode getQualifier() { result.getAst() = e.getQualifier() }
162163

163164
AbstractFunctionDecl getStaticTarget() { result = e.getStaticTarget() }
164165

‎swift/ql/lib/codeql/swift/dataflow/Ssa.qll

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,7 @@ module Ssa {
160160
pbd.getAPattern() = bb.getNode(blockIndex).getNode().asAstNode() and
161161
init = var.getParentInitializer()
162162
|
163-
value.getNode().asAstNode() = init
164-
or
165-
// TODO: We should probably enumerate more cfg nodes here.
166-
value.(PropertyGetterCfgNode).getRef() = init
163+
value.getAst() = init
167164
)
168165
or
169166
exists(SsaInput::BasicBlock bb, int blockIndex, ConditionElement ce, Expr init |
@@ -172,7 +169,7 @@ module Ssa {
172169
init = ce.getInitializer() and
173170
strictcount(Ssa::WriteDefinition alt | alt.definesAt(_, bb, blockIndex)) = 1 // exclude cases where there are multiple writes from the same pattern, this is at best taint flow.
174171
|
175-
value.getNode().asAstNode() = init
172+
value.getAst() = init
176173
)
177174
}
178175
}

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ edges
22
| file://:0:0:0:0 | [summary param] this in signum() : | file://:0:0:0:0 | [summary] to write: return (return) in signum() : |
33
| file://:0:0:0:0 | self [a, x] : | file://:0:0:0:0 | .a [x] : |
44
| file://:0:0:0:0 | self [x] : | file://:0:0:0:0 | .x : |
5+
| file://:0:0:0:0 | self [x] : | file://:0:0:0:0 | .x : |
6+
| file://:0:0:0:0 | value : | file://:0:0:0:0 | [post] self [x] : |
57
| file://:0:0:0:0 | value : | file://:0:0:0:0 | [post] self [x] : |
68
| test.swift:6:19:6:26 | call to source() : | test.swift:7:15:7:15 | t1 |
79
| test.swift:6:19:6:26 | call to source() : | test.swift:9:15:9:15 | t1 |
@@ -101,6 +103,7 @@ edges
101103
| test.swift:225:14:225:21 | call to source() : | test.swift:238:13:238:15 | .source_value |
102104
| test.swift:259:12:259:19 | call to source() : | test.swift:263:13:263:28 | call to optionalSource() : |
103105
| test.swift:259:12:259:19 | call to source() : | test.swift:439:13:439:28 | call to optionalSource() : |
106+
| test.swift:259:12:259:19 | call to source() : | test.swift:466:13:466:28 | call to optionalSource() : |
104107
| test.swift:263:13:263:28 | call to optionalSource() : | test.swift:265:15:265:15 | x |
105108
| test.swift:263:13:263:28 | call to optionalSource() : | test.swift:267:15:267:16 | ...! |
106109
| test.swift:263:13:263:28 | call to optionalSource() : | test.swift:271:15:271:16 | ...? : |
@@ -142,14 +145,27 @@ edges
142145
| test.swift:360:15:360:15 | t2 [Tuple element at index 0] : | test.swift:360:15:360:18 | .0 |
143146
| test.swift:361:15:361:15 | t2 [Tuple element at index 1] : | test.swift:361:15:361:18 | .1 |
144147
| test.swift:439:13:439:28 | call to optionalSource() : | test.swift:442:19:442:19 | a |
148+
| test.swift:462:9:462:9 | self [x] : | file://:0:0:0:0 | self [x] : |
149+
| test.swift:462:9:462:9 | value : | file://:0:0:0:0 | value : |
150+
| test.swift:466:13:466:28 | call to optionalSource() : | test.swift:468:12:468:12 | x : |
151+
| test.swift:468:5:468:5 | [post] cx [x] : | test.swift:472:20:472:20 | cx [x] : |
152+
| test.swift:468:12:468:12 | x : | test.swift:462:9:462:9 | value : |
153+
| test.swift:468:12:468:12 | x : | test.swift:468:5:468:5 | [post] cx [x] : |
154+
| test.swift:472:20:472:20 | cx [x] : | test.swift:462:9:462:9 | self [x] : |
155+
| test.swift:472:20:472:20 | cx [x] : | test.swift:472:20:472:23 | .x : |
156+
| test.swift:472:20:472:23 | .x : | test.swift:473:15:473:15 | z1 |
145157
nodes
146158
| file://:0:0:0:0 | .a [x] : | semmle.label | .a [x] : |
147159
| file://:0:0:0:0 | .x : | semmle.label | .x : |
160+
| file://:0:0:0:0 | .x : | semmle.label | .x : |
161+
| file://:0:0:0:0 | [post] self [x] : | semmle.label | [post] self [x] : |
148162
| file://:0:0:0:0 | [post] self [x] : | semmle.label | [post] self [x] : |
149163
| file://:0:0:0:0 | [summary param] this in signum() : | semmle.label | [summary param] this in signum() : |
150164
| file://:0:0:0:0 | [summary] to write: return (return) in signum() : | semmle.label | [summary] to write: return (return) in signum() : |
151165
| file://:0:0:0:0 | self [a, x] : | semmle.label | self [a, x] : |
152166
| file://:0:0:0:0 | self [x] : | semmle.label | self [x] : |
167+
| file://:0:0:0:0 | self [x] : | semmle.label | self [x] : |
168+
| file://:0:0:0:0 | value : | semmle.label | value : |
153169
| file://:0:0:0:0 | value : | semmle.label | value : |
154170
| test.swift:6:19:6:26 | call to source() : | semmle.label | call to source() : |
155171
| test.swift:7:15:7:15 | t1 | semmle.label | t1 |
@@ -300,6 +316,14 @@ nodes
300316
| test.swift:361:15:361:18 | .1 | semmle.label | .1 |
301317
| test.swift:439:13:439:28 | call to optionalSource() : | semmle.label | call to optionalSource() : |
302318
| test.swift:442:19:442:19 | a | semmle.label | a |
319+
| test.swift:462:9:462:9 | self [x] : | semmle.label | self [x] : |
320+
| test.swift:462:9:462:9 | value : | semmle.label | value : |
321+
| test.swift:466:13:466:28 | call to optionalSource() : | semmle.label | call to optionalSource() : |
322+
| test.swift:468:5:468:5 | [post] cx [x] : | semmle.label | [post] cx [x] : |
323+
| test.swift:468:12:468:12 | x : | semmle.label | x : |
324+
| test.swift:472:20:472:20 | cx [x] : | semmle.label | cx [x] : |
325+
| test.swift:472:20:472:23 | .x : | semmle.label | .x : |
326+
| test.swift:473:15:473:15 | z1 | semmle.label | z1 |
303327
subpaths
304328
| test.swift:75:21:75:22 | &... : | test.swift:65:16:65:28 | arg1 : | test.swift:65:1:70:1 | arg2[return] : | test.swift:75:31:75:32 | [post] &... : |
305329
| 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 ... : |
@@ -330,6 +354,8 @@ subpaths
330354
| test.swift:271:15:271:16 | ...? : | file://:0:0:0:0 | [summary param] this in signum() : | file://:0:0:0:0 | [summary] to write: return (return) in signum() : | test.swift:271:15:271:25 | call to signum() : |
331355
| test.swift:291:16:291:17 | ...? : | file://:0:0:0:0 | [summary param] this in signum() : | file://:0:0:0:0 | [summary] to write: return (return) in signum() : | test.swift:291:16:291:26 | call to signum() : |
332356
| test.swift:303:15:303:16 | ...! : | file://:0:0:0:0 | [summary param] this in signum() : | file://:0:0:0:0 | [summary] to write: return (return) in signum() : | test.swift:303:15:303:25 | call to signum() |
357+
| test.swift:468:12:468:12 | x : | test.swift:462:9:462:9 | value : | file://:0:0:0:0 | [post] self [x] : | test.swift:468:5:468:5 | [post] cx [x] : |
358+
| test.swift:472:20:472:20 | cx [x] : | test.swift:462:9:462:9 | self [x] : | file://:0:0:0:0 | .x : | test.swift:472:20:472:23 | .x : |
333359
#select
334360
| test.swift:7:15:7:15 | t1 | test.swift:6:19:6:26 | call to source() : | test.swift:7:15:7:15 | t1 | result |
335361
| test.swift:9:15:9:15 | t1 | test.swift:6:19:6:26 | call to source() : | test.swift:9:15:9:15 | t1 | result |
@@ -381,3 +407,4 @@ subpaths
381407
| test.swift:360:15:360:18 | .0 | test.swift:351:18:351:25 | call to source() : | test.swift:360:15:360:18 | .0 | result |
382408
| test.swift:361:15:361:18 | .1 | test.swift:351:31:351:38 | call to source() : | test.swift:361:15:361:18 | .1 | result |
383409
| test.swift:442:19:442:19 | a | test.swift:259:12:259:19 | call to source() : | test.swift:442:19:442:19 | a | result |
410+
| test.swift:473:15:473:15 | z1 | test.swift:259:12:259:19 | call to source() : | test.swift:473:15:473:15 | z1 | result |

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,3 +366,27 @@
366366
| test.swift:448:10:448:37 | SSA def(b) | test.swift:450:19:450:19 | b |
367367
| test.swift:455:8:455:17 | SSA def(x) | test.swift:456:19:456:19 | x |
368368
| test.swift:455:8:455:17 | SSA def(y) | test.swift:457:19:457:19 | y |
369+
| test.swift:461:7:461:7 | SSA def(self) | test.swift:461:7:461:7 | self[return] |
370+
| test.swift:461:7:461:7 | SSA def(self) | test.swift:461:7:461:7 | self[return] |
371+
| test.swift:461:7:461:7 | self | test.swift:461:7:461:7 | SSA def(self) |
372+
| test.swift:461:7:461:7 | self | test.swift:461:7:461:7 | SSA def(self) |
373+
| test.swift:462:9:462:9 | self | test.swift:462:9:462:9 | SSA def(self) |
374+
| test.swift:462:9:462:9 | self | test.swift:462:9:462:9 | SSA def(self) |
375+
| test.swift:462:9:462:9 | self | test.swift:462:9:462:9 | SSA def(self) |
376+
| test.swift:462:9:462:9 | value | test.swift:462:9:462:9 | SSA def(value) |
377+
| test.swift:465:33:465:39 | SSA def(y) | test.swift:470:12:470:12 | y |
378+
| test.swift:465:33:465:39 | y | test.swift:465:33:465:39 | SSA def(y) |
379+
| test.swift:466:9:466:9 | SSA def(x) | test.swift:468:12:468:12 | x |
380+
| test.swift:466:13:466:28 | call to optionalSource() | test.swift:466:9:466:9 | SSA def(x) |
381+
| test.swift:467:9:467:9 | SSA def(cx) | test.swift:468:5:468:5 | cx |
382+
| test.swift:467:14:467:16 | call to C.init() | test.swift:467:9:467:9 | SSA def(cx) |
383+
| test.swift:468:5:468:5 | [post] cx | test.swift:472:20:472:20 | cx |
384+
| test.swift:468:5:468:5 | cx | test.swift:472:20:472:20 | cx |
385+
| test.swift:469:9:469:9 | SSA def(cy) | test.swift:470:5:470:5 | cy |
386+
| test.swift:469:14:469:16 | call to C.init() | test.swift:469:9:469:9 | SSA def(cy) |
387+
| test.swift:470:5:470:5 | [post] cy | test.swift:474:20:474:20 | cy |
388+
| test.swift:470:5:470:5 | cy | test.swift:474:20:474:20 | cy |
389+
| test.swift:472:11:472:15 | SSA def(z1) | test.swift:473:15:473:15 | z1 |
390+
| test.swift:472:20:472:23 | .x | test.swift:472:11:472:15 | SSA def(z1) |
391+
| test.swift:474:11:474:15 | SSA def(z2) | test.swift:475:15:475:15 | z2 |
392+
| test.swift:474:20:474:23 | .x | test.swift:474:11:474:15 | SSA def(z2) |

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,3 +457,20 @@ func testOptionals2(y: Int?) {
457457
sink(arg: y) // (taint but not data flow)
458458
}
459459
}
460+
461+
class C {
462+
var x: Int?
463+
}
464+
465+
func testOptionalPropertyAccess(y: Int?) {
466+
let x = optionalSource()
467+
let cx = C()
468+
cx.x = x
469+
let cy = C()
470+
cy.x = y
471+
472+
guard let z1 = cx.x else { return }
473+
sink(arg: z1) // $ flow=259
474+
guard let z2 = cy.x else { return }
475+
sink(arg: z2)
476+
}

0 commit comments

Comments
 (0)