Skip to content

Commit 6741896

Browse files
asgerfasgerf
committed
unified: Simple fix for getter calls missing post update node
1 parent ac8adee commit 6741896

3 files changed

Lines changed: 42 additions & 5 deletions

File tree

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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,10 @@ private predicate hasIncomingValueAtCfgNode(Expr expr, ControlFlowNode cfgNode)
2727
)
2828
}
2929

30-
private predicate hasPostUpdate(Expr expr, ControlFlowNode cfgNode) {
30+
/** Holds if `expr` has a post-update at `cfgNode` resulting from a surrounding call or assignment. */
31+
private predicate hasPostUpdateAfterCallOrAssign(Expr expr, ControlFlowNode cfgNode) {
3132
exists(MemberAccessExpr member |
32-
(hasIncomingValueAtCfgNode(member, cfgNode) or hasPostUpdate(member, cfgNode)) and
33+
(hasIncomingValueAtCfgNode(member, cfgNode) or hasPostUpdateAfterCallOrAssign(member, cfgNode)) and
3334
expr = member.getBase()
3435
)
3536
or
@@ -40,11 +41,28 @@ private predicate hasPostUpdate(Expr expr, ControlFlowNode cfgNode) {
4041
)
4142
or
4243
exists(Expr target |
43-
hasPostUpdate(target, cfgNode) and
44+
hasPostUpdateAfterCallOrAssign(target, cfgNode) and
4445
exprStep(expr, target, _)
4546
)
4647
}
4748

49+
/**
50+
* Holds if `expr` has a post-update at `cfgNode`.
51+
*/
52+
private predicate hasPostUpdate(Expr expr, ControlFlowNode cfgNode) {
53+
hasPostUpdateAfterCallOrAssign(expr, cfgNode)
54+
or
55+
not hasPostUpdateAfterCallOrAssign(expr, _) and
56+
exists(MemberAccessExpr member |
57+
// If the member access is not updated by call or assignment, it might still be a call to a getter,
58+
// and thus needs a post-update. Generate a post-update for this case.
59+
// In case it's a call to a getter, while also being post-updated from a call or assignment, we accept
60+
// the imprecision and use the post-update position from the call or assignment.
61+
expr = member.getBase() and
62+
cfgNode.isAfter(member)
63+
)
64+
}
65+
4866
/**
4967
* Holds if `repr` performs an access to `var` of the given `kind` at `cfgNode`.
5068
*
@@ -69,6 +87,11 @@ predicate performsVariableAccess(
6987
(hasIncomingValueAtCfgNode(access, cfgNode) or hasPostUpdate(access, cfgNode)) and
7088
kind.isPostUpdate()
7189
or
90+
// Generate a post-update for the receiver in case this is a call to a getter that mutates 'self'.
91+
not (hasIncomingValueAtCfgNode(access, _) or hasPostUpdate(access, _)) and
92+
cfgNode.isAfter(access) and
93+
kind.isPostUpdate()
94+
or
7295
exists(CallExpr call |
7396
access = call.getCallee() and
7497
cfgNode.isAfter(call) and

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func t9() {
109109
}
110110
let c = C()
111111
let ignored = c.x
112-
sink(c.field) // $ MISSING: hasValueFlow=t9.1
112+
sink(c.field) // $ hasValueFlow=t9.1
113113
}
114114

115115
func t10() {
@@ -122,7 +122,7 @@ func t10() {
122122

123123
func method() {
124124
let ignored = x
125-
sink(field) // $ MISSING: hasValueFlow=t10.1
125+
sink(field) // $ hasValueFlow=t10.1
126126
}
127127
}
128128
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ edges
2828
| accessors_instance.swift:98:5:98:5 | c [field] | accessors_instance.swift:93:32:93:36 | field | provenance | |
2929
| accessors_instance.swift:98:5:98:5 | c [field] | accessors_instance.swift:99:10:99:10 | c [otherField] | provenance | |
3030
| accessors_instance.swift:99:10:99:10 | c [otherField] | accessors_instance.swift:99:10:99:21 | ... .otherField | provenance | |
31+
| accessors_instance.swift:106:13:106:17 | field | accessors_instance.swift:112:10:112:10 | c [field] | provenance | |
32+
| accessors_instance.swift:106:21:106:34 | source(...) | accessors_instance.swift:106:13:106:17 | field | provenance | |
33+
| accessors_instance.swift:112:10:112:10 | c [field] | accessors_instance.swift:112:10:112:16 | ... .field | provenance | |
34+
| accessors_instance.swift:119:13:119:17 | field | accessors_instance.swift:125:18:125:22 | field | provenance | |
35+
| accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:119:13:119:17 | field | provenance | |
3136
| accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | provenance | |
3237
| accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | provenance | |
3338
| accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | provenance | |
@@ -544,6 +549,13 @@ nodes
544549
| accessors_instance.swift:98:5:98:5 | c [field] | semmle.label | c [field] |
545550
| accessors_instance.swift:99:10:99:10 | c [otherField] | semmle.label | c [otherField] |
546551
| accessors_instance.swift:99:10:99:21 | ... .otherField | semmle.label | ... .otherField |
552+
| accessors_instance.swift:106:13:106:17 | field | semmle.label | field |
553+
| accessors_instance.swift:106:21:106:34 | source(...) | semmle.label | source(...) |
554+
| accessors_instance.swift:112:10:112:10 | c [field] | semmle.label | c [field] |
555+
| accessors_instance.swift:112:10:112:16 | ... .field | semmle.label | ... .field |
556+
| accessors_instance.swift:119:13:119:17 | field | semmle.label | field |
557+
| accessors_instance.swift:119:21:119:35 | source(...) | semmle.label | source(...) |
558+
| accessors_instance.swift:125:18:125:22 | field | semmle.label | field |
547559
| accessors_local.swift:7:21:7:34 | source(...) | semmle.label | source(...) |
548560
| accessors_local.swift:8:10:8:10 | x | semmle.label | x |
549561
| accessors_local.swift:15:15:15:28 | source(...) | semmle.label | source(...) |
@@ -1171,6 +1183,8 @@ testFailures
11711183
| accessors_instance.swift:71:18:71:18 | x | accessors_instance.swift:67:25:67:38 | source(...) | accessors_instance.swift:71:18:71:18 | x | $@ | accessors_instance.swift:67:25:67:38 | source(...) | source(...) |
11721184
| accessors_instance.swift:84:10:84:12 | ... .x | accessors_instance.swift:83:15:83:28 | source(...) | accessors_instance.swift:84:10:84:12 | ... .x | $@ | accessors_instance.swift:83:15:83:28 | source(...) | source(...) |
11731185
| 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(...) |
1186+
| 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(...) |
1187+
| 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(...) |
11741188
| 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(...) |
11751189
| 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(...) |
11761190
| 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)