Skip to content

Commit a4e9ee7

Browse files
asgerfasgerf
committed
unified: Sharpen CFG node for post-updates
1 parent 3ef3122 commit a4e9ee7

5 files changed

Lines changed: 31 additions & 18 deletions

File tree

‎shared/ssa/codeql/ssa/Ssa.qll‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1419,6 +1419,22 @@ module Make<
14191419
*/
14201420
default predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { none() }
14211421

1422+
/**
1423+
* Holds if the post-update node corresponding to the given `read` occurs at `bb,i`, meaning
1424+
* it will propagate to the next use (strictly) after that point in the CFG.
1425+
*
1426+
* The default is to use the CFG node associated with the read itself, meaning the post-update
1427+
* always flows to the next use. The default can however lead to spurious flow in cases like:
1428+
* ```
1429+
* x.f = foo(x)
1430+
* ```
1431+
* where the post-update node for `x` on the left-hand side flows into the `x` on the right-hand side.
1432+
*
1433+
* NOTE: When implementing this predicate, you must ensure that `variableRead` is defined to contain
1434+
* a synthetic read of this variable at `bb,i`.
1435+
*/
1436+
default predicate postUpdateCfgNode(Expr read, BasicBlock bb, int i) { read.hasCfgNode(bb, i) }
1437+
14221438
/** An abstract value that a `Guard` may evaluate to. */
14231439
class GuardValue {
14241440
/** Gets a textual representation of this value. */
@@ -1910,7 +1926,7 @@ module Make<
19101926
nodeFrom.(ReadNode).readsAt(bb, i, v) and
19111927
isUseStep = true
19121928
or
1913-
nodeFrom.(ExprPostUpdateNode).getPreUpdateNode().(ReadNode).readsAt(bb, i, v) and
1929+
DfInput::postUpdateCfgNode(nodeFrom.(ExprPostUpdateNode).getExpr(), bb, i) and
19141930
isUseStep = true
19151931
}
19161932

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ predicate performsVariableAccess(
2727
or
2828
hasIncomingValueAtCfgNode(access, cfgNode) and kind.isWrite()
2929
or
30-
// TODO: Make the SSA library use this CFG node for post-updates. It currently picks the same as the read.
3130
hasPostUpdate(access, cfgNode) and kind.isPostUpdate()
3231
)
3332
or

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ module LocalSsaInput implements InputSig<Location, BasicBlock> {
2727
predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
2828
certain = true and
2929
performsVariableAccess(_, v, TRead(), bb.getNode(i))
30+
or
31+
certain = true and
32+
performsVariableAccess(_, v, TPostUpdate(), bb.getNode(i))
3033
}
3134
}
3235

@@ -58,6 +61,13 @@ module LocalSsaDataFlowInput implements DataFlowIntegrationInputSig {
5861
}
5962

6063
predicate guardDirectlyControlsBlock(Guard guard, BasicBlock bb, GuardValue val) { none() }
64+
65+
predicate postUpdateCfgNode(Expr read, BasicBlock bb, int i) {
66+
exists(LocalVariable var, U::Expr expr |
67+
read = TLocalVariableRefNode(expr, var, TRead()) and
68+
performsVariableAccess(expr, var, TPostUpdate(), bb.getNode(i))
69+
)
70+
}
6171
}
6272

6373
module LocalSsaDataFlowOutput = DataFlowIntegration<LocalSsaDataFlowInput>;

‎unified/ql/test/library-tests/dataflow/implicit-self.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,28 +56,28 @@ class C {
5656

5757
func t9() {
5858
x = "safe";
59-
x += sink(x) + source("t9.1"); // $ SPURIOUS: hasTaintFlow=t9.1
59+
x += sink(x) + source("t9.1");
6060
sink(x); // $ hasTaintFlow=t9.1
6161
sink(self.x); // $ hasTaintFlow=t9.1
6262
}
6363

6464
func t10() {
6565
x = "safe";
66-
self.x += sink(x) + source("t10.1"); // $ SPURIOUS: hasTaintFlow=t10.1
66+
self.x += sink(x) + source("t10.1");
6767
sink(x); // $ hasTaintFlow=t10.1
6868
sink(self.x); // $ hasTaintFlow=t10.1
6969
}
7070

7171
func t11() {
7272
self.x = "safe";
73-
x += sink(x) + source("t11.1"); // $ SPURIOUS: hasTaintFlow=t11.1
73+
x += sink(x) + source("t11.1");
7474
sink(x); // $ hasTaintFlow=t11.1
7575
sink(self.x); // $ hasTaintFlow=t11.1
7676
}
7777

7878
func t12() {
7979
self.x = "safe";
80-
self.x += sink(x) + source("t12.1"); // $ SPURIOUS: hasTaintFlow=t12.1
80+
self.x += sink(x) + source("t12.1");
8181
sink(x); // $ hasTaintFlow=t12.1
8282
sink(self.x); // $ hasTaintFlow=t12.1
8383
}

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

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,23 +32,19 @@ edges
3232
| implicit-self.swift:53:9:53:18 | MemberAccessExpr | implicit-self.swift:53:9:53:16 | [post] MemberAccessExpr [x] | provenance | |
3333
| implicit-self.swift:53:22:53:35 | CallExpr | implicit-self.swift:53:9:53:18 | MemberAccessExpr | provenance | |
3434
| implicit-self.swift:54:14:54:16 | box [x] | implicit-self.swift:54:14:54:18 | MemberAccessExpr | provenance | |
35-
| implicit-self.swift:59:9:59:9 | [incoming] x | implicit-self.swift:59:19:59:19 | x | provenance | |
3635
| implicit-self.swift:59:9:59:9 | [incoming] x | implicit-self.swift:60:14:60:14 | x | provenance | |
3736
| implicit-self.swift:59:9:59:9 | [incoming] x | implicit-self.swift:61:14:61:17 | self [x] | provenance | |
3837
| implicit-self.swift:59:24:59:37 | CallExpr | implicit-self.swift:59:9:59:9 | [incoming] x | provenance | |
3938
| implicit-self.swift:61:14:61:17 | self [x] | implicit-self.swift:61:14:61:19 | MemberAccessExpr | provenance | |
40-
| implicit-self.swift:66:9:66:12 | [post] self [x] | implicit-self.swift:66:24:66:24 | x | provenance | |
4139
| implicit-self.swift:66:9:66:12 | [post] self [x] | implicit-self.swift:67:14:67:14 | x | provenance | |
4240
| implicit-self.swift:66:9:66:12 | [post] self [x] | implicit-self.swift:68:14:68:17 | self [x] | provenance | |
4341
| implicit-self.swift:66:9:66:14 | [incoming] MemberAccessExpr | implicit-self.swift:66:9:66:12 | [post] self [x] | provenance | |
4442
| implicit-self.swift:66:29:66:43 | CallExpr | implicit-self.swift:66:9:66:14 | [incoming] MemberAccessExpr | provenance | |
4543
| implicit-self.swift:68:14:68:17 | self [x] | implicit-self.swift:68:14:68:19 | MemberAccessExpr | provenance | |
46-
| implicit-self.swift:73:9:73:9 | [incoming] x | implicit-self.swift:73:19:73:19 | x | provenance | |
4744
| implicit-self.swift:73:9:73:9 | [incoming] x | implicit-self.swift:74:14:74:14 | x | provenance | |
4845
| implicit-self.swift:73:9:73:9 | [incoming] x | implicit-self.swift:75:14:75:17 | self [x] | provenance | |
4946
| implicit-self.swift:73:24:73:38 | CallExpr | implicit-self.swift:73:9:73:9 | [incoming] x | provenance | |
5047
| implicit-self.swift:75:14:75:17 | self [x] | implicit-self.swift:75:14:75:19 | MemberAccessExpr | provenance | |
51-
| implicit-self.swift:80:9:80:12 | [post] self [x] | implicit-self.swift:80:24:80:24 | x | provenance | |
5248
| implicit-self.swift:80:9:80:12 | [post] self [x] | implicit-self.swift:81:14:81:14 | x | provenance | |
5349
| implicit-self.swift:80:9:80:12 | [post] self [x] | implicit-self.swift:82:14:82:17 | self [x] | provenance | |
5450
| implicit-self.swift:80:9:80:14 | [incoming] MemberAccessExpr | implicit-self.swift:80:9:80:12 | [post] self [x] | provenance | |
@@ -165,27 +161,23 @@ nodes
165161
| implicit-self.swift:54:14:54:16 | box [x] | semmle.label | box [x] |
166162
| implicit-self.swift:54:14:54:18 | MemberAccessExpr | semmle.label | MemberAccessExpr |
167163
| implicit-self.swift:59:9:59:9 | [incoming] x | semmle.label | [incoming] x |
168-
| implicit-self.swift:59:19:59:19 | x | semmle.label | x |
169164
| implicit-self.swift:59:24:59:37 | CallExpr | semmle.label | CallExpr |
170165
| implicit-self.swift:60:14:60:14 | x | semmle.label | x |
171166
| implicit-self.swift:61:14:61:17 | self [x] | semmle.label | self [x] |
172167
| implicit-self.swift:61:14:61:19 | MemberAccessExpr | semmle.label | MemberAccessExpr |
173168
| implicit-self.swift:66:9:66:12 | [post] self [x] | semmle.label | [post] self [x] |
174169
| implicit-self.swift:66:9:66:14 | [incoming] MemberAccessExpr | semmle.label | [incoming] MemberAccessExpr |
175-
| implicit-self.swift:66:24:66:24 | x | semmle.label | x |
176170
| implicit-self.swift:66:29:66:43 | CallExpr | semmle.label | CallExpr |
177171
| implicit-self.swift:67:14:67:14 | x | semmle.label | x |
178172
| implicit-self.swift:68:14:68:17 | self [x] | semmle.label | self [x] |
179173
| implicit-self.swift:68:14:68:19 | MemberAccessExpr | semmle.label | MemberAccessExpr |
180174
| implicit-self.swift:73:9:73:9 | [incoming] x | semmle.label | [incoming] x |
181-
| implicit-self.swift:73:19:73:19 | x | semmle.label | x |
182175
| implicit-self.swift:73:24:73:38 | CallExpr | semmle.label | CallExpr |
183176
| implicit-self.swift:74:14:74:14 | x | semmle.label | x |
184177
| implicit-self.swift:75:14:75:17 | self [x] | semmle.label | self [x] |
185178
| implicit-self.swift:75:14:75:19 | MemberAccessExpr | semmle.label | MemberAccessExpr |
186179
| implicit-self.swift:80:9:80:12 | [post] self [x] | semmle.label | [post] self [x] |
187180
| implicit-self.swift:80:9:80:14 | [incoming] MemberAccessExpr | semmle.label | [incoming] MemberAccessExpr |
188-
| implicit-self.swift:80:24:80:24 | x | semmle.label | x |
189181
| implicit-self.swift:80:29:80:43 | CallExpr | semmle.label | CallExpr |
190182
| implicit-self.swift:81:14:81:14 | x | semmle.label | x |
191183
| implicit-self.swift:82:14:82:17 | self [x] | semmle.label | self [x] |
@@ -294,16 +286,12 @@ testFailures
294286
| implicit-self.swift:42:14:42:18 | MemberAccessExpr | implicit-self.swift:41:17:41:30 | CallExpr | implicit-self.swift:42:14:42:18 | MemberAccessExpr | $@ | implicit-self.swift:41:17:41:30 | CallExpr | CallExpr |
295287
| implicit-self.swift:48:14:48:23 | MemberAccessExpr | implicit-self.swift:47:17:47:30 | CallExpr | implicit-self.swift:48:14:48:23 | MemberAccessExpr | $@ | implicit-self.swift:47:17:47:30 | CallExpr | CallExpr |
296288
| implicit-self.swift:54:14:54:18 | MemberAccessExpr | implicit-self.swift:53:22:53:35 | CallExpr | implicit-self.swift:54:14:54:18 | MemberAccessExpr | $@ | implicit-self.swift:53:22:53:35 | CallExpr | CallExpr |
297-
| implicit-self.swift:59:19:59:19 | x | implicit-self.swift:59:24:59:37 | CallExpr | implicit-self.swift:59:19:59:19 | x | $@ | implicit-self.swift:59:24:59:37 | CallExpr | CallExpr |
298289
| implicit-self.swift:60:14:60:14 | x | implicit-self.swift:59:24:59:37 | CallExpr | implicit-self.swift:60:14:60:14 | x | $@ | implicit-self.swift:59:24:59:37 | CallExpr | CallExpr |
299290
| implicit-self.swift:61:14:61:19 | MemberAccessExpr | implicit-self.swift:59:24:59:37 | CallExpr | implicit-self.swift:61:14:61:19 | MemberAccessExpr | $@ | implicit-self.swift:59:24:59:37 | CallExpr | CallExpr |
300-
| implicit-self.swift:66:24:66:24 | x | implicit-self.swift:66:29:66:43 | CallExpr | implicit-self.swift:66:24:66:24 | x | $@ | implicit-self.swift:66:29:66:43 | CallExpr | CallExpr |
301291
| implicit-self.swift:67:14:67:14 | x | implicit-self.swift:66:29:66:43 | CallExpr | implicit-self.swift:67:14:67:14 | x | $@ | implicit-self.swift:66:29:66:43 | CallExpr | CallExpr |
302292
| implicit-self.swift:68:14:68:19 | MemberAccessExpr | implicit-self.swift:66:29:66:43 | CallExpr | implicit-self.swift:68:14:68:19 | MemberAccessExpr | $@ | implicit-self.swift:66:29:66:43 | CallExpr | CallExpr |
303-
| implicit-self.swift:73:19:73:19 | x | implicit-self.swift:73:24:73:38 | CallExpr | implicit-self.swift:73:19:73:19 | x | $@ | implicit-self.swift:73:24:73:38 | CallExpr | CallExpr |
304293
| implicit-self.swift:74:14:74:14 | x | implicit-self.swift:73:24:73:38 | CallExpr | implicit-self.swift:74:14:74:14 | x | $@ | implicit-self.swift:73:24:73:38 | CallExpr | CallExpr |
305294
| implicit-self.swift:75:14:75:19 | MemberAccessExpr | implicit-self.swift:73:24:73:38 | CallExpr | implicit-self.swift:75:14:75:19 | MemberAccessExpr | $@ | implicit-self.swift:73:24:73:38 | CallExpr | CallExpr |
306-
| implicit-self.swift:80:24:80:24 | x | implicit-self.swift:80:29:80:43 | CallExpr | implicit-self.swift:80:24:80:24 | x | $@ | implicit-self.swift:80:29:80:43 | CallExpr | CallExpr |
307295
| implicit-self.swift:81:14:81:14 | x | implicit-self.swift:80:29:80:43 | CallExpr | implicit-self.swift:81:14:81:14 | x | $@ | implicit-self.swift:80:29:80:43 | CallExpr | CallExpr |
308296
| implicit-self.swift:82:14:82:19 | MemberAccessExpr | implicit-self.swift:80:29:80:43 | CallExpr | implicit-self.swift:82:14:82:19 | MemberAccessExpr | $@ | implicit-self.swift:80:29:80:43 | CallExpr | CallExpr |
309297
| test.swift:2:10:2:21 | CallExpr | test.swift:2:10:2:21 | CallExpr | test.swift:2:10:2:21 | CallExpr | $@ | test.swift:2:10:2:21 | CallExpr | CallExpr |

0 commit comments

Comments
 (0)