Skip to content

Commit 5dbb91f

Browse files
owen-mcowen-mc
authored
Merge pull request #16504 from owen-mc/go/allow-array-reads-from-named-types
Go: allow read and store steps from named types
2 parents c4d33fb + 6ffa821 commit 5dbb91f

File tree

9 files changed

+233
-153
lines changed

9 files changed

+233
-153
lines changed

‎go/ql/lib/change-notes/2024-05-16-read-store-steps-named-types.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* A bug has been fixed which meant flow was not followed through some ranged for loops. This may lead to more alerts being found.

‎go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll

Lines changed: 61 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -14,38 +14,40 @@ private import semmle.go.dataflow.ExternalFlow
1414
* varargs.
1515
*/
1616
predicate containerStoreStep(Node node1, Node node2, Content c) {
17-
c instanceof ArrayContent and
18-
(
17+
exists(Type t | t = node2.getType().getUnderlyingType() |
18+
c instanceof ArrayContent and
1919
(
20-
node2.getType() instanceof ArrayType or
21-
node2.getType() instanceof SliceType
22-
) and
23-
(
24-
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
25-
or
26-
node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
27-
or
28-
// To model data flow from array elements of the base of a `SliceNode` to
29-
// the `SliceNode` itself, we consider there to be a read step with array
30-
// content from the base to the corresponding `SliceElementNode` and then
31-
// a store step with array content from the `SliceelementNode` to the
32-
// `SliceNode` itself.
33-
node2 = node1.(SliceElementNode).getSliceNode()
20+
(
21+
t instanceof ArrayType or
22+
t instanceof SliceType
23+
) and
24+
(
25+
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
26+
or
27+
node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
28+
or
29+
// To model data flow from array elements of the base of a `SliceNode` to
30+
// the `SliceNode` itself, we consider there to be a read step with array
31+
// content from the base to the corresponding `SliceElementNode` and then
32+
// a store step with array content from the `SliceelementNode` to the
33+
// `SliceNode` itself.
34+
node2 = node1.(SliceElementNode).getSliceNode()
35+
)
3436
)
37+
or
38+
c instanceof CollectionContent and
39+
exists(SendStmt send |
40+
send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
41+
)
42+
or
43+
c instanceof MapKeyContent and
44+
t instanceof MapType and
45+
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), node1, _))
46+
or
47+
c instanceof MapValueContent and
48+
t instanceof MapType and
49+
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
3550
)
36-
or
37-
c instanceof CollectionContent and
38-
exists(SendStmt send |
39-
send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
40-
)
41-
or
42-
c instanceof MapKeyContent and
43-
node2.getType() instanceof MapType and
44-
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), node1, _))
45-
or
46-
c instanceof MapValueContent and
47-
node2.getType() instanceof MapType and
48-
exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
4951
}
5052

5153
/**
@@ -55,35 +57,37 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
5557
* as well as array iteration through enhanced `for` statements.
5658
*/
5759
predicate containerReadStep(Node node1, Node node2, Content c) {
58-
c instanceof ArrayContent and
59-
(
60-
node1.getType() instanceof ArrayType or
61-
node1.getType() instanceof SliceType
62-
) and
63-
(
64-
node2.(Read).readsElement(node1, _)
60+
exists(Type t | t = node1.getType().getUnderlyingType() |
61+
c instanceof ArrayContent and
62+
(
63+
t instanceof ArrayType or
64+
t instanceof SliceType
65+
) and
66+
(
67+
node2.(Read).readsElement(node1, _)
68+
or
69+
node2.(RangeElementNode).getBase() = node1
70+
or
71+
// To model data flow from array elements of the base of a `SliceNode` to
72+
// the `SliceNode` itself, we consider there to be a read step with array
73+
// content from the base to the corresponding `SliceElementNode` and then
74+
// a store step with array content from the `SliceelementNode` to the
75+
// `SliceNode` itself.
76+
node2.(SliceElementNode).getSliceNode().getBase() = node1
77+
)
6578
or
66-
node2.(RangeElementNode).getBase() = node1
79+
c instanceof CollectionContent and
80+
exists(UnaryOperationNode recv | recv = node2 |
81+
node1 = recv.getOperand() and
82+
recv.getOperator() = "<-"
83+
)
6784
or
68-
// To model data flow from array elements of the base of a `SliceNode` to
69-
// the `SliceNode` itself, we consider there to be a read step with array
70-
// content from the base to the corresponding `SliceElementNode` and then
71-
// a store step with array content from the `SliceelementNode` to the
72-
// `SliceNode` itself.
73-
node2.(SliceElementNode).getSliceNode().getBase() = node1
74-
)
75-
or
76-
c instanceof CollectionContent and
77-
exists(UnaryOperationNode recv | recv = node2 |
78-
node1 = recv.getOperand() and
79-
recv.getOperator() = "<-"
85+
c instanceof MapKeyContent and
86+
t instanceof MapType and
87+
node2.(RangeIndexNode).getBase() = node1
88+
or
89+
c instanceof MapValueContent and
90+
t instanceof MapType and
91+
(node2.(Read).readsElement(node1, _) or node2.(RangeElementNode).getBase() = node1)
8092
)
81-
or
82-
c instanceof MapKeyContent and
83-
node1.getType() instanceof MapType and
84-
node2.(RangeIndexNode).getBase() = node1
85-
or
86-
c instanceof MapValueContent and
87-
node1.getType() instanceof MapType and
88-
(node2.(Read).readsElement(node1, _) or node2.(RangeElementNode).getBase() = node1)
8993
}

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/sinks.expected

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,27 +10,35 @@ invalidModelRow
1010
| test.go:79:10:79:15 | taint6 | qltest |
1111
| test.go:82:10:82:15 | taint7 | qltest |
1212
| test.go:85:10:85:18 | index expression | qltest |
13-
| test.go:89:10:89:15 | taint9 | qltest |
14-
| test.go:92:10:92:33 | call to GetElement | qltest |
15-
| test.go:93:10:93:18 | <-... | qltest |
16-
| test.go:97:10:97:16 | taint11 | qltest |
17-
| test.go:100:10:100:32 | call to GetMapKey | qltest |
18-
| test.go:102:11:102:11 | k | qltest |
19-
| test.go:105:11:105:11 | k | qltest |
20-
| test.go:110:10:110:16 | taint13 | qltest |
21-
| test.go:113:10:113:20 | index expression | qltest |
22-
| test.go:117:10:117:16 | taint15 | qltest |
23-
| test.go:121:10:121:17 | index expression | qltest |
24-
| test.go:127:10:127:18 | index expression | qltest |
25-
| test.go:132:10:132:16 | taint16 | qltest |
26-
| test.go:136:10:136:13 | selection of F | qltest |
27-
| test.go:139:10:139:17 | call to Get | qltest |
28-
| test.go:143:10:143:17 | call to Get | qltest |
29-
| test.go:148:10:148:17 | call to Get | qltest |
30-
| test.go:152:10:152:14 | selection of F | qltest |
31-
| test.go:155:10:155:32 | call to GetThroughPointer | qltest |
32-
| test.go:159:10:159:32 | call to GetThroughPointer | qltest |
33-
| test.go:164:10:164:32 | call to GetThroughPointer | qltest |
34-
| test.go:170:17:170:20 | arg1 | qltest |
35-
| test.go:170:23:170:26 | arg2 | qltest |
36-
| test.go:170:29:170:32 | arg3 | qltest |
13+
| test.go:87:11:87:11 | x | qltest |
14+
| test.go:90:11:90:11 | x | qltest |
15+
| test.go:95:10:95:15 | taint9 | qltest |
16+
| test.go:98:10:98:33 | call to GetElement | qltest |
17+
| test.go:99:10:99:18 | <-... | qltest |
18+
| test.go:101:11:101:11 | e | qltest |
19+
| test.go:104:11:104:11 | e | qltest |
20+
| test.go:109:10:109:16 | taint11 | qltest |
21+
| test.go:112:10:112:32 | call to GetMapKey | qltest |
22+
| test.go:114:11:114:11 | k | qltest |
23+
| test.go:117:11:117:11 | k | qltest |
24+
| test.go:120:11:120:11 | k | qltest |
25+
| test.go:123:11:123:11 | k | qltest |
26+
| test.go:128:10:128:16 | taint13 | qltest |
27+
| test.go:131:10:131:20 | index expression | qltest |
28+
| test.go:133:11:133:11 | v | qltest |
29+
| test.go:136:11:136:11 | v | qltest |
30+
| test.go:141:10:141:16 | taint15 | qltest |
31+
| test.go:145:10:145:17 | index expression | qltest |
32+
| test.go:151:10:151:18 | index expression | qltest |
33+
| test.go:156:10:156:16 | taint16 | qltest |
34+
| test.go:160:10:160:13 | selection of F | qltest |
35+
| test.go:163:10:163:17 | call to Get | qltest |
36+
| test.go:167:10:167:17 | call to Get | qltest |
37+
| test.go:172:10:172:17 | call to Get | qltest |
38+
| test.go:176:10:176:14 | selection of F | qltest |
39+
| test.go:179:10:179:32 | call to GetThroughPointer | qltest |
40+
| test.go:183:10:183:32 | call to GetThroughPointer | qltest |
41+
| test.go:188:10:188:32 | call to GetThroughPointer | qltest |
42+
| test.go:194:17:194:20 | arg1 | qltest |
43+
| test.go:194:23:194:26 | arg2 | qltest |
44+
| test.go:194:29:194:32 | arg3 | qltest |

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/srcs.expected

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ invalidModelRow
99
| test.go:41:2:41:21 | ... = ...[1] | qltest-w-subtypes |
1010
| test.go:42:2:42:22 | ... = ...[1] | qltest-w-subtypes |
1111
| test.go:58:9:58:16 | call to Src1 | qltest |
12-
| test.go:91:46:91:53 | call to Src1 | qltest |
13-
| test.go:95:35:95:42 | call to Src1 | qltest |
14-
| test.go:99:42:99:49 | call to Src1 | qltest |
15-
| test.go:130:8:130:15 | call to Src1 | qltest |
16-
| test.go:135:9:135:16 | call to Src1 | qltest |
17-
| test.go:138:15:138:22 | call to Src1 | qltest |
18-
| test.go:142:9:142:16 | call to Src1 | qltest |
19-
| test.go:146:9:146:16 | call to Src1 | qltest |
20-
| test.go:151:24:151:31 | call to Src1 | qltest |
21-
| test.go:154:17:154:24 | call to Src1 | qltest |
22-
| test.go:158:24:158:31 | call to Src1 | qltest |
23-
| test.go:162:24:162:31 | call to Src1 | qltest |
12+
| test.go:97:46:97:53 | call to Src1 | qltest |
13+
| test.go:107:35:107:42 | call to Src1 | qltest |
14+
| test.go:111:42:111:49 | call to Src1 | qltest |
15+
| test.go:154:8:154:15 | call to Src1 | qltest |
16+
| test.go:159:9:159:16 | call to Src1 | qltest |
17+
| test.go:162:15:162:22 | call to Src1 | qltest |
18+
| test.go:166:9:166:16 | call to Src1 | qltest |
19+
| test.go:170:9:170:16 | call to Src1 | qltest |
20+
| test.go:175:24:175:31 | call to Src1 | qltest |
21+
| test.go:178:17:178:24 | call to Src1 | qltest |
22+
| test.go:182:24:182:31 | call to Src1 | qltest |
23+
| test.go:186:24:186:31 | call to Src1 | qltest |

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalTaintFlow/test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ func simpleflow() {
8383

8484
taint8 := test.StepArgResArrayContent(src)
8585
b.Sink1(taint8[0]) // $ hasTaintFlow="index expression"
86+
for _, x := range taint8 {
87+
b.Sink1(x) // $ hasTaintFlow="x"
88+
}
89+
for _, x := range arraytype(taint8) {
90+
b.Sink1(x) // $ hasTaintFlow="x"
91+
}
8692

8793
srcArray := []interface{}{nil, src}
8894
taint9 := test.StepArgArrayContentRes(srcArray)
@@ -91,6 +97,12 @@ func simpleflow() {
9197
taint10 := test.StepArgResCollectionContent(a.Src1()).(chan interface{})
9298
b.Sink1(test.GetElement(taint10)) // $ hasTaintFlow="call to GetElement"
9399
b.Sink1(<-taint10) // $ hasTaintFlow="<-..."
100+
for e := range taint10 {
101+
b.Sink1(e) // $ MISSING: hasTaintFlow="e"
102+
}
103+
for e := range channeltype(taint10) {
104+
b.Sink1(e) // $ MISSING: hasTaintFlow="e"
105+
}
94106

95107
srcCollection := test.SetElement(a.Src1())
96108
taint11 := test.StepArgCollectionContentRes(srcCollection)
@@ -104,13 +116,25 @@ func simpleflow() {
104116
for k := range taint12 {
105117
b.Sink1(k) // $ hasTaintFlow="k"
106118
}
119+
for k, _ := range mapstringstringtype(taint12) {
120+
b.Sink1(k) // $ hasTaintFlow="k"
121+
}
122+
for k := range mapstringstringtype(taint12) {
123+
b.Sink1(k) // $ hasTaintFlow="k"
124+
}
107125

108126
srcMap13 := map[string]string{src.(string): ""}
109127
taint13 := test.StepArgMapKeyContentRes(srcMap13)
110128
b.Sink1(taint13) // $ hasTaintFlow="taint13"
111129

112130
taint14 := test.StepArgResMapValueContent(src).(map[string]string)
113131
b.Sink1(taint14[""]) // $ hasTaintFlow="index expression"
132+
for _, v := range taint14 {
133+
b.Sink1(v) // $ hasTaintFlow="v"
134+
}
135+
for _, v := range mapstringstringtype(taint14) {
136+
b.Sink1(v) // $ hasTaintFlow="v"
137+
}
114138

115139
srcMap15 := map[string]string{"": src.(string)}
116140
taint15 := test.StepArgMapValueContentRes(srcMap15)
@@ -169,3 +193,7 @@ func simpleflow() {
169193
arg4 := src
170194
b.SinkManyArgs(arg1, arg2, arg3, arg4) // $ hasTaintFlow="arg1" hasTaintFlow="arg2" hasTaintFlow="arg3"
171195
}
196+
197+
type mapstringstringtype map[string]string
198+
type arraytype []interface{}
199+
type channeltype chan interface{}

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalValueFlow/sinks.expected

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -10,33 +10,41 @@ invalidModelRow
1010
| test.go:79:10:79:15 | taint6 | qltest |
1111
| test.go:82:10:82:15 | taint7 | qltest |
1212
| test.go:85:10:85:18 | index expression | qltest |
13-
| test.go:89:10:89:15 | taint9 | qltest |
14-
| test.go:92:10:92:33 | call to GetElement | qltest |
15-
| test.go:93:10:93:18 | <-... | qltest |
16-
| test.go:97:10:97:16 | taint11 | qltest |
17-
| test.go:100:10:100:32 | call to GetMapKey | qltest |
18-
| test.go:102:11:102:11 | k | qltest |
19-
| test.go:105:11:105:11 | k | qltest |
20-
| test.go:110:10:110:16 | taint13 | qltest |
21-
| test.go:113:10:113:20 | index expression | qltest |
22-
| test.go:117:10:117:16 | taint15 | qltest |
23-
| test.go:121:10:121:17 | index expression | qltest |
24-
| test.go:127:10:127:18 | index expression | qltest |
25-
| test.go:132:10:132:16 | taint16 | qltest |
26-
| test.go:136:10:136:13 | selection of F | qltest |
27-
| test.go:139:10:139:17 | call to Get | qltest |
28-
| test.go:143:10:143:17 | call to Get | qltest |
29-
| test.go:148:10:148:17 | call to Get | qltest |
30-
| test.go:152:10:152:14 | selection of F | qltest |
31-
| test.go:155:10:155:32 | call to GetThroughPointer | qltest |
32-
| test.go:159:10:159:32 | call to GetThroughPointer | qltest |
33-
| test.go:164:10:164:32 | call to GetThroughPointer | qltest |
34-
| test.go:170:17:170:20 | arg1 | qltest |
35-
| test.go:170:23:170:26 | arg2 | qltest |
36-
| test.go:170:29:170:32 | arg3 | qltest |
37-
| test.go:173:10:173:26 | call to max | qltest |
38-
| test.go:174:10:174:26 | call to max | qltest |
39-
| test.go:175:10:175:26 | call to max | qltest |
40-
| test.go:176:10:176:26 | call to min | qltest |
41-
| test.go:177:10:177:26 | call to min | qltest |
42-
| test.go:178:10:178:26 | call to min | qltest |
13+
| test.go:87:11:87:11 | x | qltest |
14+
| test.go:90:11:90:11 | x | qltest |
15+
| test.go:95:10:95:15 | taint9 | qltest |
16+
| test.go:98:10:98:33 | call to GetElement | qltest |
17+
| test.go:99:10:99:18 | <-... | qltest |
18+
| test.go:101:11:101:11 | e | qltest |
19+
| test.go:104:11:104:11 | e | qltest |
20+
| test.go:109:10:109:16 | taint11 | qltest |
21+
| test.go:112:10:112:32 | call to GetMapKey | qltest |
22+
| test.go:114:11:114:11 | k | qltest |
23+
| test.go:117:11:117:11 | k | qltest |
24+
| test.go:120:11:120:11 | k | qltest |
25+
| test.go:123:11:123:11 | k | qltest |
26+
| test.go:128:10:128:16 | taint13 | qltest |
27+
| test.go:131:10:131:20 | index expression | qltest |
28+
| test.go:133:11:133:11 | v | qltest |
29+
| test.go:136:11:136:11 | v | qltest |
30+
| test.go:141:10:141:16 | taint15 | qltest |
31+
| test.go:145:10:145:17 | index expression | qltest |
32+
| test.go:151:10:151:18 | index expression | qltest |
33+
| test.go:156:10:156:16 | taint16 | qltest |
34+
| test.go:160:10:160:13 | selection of F | qltest |
35+
| test.go:163:10:163:17 | call to Get | qltest |
36+
| test.go:167:10:167:17 | call to Get | qltest |
37+
| test.go:172:10:172:17 | call to Get | qltest |
38+
| test.go:176:10:176:14 | selection of F | qltest |
39+
| test.go:179:10:179:32 | call to GetThroughPointer | qltest |
40+
| test.go:183:10:183:32 | call to GetThroughPointer | qltest |
41+
| test.go:188:10:188:32 | call to GetThroughPointer | qltest |
42+
| test.go:194:17:194:20 | arg1 | qltest |
43+
| test.go:194:23:194:26 | arg2 | qltest |
44+
| test.go:194:29:194:32 | arg3 | qltest |
45+
| test.go:197:10:197:26 | call to max | qltest |
46+
| test.go:198:10:198:26 | call to max | qltest |
47+
| test.go:199:10:199:26 | call to max | qltest |
48+
| test.go:200:10:200:26 | call to min | qltest |
49+
| test.go:201:10:201:26 | call to min | qltest |
50+
| test.go:202:10:202:26 | call to min | qltest |

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalValueFlow/srcs.expected

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ invalidModelRow
99
| test.go:41:2:41:21 | ... = ...[1] | qltest-w-subtypes |
1010
| test.go:42:2:42:22 | ... = ...[1] | qltest-w-subtypes |
1111
| test.go:58:9:58:16 | call to Src1 | qltest |
12-
| test.go:91:46:91:53 | call to Src1 | qltest |
13-
| test.go:95:35:95:42 | call to Src1 | qltest |
14-
| test.go:99:42:99:49 | call to Src1 | qltest |
15-
| test.go:130:8:130:15 | call to Src1 | qltest |
16-
| test.go:135:9:135:16 | call to Src1 | qltest |
17-
| test.go:138:15:138:22 | call to Src1 | qltest |
18-
| test.go:142:9:142:16 | call to Src1 | qltest |
19-
| test.go:146:9:146:16 | call to Src1 | qltest |
20-
| test.go:151:24:151:31 | call to Src1 | qltest |
21-
| test.go:154:17:154:24 | call to Src1 | qltest |
22-
| test.go:158:24:158:31 | call to Src1 | qltest |
23-
| test.go:162:24:162:31 | call to Src1 | qltest |
12+
| test.go:97:46:97:53 | call to Src1 | qltest |
13+
| test.go:107:35:107:42 | call to Src1 | qltest |
14+
| test.go:111:42:111:49 | call to Src1 | qltest |
15+
| test.go:154:8:154:15 | call to Src1 | qltest |
16+
| test.go:159:9:159:16 | call to Src1 | qltest |
17+
| test.go:162:15:162:22 | call to Src1 | qltest |
18+
| test.go:166:9:166:16 | call to Src1 | qltest |
19+
| test.go:170:9:170:16 | call to Src1 | qltest |
20+
| test.go:175:24:175:31 | call to Src1 | qltest |
21+
| test.go:178:17:178:24 | call to Src1 | qltest |
22+
| test.go:182:24:182:31 | call to Src1 | qltest |
23+
| test.go:186:24:186:31 | call to Src1 | qltest |

0 commit comments

Comments
 (0)