Merge pull request #184 from max-schaefer/lookup-fields-in-cyclic-struct

max-schaefer · web-flow · commit 18db1fe79f45 · 2020-06-21T09:23:57.000+01:00
Fix field lookup in cyclic structs
diff --git a/change-notes/2020-06-19-cyclic-field-lookup.md b/change-notes/2020-06-19-cyclic-field-lookup.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A bug has been fixed that could cause the analysis not to terminate in the presence of cycles through embedded struct fields.
diff --git a/ql/src/semmle/go/Types.qll b/ql/src/semmle/go/Types.qll
@@ -389,6 +389,7 @@ class StructType extends @structtype, CompositeType {
   ) {
     hasOwnField(_, name, tp, isEmbedded) and depth = 0 and isMethod = false
     or
+    not hasOwnField(_, name, _, _) and
     exists(Type embedded | hasEmbeddedField(embedded, depth - 1) |
       embedded.getUnderlyingType().(StructType).hasOwnField(_, name, tp, isEmbedded) and
       isMethod = false
diff --git a/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected b/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected
@@ -1,3 +1,12 @@
+| cyclic.go:4:3:4:3 | s | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
+| 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 |
+| 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 |
 | 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
@@ -1,3 +1,18 @@
+| cyclic.go:4:3:4:3 | s | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.s | s |
+| cyclic.go:8:3:8:3 | u | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.t | u |
+| cyclic.go:8:3:8:3 | u | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.u | u |
+| cyclic.go:9:2:9:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.t | f |
+| 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 |
+| 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 |
+| depth.go:11:2:11:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.b | f |
+| depth.go:15:2:15:2 | d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.a | d |
+| 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 |
 | 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
@@ -1,3 +1,18 @@
+| cyclic.go:4:3:4:3 | s | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | s | s |
+| cyclic.go:8:3:8:3 | u | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | t | u |
+| cyclic.go:8:3:8:3 | u | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | u | u |
+| cyclic.go:9:2:9:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | t | f |
+| 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 |
+| 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 |
+| depth.go:11:2:11:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | b | f |
+| depth.go:15:2:15:2 | d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | a | d |
+| 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 |
 | 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/QualifiedNames.expected b/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected
@@ -13,9 +13,16 @@
 | T2 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.T2 |
 | T3 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.T3 |
 | T4 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.T4 |
+| a | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.a |
+| b | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.b |
 | base | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.base |
+| c | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.c |
+| d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.d |
 | embedder | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder |
 | embedder2 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder2 |
 | embedder3 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder3 |
 | embedder4 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.embedder4 |
 | ptrembedder | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.ptrembedder |
+| 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 |
diff --git a/ql/test/library-tests/semmle/go/Types/SignatureType_getNumParameter.expected b/ql/test/library-tests/semmle/go/Types/SignatureType_getNumParameter.expected
@@ -1,3 +1,4 @@
+| depth.go:22:1:25:1 | function declaration | 0 |
 | main.go:5:1:5:30 | function declaration | 1 |
 | main.go:7:1:9:1 | function declaration | 2 |
 | main.go:11:1:11:14 | function declaration | 0 |
diff --git a/ql/test/library-tests/semmle/go/Types/SignatureType_getNumResult.expected b/ql/test/library-tests/semmle/go/Types/SignatureType_getNumResult.expected
@@ -1,3 +1,4 @@
+| depth.go:22:1:25:1 | function declaration | 1 |
 | main.go:5:1:5:30 | function declaration | 0 |
 | main.go:7:1:9:1 | function declaration | 2 |
 | main.go:11:1:11:14 | function declaration | 0 |
diff --git a/ql/test/library-tests/semmle/go/Types/StructFields.expected b/ql/test/library-tests/semmle/go/Types/StructFields.expected
@@ -19,6 +19,14 @@
 | T4 | pkg1/tst.go:19:9:22:1 | struct type | Foo | * Foo |
 | T4 | pkg1/tst.go:19:9:22:1 | struct type | flag | bool |
 | T4 | pkg1/tst.go:19:9:22:1 | struct type | val | int |
+| a | depth.go:5:8:8:1 | struct type | b | b |
+| a | depth.go:5:8:8:1 | struct type | c | c |
+| a | depth.go:5:8:8:1 | struct type | d | d |
+| a | depth.go:5:8:8:1 | struct type | f | int |
+| b | depth.go:10:8:12:1 | struct type | f | int |
+| c | depth.go:14:8:16:1 | struct type | d | d |
+| c | depth.go:14:8:16:1 | struct type | f | string |
+| d | depth.go:18:8:20:1 | struct type | f | string |
 | embedder | pkg1/embedding.go:19:15:19:28 | struct type | base | base |
 | embedder2 | pkg1/embedding.go:25:16:25:33 | struct type | base | base |
 | embedder2 | pkg1/embedding.go:25:16:25:33 | struct type | embedder | embedder |
@@ -31,3 +39,10 @@
 | embedder4 | pkg1/embedding.go:35:16:38:1 | struct type | base | base |
 | embedder4 | pkg1/embedding.go:35:16:38:1 | struct type | f | int |
 | ptrembedder | pkg1/embedding.go:22:18:22:32 | struct type | base | * base |
+| s | cyclic.go:3:8:5:1 | struct type | s | * s |
+| t | cyclic.go:7:8:10:1 | struct type | f | int |
+| t | cyclic.go:7:8:10:1 | struct type | t | t |
+| t | cyclic.go:7:8:10:1 | struct type | u | * u |
+| 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 |
diff --git a/ql/test/library-tests/semmle/go/Types/Types.expected b/ql/test/library-tests/semmle/go/Types/Types.expected
@@ -13,9 +13,16 @@
 | T2 | T2 |
 | T3 | T3 |
 | T4 | T4 |
+| a | a |
+| b | b |
 | base | base |
+| c | c |
+| d | d |
 | embedder | embedder |
 | embedder2 | embedder2 |
 | embedder3 | embedder3 |
 | embedder4 | embedder4 |
 | ptrembedder | ptrembedder |
+| s | s |
+| t | t |
+| u | u |
diff --git a/ql/test/library-tests/semmle/go/Types/cyclic.go b/ql/test/library-tests/semmle/go/Types/cyclic.go
@@ -0,0 +1,14 @@
+package main
+
+type s struct {
+	*s
+}
+
+type t struct {
+	*u
+	f int
+}
+
+type u struct {
+	t
+}
diff --git a/ql/test/library-tests/semmle/go/Types/depth.go b/ql/test/library-tests/semmle/go/Types/depth.go
@@ -0,0 +1,25 @@
+package main
+
+import "fmt"
+
+type a struct {
+	b // we get f from here
+	c // but not from here because it is nested more deeply
+}
+
+type b struct {
+	f int
+}
+
+type c struct {
+	d
+}
+
+type d struct {
+	f string
+}
+
+func test2() int {
+	x := a{b{0}, c{d{"hi"}}}
+	fmt.Printf("%v", x.f) // prints `0`, not `"hi"`
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+lgtm,codescanning`
	`2`	`+* A bug has been fixed that could cause the analysis not to terminate in the presence of cycles through embedded struct fields.`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+\| depth.go:22:1:25:1 \| function declaration \| 0 \|`
`1`	`2`	`\| main.go:5:1:5:30 \| function declaration \| 1 \|`
`2`	`3`	`\| main.go:7:1:9:1 \| function declaration \| 2 \|`
`3`	`4`	`\| main.go:11:1:11:14 \| function declaration \| 0 \|`