Skip to content

Commit 668d51f

Browse files
hvitvedCopilot
hvitved
and
Copilot
committed
Unified: use type-inferred field resolution in data flow
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 8e7a6d2 commit 668d51f

5 files changed

Lines changed: 73 additions & 22 deletions

File tree

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

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
private import unified
22
private import AllDataFlow
3+
private import codeql.unified.internal.typeinference.TypeInference as T
4+
private import codeql.unified.internal.typeinference.TypeInferencePlugin as TP
35

46
/**
57
* Holds if a MaD access path token of form `name[arg]` exists.
@@ -8,8 +10,20 @@ predicate isAccessPathTokenPresent(string name, string arg) {
810
arg = any(FlowSummaryOutput::Private::AccessPathToken tok).getAnArgument(name)
911
}
1012

13+
private class ResolvedField extends VariableDeclaration {
14+
ResolvedField() { this = T::resolveFieldAccess(_) }
15+
16+
string getName() {
17+
result = super.getName() and
18+
not this = any(TP::TupleType tt).getField(_)
19+
or
20+
this = any(TP::TupleType tt).getField(result)
21+
}
22+
}
23+
1124
private newtype TContent =
1225
TArrayElement() or
26+
TFieldContent(ResolvedField field) or
1327
TNamedMember(string name) {
1428
name = any(Identifier id).getValue()
1529
or
@@ -24,23 +38,35 @@ private newtype TContent =
2438
TCapturedVariable(CaptureSsaInput::CapturedVariable v)
2539

2640
class Content extends TContent {
41+
ResolvedField asField() { this = TFieldContent(result) }
42+
2743
string asNamedMember() { this = TNamedMember(result) }
2844

2945
predicate isArrayElement() { this = TArrayElement() }
3046

3147
string toString() {
3248
result = this.asNamedMember()
3349
or
50+
result = this.asField().getName()
51+
or
3452
this.isArrayElement() and result = "ArrayElement"
3553
or
3654
result = this.asCapturedVariable().toString()
3755
}
3856

3957
LocalVariable asCapturedVariable() { this = TCapturedVariable(result) }
4058

41-
Location getLocation() { result = this.asCapturedVariable().getLocation() }
59+
Location getLocation() {
60+
result = this.asCapturedVariable().getLocation()
61+
or
62+
result = this.asField().getLocation()
63+
}
4264
}
4365

66+
private class TContentApprox = TArrayElement or TNamedMember or TCapturedVariable;
67+
68+
class ContentApprox extends Content, TContentApprox { }
69+
4470
private newtype TContentSet = TSingleton(Content content)
4571

4672
class ContentSet extends TContentSet {
@@ -52,10 +78,41 @@ class ContentSet extends TContentSet {
5278

5379
Content getAStoreContent() { result = this.asSingleton() }
5480

55-
Content getAReadContent() { result = this.asSingleton() }
81+
Content getAReadContent() {
82+
result = this.asSingleton()
83+
or
84+
result.asNamedMember() = this.asSingleton().asField().getName()
85+
or
86+
result.asField().getName() = this.asSingleton().asNamedMember()
87+
}
5688
}
5789

5890
module ContentSet {
91+
/**
92+
* Gets the member name to use for `access`, if any.
93+
*
94+
* Only returns a member name if it cannot be resolved uniquely to a field.
95+
*/
96+
private string getMemberName(Expr access) {
97+
access =
98+
any(MemberAccessExpr mae |
99+
result = mae.getMemberName() and
100+
if exists(T::inferTypeUnique(mae.getBase()))
101+
then not exists(T::resolveFieldAccess(mae))
102+
else any()
103+
)
104+
or
105+
result = access.(UnqualifiedMemberAccess).getMemberName() and
106+
not exists(T::resolveFieldAccess(access))
107+
}
108+
109+
ContentSet member(Expr access) {
110+
result.asSingleton().asField() = T::resolveFieldAccess(access) and
111+
not exists(getMemberName(access))
112+
or
113+
result = namedMember(getMemberName(access))
114+
}
115+
59116
ContentSet namedMember(string name) { result.asSingleton().asNamedMember() = name }
60117

61118
ContentSet arrayElement() { result.asSingleton().isArrayElement() }

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,11 @@ predicate step(BuilderNode node1, Step step, BuilderNode node2) {
9292
or
9393
exists(UnqualifiedMemberAccess access | access.isInstanceAccess() |
9494
node1.isLocalVariableRead(access, access.getImplicitQualifierVariable()) and
95-
step.readName(access.getMemberName()) and
95+
step.read(ContentSet::member(access)) and
9696
node2.isResultValue(access)
9797
or
9898
(node1.isIncomingValue(access) or node1.isPostUpdate(access)) and
99-
step.storeName(access.getMemberName()) and
99+
step.store(ContentSet::member(access)) and
100100
node2.isLocalVariablePostUpdate(access, access.getImplicitQualifierVariable())
101101
or
102102
// Pass the receiver along to the getter/setter invoked by this expression
@@ -129,11 +129,11 @@ predicate step(BuilderNode node1, Step step, BuilderNode node2) {
129129
or
130130
exists(MemberAccessExpr expr |
131131
node1.isResultValue(expr.getBase()) and
132-
step.readName(expr.getMemberName()) and
132+
step.read(ContentSet::member(expr)) and
133133
node2.isResultValue(expr)
134134
or
135135
(node1.isIncomingValue(expr) or node1.isPostUpdate(expr)) and
136-
step.storeName(expr.getMemberName()) and
136+
step.store(ContentSet::member(expr)) and
137137
node2.isPostUpdate(expr.getBase())
138138
or
139139
// Pass the receiver along to the getter/setter invoked by this expression

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,13 @@ module DataFlowInput implements InputSig<Location> {
1515

1616
class ContentSet = D::ContentSet;
1717

18-
class ContentApprox = Content; // TODO
18+
class ContentApprox = D::ContentApprox;
1919

20-
ContentApprox getContentApprox(Content c) { result = c } // TODO
20+
ContentApprox getContentApprox(Content c) {
21+
result = c
22+
or
23+
result.asNamedMember() = c.asField().getName()
24+
}
2125

2226
//
2327
// Parameter, argument, return, and out nodes and their positions/kinds

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ func resolvedFields() {
1717
let box = FieldBox()
1818
box.field = source("fields.resolved")
1919
sink(box.field) // $ hasValueFlow=fields.resolved
20-
sink((box as! OtherFieldBox).field) // $ SPURIOUS: hasValueFlow=fields.resolved
20+
sink((box as! OtherFieldBox).field) // no flow
2121
}
2222

2323
func inheritedFields() {
@@ -81,7 +81,7 @@ func protocolFieldRead() {
8181
let p = box as FieldProtocol
8282
sink(p.field) // $ hasValueFlow=fields.protocol
8383
let box2 = box as! ProtocolBox2
84-
sink(box2.field) // $ SPURIOUS: hasValueFlow=fields.protocol
84+
sink(box2.field)
8585
let box1or2 = 2 > 3 ? box as FieldProtocol : box2
8686
sink(box1or2.field) // $ hasValueFlow=fields.protocol
8787
}

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

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ edges
3232
| accessors_instance.swift:119:13:119:17 | field | accessors_instance.swift:125:18:125:22 | field | provenance | |
3333
| accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:119:13:119:17 | field | provenance | |
3434
| accessors_instance.swift:134:24:134:31 | newValue [0] | accessors_instance.swift:134:24:134:33 | ... .0 | provenance | |
35+
| accessors_instance.swift:134:24:134:31 | newValue [0] | accessors_instance.swift:134:24:134:33 | ... .0 | provenance | |
3536
| accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
3637
| accessors_instance.swift:143:5:143:9 | ... .0 | accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | provenance | |
3738
| accessors_instance.swift:143:13:143:27 | source(...) | accessors_instance.swift:143:5:143:9 | ... .0 | provenance | |
@@ -252,12 +253,9 @@ edges
252253
| enums.swift:83:10:83:23 | ... .foo(...) [foo.0] | enums.swift:83:22:83:22 | x | provenance | |
253254
| enums.swift:83:22:83:22 | x | enums.swift:84:14:84:14 | x | provenance | |
254255
| fields.swift:18:5:18:7 | [post] box [field] | fields.swift:19:10:19:12 | box [field] | provenance | |
255-
| fields.swift:18:5:18:7 | [post] box [field] | fields.swift:20:11:20:13 | box [field] | provenance | |
256256
| fields.swift:18:5:18:13 | ... .field | fields.swift:18:5:18:7 | [post] box [field] | provenance | |
257257
| fields.swift:18:17:18:41 | source(...) | fields.swift:18:5:18:13 | ... .field | provenance | |
258258
| fields.swift:19:10:19:12 | box [field] | fields.swift:19:10:19:18 | ... .field | provenance | |
259-
| fields.swift:20:11:20:13 | box [field] | fields.swift:20:11:20:31 | TypeCastExpr [field] | provenance | |
260-
| fields.swift:20:11:20:31 | TypeCastExpr [field] | fields.swift:20:10:20:38 | ... .field | provenance | |
261259
| fields.swift:25:5:25:7 | [post] box [field] | fields.swift:26:11:26:13 | box [field] | provenance | |
262260
| fields.swift:25:5:25:13 | ... .field | fields.swift:25:5:25:7 | [post] box [field] | provenance | |
263261
| fields.swift:25:17:25:42 | source(...) | fields.swift:25:5:25:13 | ... .field | provenance | |
@@ -294,10 +292,8 @@ edges
294292
| fields.swift:81:9:81:9 | p [field] | fields.swift:82:10:82:10 | p [field] | provenance | |
295293
| fields.swift:81:13:81:15 | box [field] | fields.swift:81:9:81:9 | p [field] | provenance | |
296294
| fields.swift:82:10:82:10 | p [field] | fields.swift:82:10:82:16 | ... .field | provenance | |
297-
| fields.swift:83:9:83:12 | box2 [field] | fields.swift:84:10:84:13 | box2 [field] | provenance | |
298295
| fields.swift:83:9:83:12 | box2 [field] | fields.swift:85:50:85:53 | box2 [field] | provenance | |
299296
| fields.swift:83:16:83:18 | box [field] | fields.swift:83:9:83:12 | box2 [field] | provenance | |
300-
| fields.swift:84:10:84:13 | box2 [field] | fields.swift:84:10:84:19 | ... .field | provenance | |
301297
| fields.swift:85:9:85:15 | box1or2 [field] | fields.swift:86:10:86:16 | box1or2 [field] | provenance | |
302298
| fields.swift:85:27:85:29 | box [field] | fields.swift:85:9:85:15 | box1or2 [field] | provenance | |
303299
| fields.swift:85:50:85:53 | box2 [field] | fields.swift:85:9:85:15 | box1or2 [field] | provenance | |
@@ -635,6 +631,7 @@ nodes
635631
| accessors_instance.swift:119:21:119:35 | source(...) | semmle.label | source(...) |
636632
| accessors_instance.swift:125:18:125:22 | field | semmle.label | field |
637633
| accessors_instance.swift:134:24:134:31 | newValue [0] | semmle.label | newValue [0] |
634+
| accessors_instance.swift:134:24:134:31 | newValue [0] | semmle.label | newValue [0] |
638635
| accessors_instance.swift:134:24:134:33 | ... .0 | semmle.label | ... .0 |
639636
| accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | semmle.label | [post] ... .p [0] |
640637
| accessors_instance.swift:143:5:143:9 | ... .0 | semmle.label | ... .0 |
@@ -913,9 +910,6 @@ nodes
913910
| fields.swift:18:17:18:41 | source(...) | semmle.label | source(...) |
914911
| fields.swift:19:10:19:12 | box [field] | semmle.label | box [field] |
915912
| fields.swift:19:10:19:18 | ... .field | semmle.label | ... .field |
916-
| fields.swift:20:10:20:38 | ... .field | semmle.label | ... .field |
917-
| fields.swift:20:11:20:13 | box [field] | semmle.label | box [field] |
918-
| fields.swift:20:11:20:31 | TypeCastExpr [field] | semmle.label | TypeCastExpr [field] |
919913
| fields.swift:25:5:25:7 | [post] box [field] | semmle.label | [post] box [field] |
920914
| fields.swift:25:5:25:13 | ... .field | semmle.label | ... .field |
921915
| fields.swift:25:17:25:42 | source(...) | semmle.label | source(...) |
@@ -958,8 +952,6 @@ nodes
958952
| fields.swift:82:10:82:16 | ... .field | semmle.label | ... .field |
959953
| fields.swift:83:9:83:12 | box2 [field] | semmle.label | box2 [field] |
960954
| fields.swift:83:16:83:18 | box [field] | semmle.label | box [field] |
961-
| fields.swift:84:10:84:13 | box2 [field] | semmle.label | box2 [field] |
962-
| fields.swift:84:10:84:19 | ... .field | semmle.label | ... .field |
963955
| fields.swift:85:9:85:15 | box1or2 [field] | semmle.label | box1or2 [field] |
964956
| fields.swift:85:27:85:29 | box [field] | semmle.label | box [field] |
965957
| fields.swift:85:50:85:53 | box2 [field] | semmle.label | box2 [field] |
@@ -1435,15 +1427,13 @@ testFailures
14351427
| enums.swift:76:14:76:14 | x | enums.swift:73:34:73:47 | source(...) | enums.swift:76:14:76:14 | x | $@ | enums.swift:73:34:73:47 | source(...) | source(...) |
14361428
| enums.swift:84:14:84:14 | x | enums.swift:73:34:73:47 | source(...) | enums.swift:84:14:84:14 | x | $@ | enums.swift:73:34:73:47 | source(...) | source(...) |
14371429
| fields.swift:19:10:19:18 | ... .field | fields.swift:18:17:18:41 | source(...) | fields.swift:19:10:19:18 | ... .field | $@ | fields.swift:18:17:18:41 | source(...) | source(...) |
1438-
| fields.swift:20:10:20:38 | ... .field | fields.swift:18:17:18:41 | source(...) | fields.swift:20:10:20:38 | ... .field | $@ | fields.swift:18:17:18:41 | source(...) | source(...) |
14391430
| fields.swift:26:10:26:33 | ... .field | fields.swift:25:17:25:42 | source(...) | fields.swift:26:10:26:33 | ... .field | $@ | fields.swift:25:17:25:42 | source(...) | source(...) |
14401431
| fields.swift:32:10:32:40 | ... .field | fields.swift:31:17:31:48 | source(...) | fields.swift:32:10:32:40 | ... .field | $@ | fields.swift:31:17:31:48 | source(...) | source(...) |
14411432
| fields.swift:38:10:38:18 | ... .field | fields.swift:37:39:37:71 | source(...) | fields.swift:38:10:38:18 | ... .field | $@ | fields.swift:37:39:37:71 | source(...) | source(...) |
14421433
| fields.swift:44:10:44:32 | ... .readField(...) | fields.swift:43:17:43:40 | source(...) | fields.swift:44:10:44:32 | ... .readField(...) | $@ | fields.swift:43:17:43:40 | source(...) | source(...) |
14431434
| fields.swift:62:10:62:18 | ... .field | fields.swift:60:18:60:47 | source(...) | fields.swift:62:10:62:18 | ... .field | $@ | fields.swift:60:18:60:47 | source(...) | source(...) |
14441435
| fields.swift:80:10:80:18 | ... .field | fields.swift:79:17:79:41 | source(...) | fields.swift:80:10:80:18 | ... .field | $@ | fields.swift:79:17:79:41 | source(...) | source(...) |
14451436
| fields.swift:82:10:82:16 | ... .field | fields.swift:79:17:79:41 | source(...) | fields.swift:82:10:82:16 | ... .field | $@ | fields.swift:79:17:79:41 | source(...) | source(...) |
1446-
| fields.swift:84:10:84:19 | ... .field | fields.swift:79:17:79:41 | source(...) | fields.swift:84:10:84:19 | ... .field | $@ | fields.swift:79:17:79:41 | source(...) | source(...) |
14471437
| fields.swift:86:10:86:22 | ... .field | fields.swift:79:17:79:41 | source(...) | fields.swift:86:10:86:22 | ... .field | $@ | fields.swift:79:17:79:41 | source(...) | source(...) |
14481438
| fields.swift:93:10:93:16 | ... .field | fields.swift:92:15:92:39 | source(...) | fields.swift:93:10:93:16 | ... .field | $@ | fields.swift:92:15:92:39 | source(...) | source(...) |
14491439
| fields.swift:96:10:96:19 | ... .field | fields.swift:92:15:92:39 | source(...) | fields.swift:96:10:96:19 | ... .field | $@ | fields.swift:92:15:92:39 | source(...) | source(...) |

0 commit comments

Comments
 (0)