Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 349518b

Browse filesBrowse files
authored
Merge pull request #17618 from owen-mc/go/mad/subtypes-promoted-methods
Go: Make the models-as-data subtypes column do something more sensible for promoted methods
2 parents 7517ad3 + fd4a6d4 commit 349518b
Copy full SHA for 349518b

File tree

Expand file treeCollapse file tree

73 files changed

+1125
-271
lines changed
Filter options

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Dismiss banner
Expand file treeCollapse file tree

73 files changed

+1125
-271
lines changed
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* The behaviour of the `subtypes` column in models-as-data now matches other languages more closely.

‎go/ql/lib/semmle/go/dataflow/ExternalFlow.qll

Copy file name to clipboardExpand all lines: go/ql/lib/semmle/go/dataflow/ExternalFlow.qll
+20-9Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@
2525
* packages in the group `<groupname>` according to the `packageGrouping`
2626
* predicate.
2727
* 2. The `type` column selects a type within that package.
28-
* 3. The `subtypes` is a boolean that indicates whether to jump to an
29-
* arbitrary subtype of that type.
28+
* 3. The `subtypes` column is a boolean that controls what restrictions we
29+
* place on the type `t` of the selector base when accessing a field or
30+
* calling a method. When it is false, `t` must be the exact type specified
31+
* by this row. When it is true, `t` may be a type which embeds the specified
32+
* type, and for interface methods `t` may be a type which implements the
33+
* interface.
3034
* 4. The `name` column optionally selects a specific named member of the type.
3135
* 5. The `signature` column is always empty.
3236
* 6. The `ext` column is always empty.
@@ -470,17 +474,24 @@ SourceSinkInterpretationInput::SourceOrSinkElement interpretElement(
470474
elementSpec(pkg, type, subtypes, name, signature, ext) and
471475
// Go does not need to distinguish functions with signature
472476
signature = "" and
473-
(
474-
exists(Field f | f.hasQualifiedName(interpretPackage(pkg), type, name) | result.asEntity() = f)
477+
exists(string p | p = interpretPackage(pkg) |
478+
exists(Entity e | result.hasFullInfo(e, p, type, subtypes) |
479+
e.(Field).hasQualifiedName(p, type, name) or
480+
e.(Method).hasQualifiedName(p, type, name)
481+
)
475482
or
476-
exists(Method m | m.hasQualifiedName(interpretPackage(pkg), type, name) |
477-
result.asEntity() = m
478-
or
479-
subtypes = true and result.asEntity().(Method).implementsIncludingInterfaceMethods(m)
483+
subtypes = true and
484+
// p.type is an interface and we include types which implement it
485+
exists(Method m2, string pkg2, string type2 |
486+
m2.getReceiverType().implements(p, type) and
487+
m2.getName() = name and
488+
m2.getReceiverBaseType().hasQualifiedName(pkg2, type2)
489+
|
490+
result.hasFullInfo(m2, pkg2, type2, subtypes)
480491
)
481492
or
482493
type = "" and
483-
exists(Entity e | e.hasQualifiedName(interpretPackage(pkg), name) | result.asEntity() = e)
494+
exists(Entity e | e.hasQualifiedName(p, name) | result.asOtherEntity() = e)
484495
)
485496
}
486497

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

Copy file name to clipboardExpand all lines: go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll
+165-9Lines changed: 165 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,63 @@ module SourceSinkInterpretationInput implements
149149
)
150150
}
151151

152+
// Note that due to embedding, which is currently implemented via some
153+
// Methods having multiple qualified names, a given Method is liable to have
154+
// more than one SourceOrSinkElement, one for each of the names it claims.
152155
private newtype TSourceOrSinkElement =
153-
TEntityElement(Entity e) or
156+
TMethodEntityElement(Method m, string pkg, string type, boolean subtypes) {
157+
m.hasQualifiedName(pkg, type, _) and
158+
subtypes = [true, false]
159+
} or
160+
TFieldEntityElement(Field f, string pkg, string type, boolean subtypes) {
161+
f.hasQualifiedName(pkg, type, _) and
162+
subtypes = [true, false]
163+
} or
164+
TOtherEntityElement(Entity e) {
165+
not e instanceof Method and
166+
not e instanceof Field
167+
} or
154168
TAstElement(AstNode n)
155169

156170
/** An element representable by CSV modeling. */
157171
class SourceOrSinkElement extends TSourceOrSinkElement {
158172
/** Gets this source or sink element as an entity, if it is one. */
159-
Entity asEntity() { this = TEntityElement(result) }
173+
Entity asEntity() {
174+
result = [this.asMethodEntity(), this.asFieldEntity(), this.asOtherEntity()]
175+
}
176+
177+
/** Gets this source or sink element as a method, if it is one. */
178+
Method asMethodEntity() { this = TMethodEntityElement(result, _, _, _) }
179+
180+
/** Gets this source or sink element as a field, if it is one. */
181+
Field asFieldEntity() { this = TFieldEntityElement(result, _, _, _) }
182+
183+
/** Gets this source or sink element as an entity which isn't a field or method, if it is one. */
184+
Entity asOtherEntity() { this = TOtherEntityElement(result) }
160185

161186
/** Gets this source or sink element as an AST node, if it is one. */
162187
AstNode asAstNode() { this = TAstElement(result) }
163188

189+
/**
190+
* Holds if this source or sink element is a method or field that was specified
191+
* with the given values for `e`, `pkg`, `type` and `subtypes`.
192+
*/
193+
predicate hasFullInfo(Entity e, string pkg, string type, boolean subtypes) {
194+
this = TMethodEntityElement(e, pkg, type, subtypes) or
195+
this = TFieldEntityElement(e, pkg, type, subtypes)
196+
}
197+
164198
/** Gets a textual representation of this source or sink element. */
165199
string toString() {
200+
(this instanceof TOtherEntityElement or this instanceof TAstElement) and
166201
result = "element representing " + [this.asEntity().toString(), this.asAstNode().toString()]
202+
or
203+
exists(Entity e, string pkg, string name, boolean subtypes |
204+
this.hasFullInfo(e, pkg, name, subtypes) and
205+
result =
206+
"element representing " + e.toString() + " with receiver type " + pkg + "." + name +
207+
" and subtypes=" + subtypes
208+
)
167209
}
168210

169211
/** Gets the location of this element. */
@@ -203,7 +245,17 @@ module SourceSinkInterpretationInput implements
203245

204246
/** Gets the target of this call, if any. */
205247
SourceOrSinkElement getCallTarget() {
206-
result.asEntity() = this.asCall().getNode().(DataFlow::CallNode).getTarget()
248+
exists(DataFlow::CallNode cn, Function callTarget |
249+
cn = this.asCall().getNode() and
250+
callTarget = cn.getTarget()
251+
|
252+
(
253+
result.asOtherEntity() = callTarget
254+
or
255+
callTarget instanceof Method and
256+
result = getElementWithQualifier(callTarget, cn.getReceiver())
257+
)
258+
)
207259
}
208260

209261
/** Gets a textual representation of this node. */
@@ -228,6 +280,105 @@ module SourceSinkInterpretationInput implements
228280
}
229281
}
230282

283+
/**
284+
* Gets a method or field spec for `e` which applies in the context of
285+
* qualifier `qual`.
286+
*
287+
* Note that naively checking `e`'s qualified name is not correct, because
288+
* `Method`s and `Field`s may have multiple qualified names due to embedding.
289+
* We must instead check that the package and type name given by
290+
* `result.hasFullInfo` refer to either `qual`'s type or to a type it embeds.
291+
*/
292+
bindingset[e, qual]
293+
pragma[inline_late]
294+
private SourceOrSinkElement getElementWithQualifier(Entity e, DataFlow::Node qual) {
295+
exists(boolean subtypes, Type syntacticQualBaseType, Type targetType |
296+
syntacticQualBaseType = getSyntacticQualifierBaseType(qual) and
297+
result = constructElement(e, targetType, subtypes)
298+
|
299+
subtypes = [true, false] and
300+
syntacticQualBaseType = targetType
301+
or
302+
subtypes = true and
303+
(
304+
// `syntacticQualBaseType`'s underlying type might be an interface type and `sse`
305+
// might refer to a method defined on an interface embedded within it.
306+
targetType =
307+
syntacticQualBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface()
308+
or
309+
// `syntacticQualBaseType`'s underlying type might be a struct type and `sse`
310+
// might be a promoted method or field in it.
311+
targetType = getAnIntermediateEmbeddedType(e, syntacticQualBaseType.getUnderlyingType())
312+
)
313+
)
314+
}
315+
316+
bindingset[e, targetType, subtypes]
317+
pragma[inline_late]
318+
private SourceOrSinkElement constructElement(Entity e, Type targetType, boolean subtypes) {
319+
exists(string pkg, string typename |
320+
targetType.hasQualifiedName(pkg, typename) and
321+
result.hasFullInfo(e, pkg, typename, subtypes)
322+
)
323+
}
324+
325+
/**
326+
* Gets the type of an embedded field of `st` which is on the path to `e`,
327+
* which is a promoted method or field of `st`, or its base type if it's a
328+
* pointer type.
329+
*/
330+
private Type getAnIntermediateEmbeddedType(Entity e, StructType st) {
331+
exists(Field field1, Field field2, int depth1, int depth2, Type t2 |
332+
field1 = st.getFieldAtDepth(_, depth1) and
333+
field2 = st.getFieldAtDepth(_, depth2) and
334+
result = lookThroughPointerType(field1.getType()) and
335+
t2 = lookThroughPointerType(field2.getType()) and
336+
(
337+
field1 = field2
338+
or
339+
field2 = result.getUnderlyingType().(StructType).getFieldAtDepth(_, depth2 - depth1 - 1)
340+
)
341+
|
342+
e.(Method).getReceiverBaseType() = t2
343+
or
344+
e.(Field).getDeclaringType() = t2.getUnderlyingType()
345+
)
346+
}
347+
348+
/**
349+
* Gets the base type of `underlying`, where `n` is of the form
350+
* `implicitDeref?(underlying.implicitFieldRead1.implicitFieldRead2...)`
351+
*
352+
* For Go syntax like `qualifier.method()` or `qualifier.field`, this is the type of `qualifier`, before any
353+
* implicit dereference is interposed because `qualifier` is of pointer type, or implicit field accesses
354+
* navigate to any embedded struct types that truly host `field`.
355+
*/
356+
private Type getSyntacticQualifierBaseType(DataFlow::Node n) {
357+
exists(DataFlow::Node n2 |
358+
// look through implicit dereference, if there is one
359+
not exists(n.asInstruction().(IR::EvalImplicitDerefInstruction).getOperand()) and
360+
n2 = n
361+
or
362+
n2.asExpr() = n.asInstruction().(IR::EvalImplicitDerefInstruction).getOperand()
363+
|
364+
result = lookThroughPointerType(skipImplicitFieldReads(n2).getType())
365+
)
366+
}
367+
368+
private DataFlow::Node skipImplicitFieldReads(DataFlow::Node n) {
369+
not exists(lookThroughImplicitFieldRead(n)) and result = n
370+
or
371+
result = skipImplicitFieldReads(lookThroughImplicitFieldRead(n))
372+
}
373+
374+
private DataFlow::Node lookThroughImplicitFieldRead(DataFlow::Node n) {
375+
result.asInstruction() =
376+
n.(DataFlow::InstructionNode)
377+
.asInstruction()
378+
.(IR::ImplicitFieldReadInstruction)
379+
.getBaseInstruction()
380+
}
381+
231382
/** Provides additional sink specification logic. */
232383
bindingset[c]
233384
predicate interpretOutput(string c, InterpretNode mid, InterpretNode node) {
@@ -242,10 +393,12 @@ module SourceSinkInterpretationInput implements
242393
e = mid.asElement()
243394
|
244395
(c = "Parameter" or c = "") and
245-
node.asNode().asParameter() = e.asEntity()
396+
n.asParameter() = pragma[only_bind_into](e).asEntity()
246397
or
247-
c = "" and
248-
n.(DataFlow::FieldReadNode).getField() = e.asEntity()
398+
exists(DataFlow::FieldReadNode frn | frn = n |
399+
c = "" and
400+
pragma[only_bind_into](e) = getElementWithQualifier(frn.getField(), frn.getBase())
401+
)
249402
)
250403
}
251404

@@ -259,10 +412,13 @@ module SourceSinkInterpretationInput implements
259412
mid.asCallable() = getNodeEnclosingCallable(ret)
260413
)
261414
or
262-
exists(DataFlow::Write fw, Field f |
415+
exists(SourceOrSinkElement e, DataFlow::Write fw, DataFlow::Node base, Field f |
416+
e = mid.asElement() and
417+
f = e.asFieldEntity()
418+
|
263419
c = "" and
264-
f = mid.asElement().asEntity() and
265-
fw.writesField(_, f, node.asNode())
420+
fw.writesField(base, f, node.asNode()) and
421+
pragma[only_bind_into](e) = getElementWithQualifier(f, base)
266422
)
267423
}
268424
}

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i1_subtypes_false.ext.yml renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_false.ext.yml

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_false.ext.yml
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ extensions:
33
pack: codeql/go-all
44
extensible: sourceModel
55
data:
6-
- ["github.com/nonexistent/test", "I1", False, "Source", "", "", "ReturnValue", "remote", "manual"]
6+
- ["github.com/nonexistent/test", "I1", False, "Source", "", "", "ReturnValue", "qltest", "manual"]
77
- addsTo:
88
pack: codeql/go-all
99
extensible: summaryModel
@@ -13,4 +13,4 @@ extensions:
1313
pack: codeql/go-all
1414
extensible: sinkModel
1515
data:
16-
- ["github.com/nonexistent/test", "I1", False, "Sink", "", "", "Argument[0]", "path-injection", "manual"]
16+
- ["github.com/nonexistent/test", "I1", False, "Sink", "", "", "Argument[0]", "qltest", "manual"]

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i1_subtypes_false.ql renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_false.ql

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_false.ql
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import go
2-
import semmle.go.dataflow.ExternalFlow
32
import ModelValidation
4-
import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
53
import TestUtilities.InlineExpectationsTest
64
import MakeTest<FlowTest>
75

86
module Config implements DataFlow::ConfigSig {
9-
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
7+
predicate isSource(DataFlow::Node source) { sourceNode(source, "qltest") }
108

11-
predicate isSink(DataFlow::Node sink) { sink = any(FileSystemAccess fsa).getAPathArgument() }
9+
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "qltest") }
1210
}
1311

1412
module Flow = TaintTracking::Global<Config>;

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i1_subtypes_true.ext.yml renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_true.ext.yml

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_true.ext.yml
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ extensions:
33
pack: codeql/go-all
44
extensible: sourceModel
55
data:
6-
- ["github.com/nonexistent/test", "I1", True, "Source", "", "", "ReturnValue", "remote", "manual"]
6+
- ["github.com/nonexistent/test", "I1", True, "Source", "", "", "ReturnValue", "qltest", "manual"]
77
- addsTo:
88
pack: codeql/go-all
99
extensible: summaryModel
@@ -13,4 +13,4 @@ extensions:
1313
pack: codeql/go-all
1414
extensible: sinkModel
1515
data:
16-
- ["github.com/nonexistent/test", "I1", True, "Sink", "", "", "Argument[0]", "path-injection", "manual"]
16+
- ["github.com/nonexistent/test", "I1", True, "Sink", "", "", "Argument[0]", "qltest", "manual"]

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i1_subtypes_true.ql renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_true.ql

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I1_subtypes_true.ql
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import go
2-
import semmle.go.dataflow.ExternalFlow
32
import ModelValidation
4-
import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
53
import TestUtilities.InlineExpectationsTest
64
import MakeTest<FlowTest>
75

86
module Config implements DataFlow::ConfigSig {
9-
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
7+
predicate isSource(DataFlow::Node source) { sourceNode(source, "qltest") }
108

11-
predicate isSink(DataFlow::Node sink) { sink = any(FileSystemAccess fsa).getAPathArgument() }
9+
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "qltest") }
1210
}
1311

1412
module Flow = TaintTracking::Global<Config>;

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i2_subtypes_false.ext.yml renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_false.ext.yml

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_false.ext.yml
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ extensions:
33
pack: codeql/go-all
44
extensible: sourceModel
55
data:
6-
- ["github.com/nonexistent/test", "I2", False, "Source", "", "", "ReturnValue", "remote", "manual"]
6+
- ["github.com/nonexistent/test", "I2", False, "Source", "", "", "ReturnValue", "qltest", "manual"]
77
- addsTo:
88
pack: codeql/go-all
99
extensible: summaryModel
@@ -13,4 +13,4 @@ extensions:
1313
pack: codeql/go-all
1414
extensible: sinkModel
1515
data:
16-
- ["github.com/nonexistent/test", "I2", False, "Sink", "", "", "Argument[0]", "path-injection", "manual"]
16+
- ["github.com/nonexistent/test", "I2", False, "Sink", "", "", "Argument[0]", "qltest", "manual"]

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i2_subtypes_false.ql renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_false.ql

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_false.ql
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import go
2-
import semmle.go.dataflow.ExternalFlow
32
import ModelValidation
4-
import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
53
import TestUtilities.InlineExpectationsTest
64
import MakeTest<FlowTest>
75

86
module Config implements DataFlow::ConfigSig {
9-
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
7+
predicate isSource(DataFlow::Node source) { sourceNode(source, "qltest") }
108

11-
predicate isSink(DataFlow::Node sink) { sink = any(FileSystemAccess fsa).getAPathArgument() }
9+
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "qltest") }
1210
}
1311

1412
module Flow = TaintTracking::Global<Config>;

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_i2_subtypes_true.ext.yml renamed to ‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_true.ext.yml

Copy file name to clipboardExpand all lines: go/ql/test/library-tests/semmle/go/dataflow/ExternalFlowInheritance/mad_I2_subtypes_true.ext.yml
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ extensions:
33
pack: codeql/go-all
44
extensible: sourceModel
55
data:
6-
- ["github.com/nonexistent/test", "I2", True, "Source", "", "", "ReturnValue", "remote", "manual"]
6+
- ["github.com/nonexistent/test", "I2", True, "Source", "", "", "ReturnValue", "qltest", "manual"]
77
- addsTo:
88
pack: codeql/go-all
99
extensible: summaryModel
@@ -13,4 +13,4 @@ extensions:
1313
pack: codeql/go-all
1414
extensible: sinkModel
1515
data:
16-
- ["github.com/nonexistent/test", "I2", True, "Sink", "", "", "Argument[0]", "path-injection", "manual"]
16+
- ["github.com/nonexistent/test", "I2", True, "Sink", "", "", "Argument[0]", "qltest", "manual"]

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.