From e1b4867a197df2e44ddddb21897dc2b27aff8449 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 10 Mar 2021 04:48:59 -0800 Subject: [PATCH] Refactor embedded field calculation to expose access chain This allows us to reuse the embedded field calculation in the logic for generating implicit field selection nodes. --- ql/src/semmle/go/Types.qll | 65 +++++++++++-------- .../go/controlflow/ControlFlowGraphImpl.qll | 28 ++++---- .../semmle/go/Types/Field_getPackage.expected | 5 ++ .../go/Types/Field_hasQualifiedName2.expected | 6 ++ .../go/Types/Field_hasQualifiedName3.expected | 6 ++ .../semmle/go/Types/MethodTypes.expected | 1 + .../semmle/go/Types/QualifiedNames.expected | 4 ++ .../semmle/go/Types/StructFields.expected | 6 ++ .../semmle/go/Types/Types.expected | 4 ++ .../library-tests/semmle/go/Types/cyclic.go | 9 +++ .../library-tests/semmle/go/Types/embedded.go | 15 +++++ 11 files changed, 107 insertions(+), 42 deletions(-) create mode 100644 ql/test/library-tests/semmle/go/Types/embedded.go diff --git a/ql/src/semmle/go/Types.qll b/ql/src/semmle/go/Types.qll index a24ec85c852..cd51783237a 100644 --- a/ql/src/semmle/go/Types.qll +++ b/ql/src/semmle/go/Types.qll @@ -375,39 +375,38 @@ class StructType extends @structtype, CompositeType { this.hasOwnField(_, name, _, isEmbedded) } + /** + * hasEmbeddedField holds if there is an embedded field at int `depth`, with either type `tp` or `tp`'s pointer type. + */ private predicate hasEmbeddedField(Type tp, int depth) { - hasFieldOrMethodCand(_, tp, depth, true, false) - or - exists(PointerType embeddedPtr | - hasFieldOrMethodCand(_, embeddedPtr, depth, true, false) and - tp = embeddedPtr.getBaseType() + exists(Field f | this.hasFieldCand(_, f, depth, true) | tp = f.getType().getBaseType*()) + } + + /** + * getFieldOfEmbedded gets a field of `embeddedParent`, which is then embedded into this struct type. + */ + Field getFieldOfEmbedded(Field embeddedParent, string name, int depth, boolean isEmbedded) { + // embeddedParent is a field of 'this' at depth 'depth - 1' + this.hasFieldCand(_, embeddedParent, depth - 1, true) and + // embeddedParent's type has the result field + exists(StructType embeddedType | + embeddedType = embeddedParent.getType().getBaseType*().getUnderlyingType() + | + result = embeddedType.getOwnField(name, isEmbedded) ) } - private predicate hasFieldOrMethodCand( - string name, Type tp, int depth, boolean isEmbedded, boolean isMethod - ) { - hasOwnField(_, name, tp, isEmbedded) and depth = 0 and isMethod = false + private predicate hasFieldCand(string name, Field f, int depth, boolean isEmbedded) { + f = this.getOwnField(name, isEmbedded) and depth = 0 or - not hasOwnField(_, name, _, _) and - exists(Type embedded | hasEmbeddedField(embedded, depth - 1) | - embedded.getUnderlyingType().(StructType).hasOwnField(_, name, tp, isEmbedded) and - isMethod = false - or - exists(MethodDecl md | md.getReceiverType() = embedded | - name = md.getName() and - tp = md.getType() - ) and - isEmbedded = false and - isMethod = true - ) + not this.hasOwnField(_, name, _, _) and + f = this.getFieldOfEmbedded(_, name, depth, isEmbedded) } - private predicate hasFieldOrMethod(string name, Type tp, boolean isMethod) { - exists(int mindepth | - mindepth = min(int depth | hasFieldOrMethodCand(name, _, depth, _, _)) and - hasFieldOrMethodCand(name, tp, mindepth, _, isMethod) and - (strictcount(getFieldCand(name, mindepth, _)) = 1 or isMethod = true) + private predicate hasMethodCand(string name, Method m, int depth) { + name = m.getName() and + exists(Type embedded | this.hasEmbeddedField(embedded, depth - 1) | + m.getReceiverType() = embedded ) } @@ -415,7 +414,12 @@ class StructType extends @structtype, CompositeType { * Holds if this struct contains a field `name` with type `tp`, possibly inside a (nested) * embedded field. */ - predicate hasField(string name, Type tp) { hasFieldOrMethod(name, tp, false) } + predicate hasField(string name, Type tp) { + exists(int mindepth | + mindepth = min(int depth | this.hasFieldCand(name, _, depth, _)) and + tp = unique(Field f | f = this.getFieldCand(name, mindepth, _)).getType() + ) + } private Field getFieldCand(string name, int depth, boolean isEmbedded) { result = this.getOwnField(name, isEmbedded) and depth = 0 @@ -441,7 +445,12 @@ class StructType extends @structtype, CompositeType { strictcount(getFieldCand(name, depth, _)) = 1 } - override predicate hasMethod(string name, SignatureType tp) { hasFieldOrMethod(name, tp, true) } + override predicate hasMethod(string name, SignatureType tp) { + exists(int mindepth | + mindepth = min(int depth | this.hasMethodCand(name, _, depth)) and + tp = unique(Method m | this.hasMethodCand(name, m, mindepth)).getType() + ) + } language[monotonicAggregates] override string pp() { diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index 486072c57cf..fd4657d8741 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -48,6 +48,19 @@ private predicate isCond(Expr e) { e = any(ParenExpr par | isCond(par)).getExpr() } +private predicate implicitFieldSelection(SelectorExpr e, int i, Field implicitField) { + exists(StructType baseType, Field child | + baseType = e.getBase().getType().getBaseType*().getUnderlyingType() and + ( + e.getSelector() = child.getAReference() + or + implicitFieldSelection(e, i + 1, child) + ) + | + child = baseType.getFieldOfEmbedded(implicitField, _, i, _) + ) +} + /** * A node in the intra-procedural control-flow graph of a Go function or file. * @@ -309,20 +322,7 @@ newtype TControlFlowNode = * represents an implicit dereference of it. */ MkImplicitFieldSelection(SelectorExpr e, int i, Field implicitField) { - exists(Type baseType, StructType baseStructType, Field eField, int minDepth | - eField.getAReference() = e.getSelector() and - baseType = e.getBase().getType().getUnderlyingType() and - baseStructType = [baseType, baseType.(PointerType).getBaseType().getUnderlyingType()] and - baseStructType.getFieldAtDepth(_, minDepth) = eField - | - baseStructType.getFieldAtDepth(_, i) = implicitField and - exists(Type implicitFieldType, StructType implicitFieldStructType | - implicitFieldType = implicitField.getType().getUnderlyingType() and - implicitFieldStructType = - [implicitFieldType, implicitFieldType.(PointerType).getBaseType().getUnderlyingType()] and - implicitFieldStructType.getFieldAtDepth(_, minDepth - i - 1) = eField - ) - ) + implicitFieldSelection(e, i, implicitField) } or /** * A control-flow node that represents the start of the execution of a function or file. diff --git a/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected b/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected index eee9ee27325..18a29cc61a6 100644 --- a/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected +++ b/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected @@ -2,11 +2,16 @@ | cyclic.go:8:3:8:3 | u | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | cyclic.go:9:2:9:2 | f | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | cyclic.go:13:2:13:2 | t | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | +| cyclic.go:17:2:17:2 | s | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | depth.go:6:2:6:2 | b | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | depth.go:7:2:7:2 | c | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | depth.go:11:2:11:2 | f | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | depth.go:15:2:15:2 | d | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | depth.go:19:2:19:2 | f | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | +| embedded.go:4:2:4:2 | A | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | +| embedded.go:8:3:8:5 | Baz | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | +| embedded.go:13:2:13:4 | Qux | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | +| embedded.go:14:2:14:4 | Baz | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | | pkg1/embedding.go:19:23:19:26 | base | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | | pkg1/embedding.go:22:27:22:30 | base | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | | pkg1/embedding.go:25:24:25:31 | embedder | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | diff --git a/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected b/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected index 539356cc3a8..6baf1c54d70 100644 --- a/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected +++ b/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected @@ -5,6 +5,7 @@ | cyclic.go:9:2:9:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.u | f | | cyclic.go:13:2:13:2 | t | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.t | t | | cyclic.go:13:2:13:2 | t | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.u | t | +| cyclic.go:17:2:17:2 | s | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.v | s | | depth.go:6:2:6:2 | b | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.a | b | | depth.go:7:2:7:2 | c | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.a | c | | depth.go:11:2:11:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.a | f | @@ -13,6 +14,11 @@ | depth.go:15:2:15:2 | d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.c | d | | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.c | f | | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.d | f | +| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Baz | A | +| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux | A | +| embedded.go:8:3:8:5 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux | Baz | +| embedded.go:13:2:13:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Qux | +| embedded.go:14:2:14:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Baz | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder | base | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder2 | base | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder3 | base | diff --git a/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected b/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected index b249ba8feb8..97524ed3f0f 100644 --- a/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected +++ b/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected @@ -5,6 +5,7 @@ | cyclic.go:9:2:9:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | u | f | | cyclic.go:13:2:13:2 | t | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | t | t | | cyclic.go:13:2:13:2 | t | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | u | t | +| cyclic.go:17:2:17:2 | s | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | v | s | | depth.go:6:2:6:2 | b | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | a | b | | depth.go:7:2:7:2 | c | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | a | c | | depth.go:11:2:11:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | a | f | @@ -13,6 +14,11 @@ | depth.go:15:2:15:2 | d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | c | d | | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | c | f | | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | d | f | +| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Baz | A | +| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Qux | A | +| embedded.go:8:3:8:5 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Qux | Baz | +| embedded.go:13:2:13:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Qux | +| embedded.go:14:2:14:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Baz | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | embedder | base | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | embedder2 | base | | pkg1/embedding.go:19:23:19:26 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | embedder3 | base | diff --git a/ql/test/library-tests/semmle/go/Types/MethodTypes.expected b/ql/test/library-tests/semmle/go/Types/MethodTypes.expected index fd509da3c0d..a30d9f893c8 100644 --- a/ql/test/library-tests/semmle/go/Types/MethodTypes.expected +++ b/ql/test/library-tests/semmle/go/Types/MethodTypes.expected @@ -16,5 +16,6 @@ | embedder | f | func() int | | embedder2 | f | func() int | | embedder3 | f | func() int | +| embedder4 | f | func() int | | ptrembedder | f | func() int | | ptrembedder | g | func() int | diff --git a/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected b/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected index d2aadb70314..3eda347c40f 100644 --- a/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected +++ b/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected @@ -5,9 +5,12 @@ | AExtended | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.AExtended | | B | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.B | | Bar | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.Bar | +| Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Baz | | C | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.C | +| EmbedsBaz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | | Foo | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.Foo | | G | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.G | +| Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux | | T | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.T | | T | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.T | | T2 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.T2 | @@ -26,3 +29,4 @@ | s | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.s | | t | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.t | | u | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.u | +| v | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.v | diff --git a/ql/test/library-tests/semmle/go/Types/StructFields.expected b/ql/test/library-tests/semmle/go/Types/StructFields.expected index 7c1d9784c1f..9bf97529652 100644 --- a/ql/test/library-tests/semmle/go/Types/StructFields.expected +++ b/ql/test/library-tests/semmle/go/Types/StructFields.expected @@ -1,8 +1,13 @@ | Bar | pkg1/tst.go:29:10:31:1 | struct type | flag | bool | +| Baz | embedded.go:3:10:5:1 | struct type | A | string | +| EmbedsBaz | embedded.go:12:16:15:1 | struct type | Baz | string | +| EmbedsBaz | embedded.go:12:16:15:1 | struct type | Qux | Qux | | Foo | pkg1/tst.go:24:10:27:1 | struct type | flag | bool | | Foo | pkg1/tst.go:24:10:27:1 | struct type | val | int | | G | pkg2/tst.go:3:8:5:1 | struct type | g | int | | G | pkg2/tst.go:7:8:9:1 | struct type | g | int | +| Qux | embedded.go:7:10:9:1 | struct type | A | string | +| Qux | embedded.go:7:10:9:1 | struct type | Baz | * Baz | | T | pkg1/tst.go:3:8:7:1 | struct type | Bar | Bar | | T | pkg1/tst.go:3:8:7:1 | struct type | Foo | Foo | | T | pkg1/tst.go:3:8:7:1 | struct type | f | int | @@ -46,3 +51,4 @@ | u | cyclic.go:12:8:14:1 | struct type | f | int | | u | cyclic.go:12:8:14:1 | struct type | t | t | | u | cyclic.go:12:8:14:1 | struct type | u | * u | +| v | cyclic.go:16:8:18:1 | struct type | s | s | diff --git a/ql/test/library-tests/semmle/go/Types/Types.expected b/ql/test/library-tests/semmle/go/Types/Types.expected index 3a39cb70c58..60f9edd55d3 100644 --- a/ql/test/library-tests/semmle/go/Types/Types.expected +++ b/ql/test/library-tests/semmle/go/Types/Types.expected @@ -5,9 +5,12 @@ | AExtended | AExtended | | B | B | | Bar | Bar | +| Baz | Baz | | C | C | +| EmbedsBaz | EmbedsBaz | | Foo | Foo | | G | G | +| Qux | Qux | | T | T | | T | T | | T2 | T2 | @@ -26,3 +29,4 @@ | s | s | | t | t | | u | u | +| v | v | diff --git a/ql/test/library-tests/semmle/go/Types/cyclic.go b/ql/test/library-tests/semmle/go/Types/cyclic.go index efe49a739b7..95abfcdc88b 100644 --- a/ql/test/library-tests/semmle/go/Types/cyclic.go +++ b/ql/test/library-tests/semmle/go/Types/cyclic.go @@ -12,3 +12,12 @@ type t struct { type u struct { t } + +type v struct { + s +} + +// the below will cause the test to not terminate +// type w struct { +// v +// } diff --git a/ql/test/library-tests/semmle/go/Types/embedded.go b/ql/test/library-tests/semmle/go/Types/embedded.go new file mode 100644 index 00000000000..c9c2cf46c2b --- /dev/null +++ b/ql/test/library-tests/semmle/go/Types/embedded.go @@ -0,0 +1,15 @@ +package main + +type Baz struct { + A string +} + +type Qux struct { + *Baz +} + +// EmbedsBaz should have a field A but does not +type EmbedsBaz struct { + Qux + Baz string +}