[go] reflect: fix stack overflow panic when using ConvertibleTo
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
reflect: fix stack overflow panic when using ConvertibleTo
reflect.ConvertibleTo will raise stack overflow when using with self referential struct having same structure
for example:
```go
type MyStruct struct {
MyStructs []MyStruct
MyStruct *MyStruct
}
```
error message like:
```
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc020596390 stack=[0xc020596000, 0xc040596000]
fatal error: stack overflow
runtime stack:
runtime.throw(0x125ec63, 0xe)
/Users/jinzhu/Projects/jinzhu/go/src/runtime/panic.go:1191 +0x74
runtime.newstack()
/Users/jinzhu/Projects/jinzhu/go/src/runtime/stack.go:1086 +0x6a5
runtime.morestack()
/Users/jinzhu/Projects/jinzhu/go/src/runtime/asm_amd64.s:465 +0x8b
goroutine 4056 [running]:
reflect.(*rtype).Elem(0x1209e00, 0x0, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:903 +0x1ab fp=0xc0205963a0 sp=0xc020596398 pc=0x10c284b
reflect.haveIdenticalUnderlyingType(0x1209e00, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x405 fp=0xc020596480 sp=0xc0205963a0 pc=0x10c6be5
reflect.haveIdenticalType(0x13a9980, 0x1209e00, 0x13a9980, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc0205964d8 sp=0xc020596480 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232420, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc0205965b8 sp=0xc0205964d8 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9980, 0x1232420, 0x13a9980, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020596610 sp=0xc0205965b8 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209e00, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc0205966f0 sp=0xc020596610 pc=0x10c6c5b
reflect.haveIdenticalType(0x13a9980, 0x1209e00, 0x13a9980, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020596748 sp=0xc0205966f0 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232420, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc020596828 sp=0xc020596748 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9980, 0x1232420, 0x13a9980, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020596880 sp=0xc020596828 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209e00, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc020596960 sp=0xc020596880 pc=0x10c6c5b
reflect.haveIdenticalType(0x13a9980, 0x1209e00, 0x13a9980, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc0205969b8 sp=0xc020596960 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232420, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc020596a98 sp=0xc0205969b8 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9980, 0x1232420, 0x13a9980, 0x1232380, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020596af0 sp=0xc020596a98 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209e00, 0x1209dc0, 0x0, 0xc040595c40, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc020596bd0 sp=0xc020596af0 pc=0x10c6c5b
```
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 28ad3750b7a79cf51bc05ba6223c0bc86505130d
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 241f6b0..bbccd31 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -14,7 +14,10 @@
"math"
"math/rand"
"os"
+ "reflect"
. "reflect"
+ example1 "reflect/internal/example1"
+ example2 "reflect/internal/example2"
"runtime"
"sort"
"strconv"
@@ -7231,3 +7234,9 @@
sort.Strings(got)
return "[" + strings.Join(got, ", ") + "]"
}
+
+func TestConvertibleTo(t *testing.T) {
+ t1 := reflect.ValueOf(example1.MyStruct{}).Type()
+ t2 := reflect.ValueOf(example2.MyStruct{}).Type()
+ t1.ConvertibleTo(t2)
+}
diff --git a/src/reflect/internal/example1/example.go b/src/reflect/internal/example1/example.go
new file mode 100644
index 0000000..a1e6fda
--- /dev/null
+++ b/src/reflect/internal/example1/example.go
@@ -0,0 +1,6 @@
+package example
+
+type MyStruct struct {
+ MyStructs []MyStruct
+ MyStruct *MyStruct
+}
diff --git a/src/reflect/internal/example2/example.go b/src/reflect/internal/example2/example.go
new file mode 100644
index 0000000..a1e6fda
--- /dev/null
+++ b/src/reflect/internal/example2/example.go
@@ -0,0 +1,6 @@
+package example
+
+type MyStruct struct {
+ MyStructs []MyStruct
+ MyStruct *MyStruct
+}
diff --git a/src/reflect/type.go b/src/reflect/type.go
index c213b31..968d54a 100644
--- a/src/reflect/type.go
+++ b/src/reflect/type.go
@@ -1566,7 +1566,7 @@
// x is a bidirectional channel value, T is a channel type,
// x's type V and T have identical element types,
// and at least one of V or T is not a defined type.
- return V.ChanDir() == BothDir && (T.Name() == "" || V.Name() == "") && haveIdenticalType(T.Elem(), V.Elem(), true)
+ return V.ChanDir() == BothDir && (T.Name() == "" || V.Name() == "") && haveIdenticalType(T.Elem(), V.Elem(), true, map[cacheKey]bool{})
}
// directlyAssignable reports whether a value x of type V can be directly
@@ -1591,10 +1591,10 @@
}
// x's type T and V must have identical underlying types.
- return haveIdenticalUnderlyingType(T, V, true)
+ return haveIdenticalUnderlyingType(T, V, true, map[cacheKey]bool{})
}
-func haveIdenticalType(T, V Type, cmpTags bool) bool {
+func haveIdenticalType(T, V Type, cmpTags bool, compared map[cacheKey]bool) bool {
if cmpTags {
return T == V
}
@@ -1603,10 +1603,10 @@
return false
}
- return haveIdenticalUnderlyingType(T.common(), V.common(), false)
+ return haveIdenticalUnderlyingType(T.common(), V.common(), false, compared)
}
-func haveIdenticalUnderlyingType(T, V *rtype, cmpTags bool) bool {
+func haveIdenticalUnderlyingType(T, V *rtype, cmpTags bool, compared map[cacheKey]bool) bool {
if T == V {
return true
}
@@ -1625,10 +1625,10 @@
// Composite types.
switch kind {
case Array:
- return T.Len() == V.Len() && haveIdenticalType(T.Elem(), V.Elem(), cmpTags)
+ return T.Len() == V.Len() && haveIdenticalType(T.Elem(), V.Elem(), cmpTags, compared)
case Chan:
- return V.ChanDir() == T.ChanDir() && haveIdenticalType(T.Elem(), V.Elem(), cmpTags)
+ return V.ChanDir() == T.ChanDir() && haveIdenticalType(T.Elem(), V.Elem(), cmpTags, compared)
case Func:
t := (*funcType)(unsafe.Pointer(T))
@@ -1637,12 +1637,12 @@
return false
}
for i := 0; i < t.NumIn(); i++ {
- if !haveIdenticalType(t.In(i), v.In(i), cmpTags) {
+ if !haveIdenticalType(t.In(i), v.In(i), cmpTags, compared) {
return false
}
}
for i := 0; i < t.NumOut(); i++ {
- if !haveIdenticalType(t.Out(i), v.Out(i), cmpTags) {
+ if !haveIdenticalType(t.Out(i), v.Out(i), cmpTags, compared) {
return false
}
}
@@ -1659,10 +1659,10 @@
return false
case Map:
- return haveIdenticalType(T.Key(), V.Key(), cmpTags) && haveIdenticalType(T.Elem(), V.Elem(), cmpTags)
+ return haveIdenticalType(T.Key(), V.Key(), cmpTags, compared) && haveIdenticalType(T.Elem(), V.Elem(), cmpTags, compared)
case Ptr, Slice:
- return haveIdenticalType(T.Elem(), V.Elem(), cmpTags)
+ return haveIdenticalType(T.Elem(), V.Elem(), cmpTags, compared)
case Struct:
t := (*structType)(unsafe.Pointer(T))
@@ -1679,7 +1679,8 @@
if tf.name.name() != vf.name.name() {
return false
}
- if !haveIdenticalType(tf.typ, vf.typ, cmpTags) {
+
+ if !haveIdenticalType(tf.typ, vf.typ, cmpTags, compared) {
return false
}
if cmpTags && tf.name.tag() != vf.name.tag() {
@@ -2024,7 +2025,7 @@
// Look in cache.
if ts, ok := funcLookupCache.m.Load(hash); ok {
for _, t := range ts.([]*rtype) {
- if haveIdenticalUnderlyingType(&ft.rtype, t, true) {
+ if haveIdenticalUnderlyingType(&ft.rtype, t, true, map[cacheKey]bool{}) {
return t
}
}
@@ -2035,7 +2036,7 @@
defer funcLookupCache.Unlock()
if ts, ok := funcLookupCache.m.Load(hash); ok {
for _, t := range ts.([]*rtype) {
- if haveIdenticalUnderlyingType(&ft.rtype, t, true) {
+ if haveIdenticalUnderlyingType(&ft.rtype, t, true, map[cacheKey]bool{}) {
return t
}
}
@@ -2053,7 +2054,7 @@
// Look in known types for the same string representation.
str := funcStr(ft)
for _, tt := range typesByString(str) {
- if haveIdenticalUnderlyingType(&ft.rtype, tt, true) {
+ if haveIdenticalUnderlyingType(&ft.rtype, tt, true, map[cacheKey]bool{}) {
return addToCache(tt)
}
}
@@ -2656,7 +2657,7 @@
if ts, ok := structLookupCache.m.Load(hash); ok {
for _, st := range ts.([]Type) {
t := st.common()
- if haveIdenticalUnderlyingType(&typ.rtype, t, true) {
+ if haveIdenticalUnderlyingType(&typ.rtype, t, true, map[cacheKey]bool{}) {
return t
}
}
@@ -2668,7 +2669,7 @@
if ts, ok := structLookupCache.m.Load(hash); ok {
for _, st := range ts.([]Type) {
t := st.common()
- if haveIdenticalUnderlyingType(&typ.rtype, t, true) {
+ if haveIdenticalUnderlyingType(&typ.rtype, t, true, map[cacheKey]bool{}) {
return t
}
}
@@ -2685,7 +2686,7 @@
// Look in known types.
for _, t := range typesByString(str) {
- if haveIdenticalUnderlyingType(&typ.rtype, t, true) {
+ if haveIdenticalUnderlyingType(&typ.rtype, t, true, map[cacheKey]bool{}) {
// even if 't' wasn't a structType with methods, we should be ok
// as the 'u uncommonType' field won't be accessed except when
// tflag&tflagUncommon is set.
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 7890c12..fa607da 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -2841,14 +2841,14 @@
}
// dst and src have same underlying type.
- if haveIdenticalUnderlyingType(dst, src, false) {
+ if haveIdenticalUnderlyingType(dst, src, false, map[cacheKey]bool{}) {
return cvtDirect
}
// dst and src are non-defined pointer types with same underlying base type.
if dst.Kind() == Ptr && dst.Name() == "" &&
src.Kind() == Ptr && src.Name() == "" &&
- haveIdenticalUnderlyingType(dst.Elem().common(), src.Elem().common(), false) {
+ haveIdenticalUnderlyingType(dst.Elem().common(), src.Elem().common(), false, map[cacheKey]bool{}) {
return cvtDirect
}
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #2 to this change.
GitHub-Last-Rev: 3d5b163bb142746a4a5223ba9a0749f7e51e01c7
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 50 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #3 to this change.
reflect: fix stack overflow panic when using ConvertibleTo
reflect.ConvertibleTo will raise stack overflow when using with self referential struct having same structure, e.g:
type MyStruct struct {
MyStructs []MyStruct
MyStruct *MyStruct
}
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 3d5b163bb142746a4a5223ba9a0749f7e51e01c7
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 50 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
https://play.golang.org/p/prRCOrQXQPW could reproduce the issue, it will raise panic message similar like:
```
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc020608350 stack=[0xc020608000, 0xc040608000]
fatal error: stack overflowruntime stack:
runtime.throw(0x125ed43, 0xe)
/Users/jinzhu/Projects/jinzhu/go/src/runtime/panic.go:1191 +0x74
runtime.newstack()
/Users/jinzhu/Projects/jinzhu/go/src/runtime/stack.go:1086 +0x6a5
runtime.morestack()
/Users/jinzhu/Projects/jinzhu/go/src/runtime/asm_amd64.s:465 +0x8b
goroutine 260 [running]:
reflect.(*rtype).Elem(0x1209ee0, 0x0, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:903 +0x1ab fp=0xc020608360 sp=0xc020608358 pc=0x10c284b
reflect.haveIdenticalUnderlyingType(0x1209ee0, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x405 fp=0xc020608440 sp=0xc020608360 pc=0x10c6be5
reflect.haveIdenticalType(0x13a9a60, 0x1209ee0, 0x13a9a60, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608498 sp=0xc020608440 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232500, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc020608578 sp=0xc020608498 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9a60, 0x1232500, 0x13a9a60, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc0206085d0 sp=0xc020608578 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209ee0, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc0206086b0 sp=0xc0206085d0 pc=0x10c6c5b
reflect.haveIdenticalType(0x13a9a60, 0x1209ee0, 0x13a9a60, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608708 sp=0xc0206086b0 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232500, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc0206087e8 sp=0xc020608708 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9a60, 0x1232500, 0x13a9a60, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608840 sp=0xc0206087e8 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209ee0, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc020608920 sp=0xc020608840 pc=0x10c6c5b
reflect.haveIdenticalType(0x13a9a60, 0x1209ee0, 0x13a9a60, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608978 sp=0xc020608920 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232500, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc020608a58 sp=0xc020608978 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9a60, 0x1232500, 0x13a9a60, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608ab0 sp=0xc020608a58 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209ee0, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc020608b90 sp=0xc020608ab0 pc=0x10c6c5b
reflect.haveIdenticalType(0x13a9a60, 0x1209ee0, 0x13a9a60, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608be8 sp=0xc020608b90 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1232500, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1683 +0x7b7 fp=0xc020608cc8 sp=0xc020608be8 pc=0x10c6f97
reflect.haveIdenticalType(0x13a9a60, 0x1232500, 0x13a9a60, 0x1232460, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1606 +0x192 fp=0xc020608d20 sp=0xc020608cc8 pc=0x10c67b2
reflect.haveIdenticalUnderlyingType(0x1209ee0, 0x1209ea0, 0x0, 0xc040607c00, 0x0)
/Users/jinzhu/Projects/jinzhu/go/src/reflect/type.go:1665 +0x47b fp=0xc020608e00 sp=0xc020608d20 pc=0x10c6c5b
```
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
7 comments:
Commit Message:
Patch Set #3, Line 9:
reflect.ConvertibleTo will raise stack overflow when using with self referential struct having same structure, e.g:In this sentence I think it would be clearer to refer to haveIdenticalUnderlyingType rather than reflect.ConvertibleTo, since haveIdenticalUnderlyingType is called in other cases, such as AssignableTo.
Also please break the long line.
Patchset:
An entirely different way to fix this problem might be to change the compiler to always set the pkgPath field of relect.structType. Currently it only sets that field if the struct contains an unexported field.
Can you open an issue for this if there isn't one open already, so that we can discuss fixes? Thanks.
File src/reflect/all_test.go:
Patch Set #3, Line 17:
"reflect"The reflect package is already imported using a dot import. Just change "reflect.ValueOf" to simply "ValueOf" in the new code.
Patch Set #3, Line 19:
example1 "reflect/internal/example1"Most of these tests use types defines in this package. Is is necessary to define these types in different packages?
And if they are in different packages, I don't see any reason that they must both be "package example".
Patch Set #3, Line 7242:
t.Errorf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)It seems to me that this is not correct. If I use this example in a standalone program
var s1 example1.MyStruct
s2 := example2.MyStruct(s1)I get a compiler error:
main.go:12:25: cannot convert s1 (type "x.mod/example".MyStruct) to type "x.mod/example2".MyStruct
That is, the types are not convertible, and ConvertibleTo should return false.
File src/reflect/type.go:
Patch Set #3, Line 1684:
if _, ok := compared[ckey]; ok {You are mapping to bool and you only store true, so just write
if compared[ckey] {Patch Set #3, Line 1686:
} else {No need for "else" after "continue".
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Gerrit Bot uploaded patch set #4 to this change.
reflect: fix stack overflow panic when using ConvertibleTo
haveIdenticalUnderlyingType will raise stack overflow when using with self referential struct having same structure, e.g:
type MyStruct struct {
MyStructs []MyStruct
MyStruct *MyStruct
}
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 3d5b163bb142746a4a5223ba9a0749f7e51e01c7
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 50 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Gerrit Bot uploaded patch set #5 to this change.
reflect: fix stack overflow panic when using ConvertibleTo
haveIdenticalUnderlyingType will raise stack overflow when using with self referential struct having same structure, e.g:
type MyStruct struct {
MyStructs []MyStruct
MyStruct *MyStruct
}
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: c1d1780a2a70876e46c88927a9fefb87b8d0aef5
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 72 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Gerrit Bot uploaded patch set #6 to this change.
reflect: fix stack overflow panic when using ConvertibleTo
haveIdenticalUnderlyingType will raise stack overflow when using with self referential struct having same structure, e.g:
type MyStruct struct {
MyStructs []MyStruct
MyStruct *MyStruct
}
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 410e5e7c73643f2c504571fa0e88c3ffa996526f
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 72 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Gerrit Bot uploaded patch set #7 to this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 410e5e7c73643f2c504571fa0e88c3ffa996526f
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
M src/reflect/value.go
5 files changed, 72 insertions(+), 20 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
6 comments:
Commit Message:
Patch Set #3, Line 9:
reflect.ConvertibleTo will raise stack overflow when using with self referential struct having same structure, e.g:
In this sentence I think it would be clearer to refer to haveIdenticalUnderlyingType rather than ref […]
Done
File src/reflect/all_test.go:
Patch Set #3, Line 17:
"reflect"
The reflect package is already imported using a dot import. Just change "reflect. […]
Done
Patch Set #3, Line 19:
example1 "reflect/internal/example1"
Is is necessary to define these types in different packages?
It is not Convertible if it is defined in same package.
I don't see any reason that they must both be "package example".
Thanks for the suggestion, renamed the package name to `example1` and `example2`
Patch Set #3, Line 7242:
t.Errorf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)
It seems to me that this is not correct. If I use this example in a standalone program […]
Added a test case to the pull request, if two structs having same structure, we can use reflect Convert one to another, e.g:
```
func TestConvertibleTo(t *testing.T) {
t1 := ValueOf(example1.MyStruct{}).Type()
t2 := ValueOf(example2.MyStruct{}).Type()
if !t1.ConvertibleTo(t2) {
t.Fatalf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)
}v1 := example1.MyStruct{
Name: "hello",
MyStruct: &example1.MyStruct{
Name: "world",
},
MyStructs: []example1.MyStruct{
{Name: "hello world 1"},
{Name: "hello world 2"},
},
}v2, ok := ValueOf(v1).Convert(t2).Interface().(example2.MyStruct)
if !ok {
t.Fatalf("(%s).Convert(%s) failed", t1, t2)
}if fmt.Sprintf("%+v", v2) != fmt.Sprintf("%+v", v1) {
t.Fatalf("(%s).Convert(%s) got %+v, expecting %+v", t1, t2, v2, v1)
}
}
```
File src/reflect/type.go:
Patch Set #3, Line 1684:
if _, ok := compared[ckey]; ok {
You are mapping to bool and you only store true, so just write […]
Done
Patch Set #3, Line 1686:
} else {No need for "else" after "continue".
Done
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
Patchset:
https://play.golang. […]
Done
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Jinzhu Zhang, Russ Cox.
1 comment:
File src/reflect/all_test.go:
Patch Set #3, Line 7242:
t.Errorf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)Added a test case to the pull request, if two structs having same structure, we can use reflect Conv […]
What I mean is that if you write similar code in Go, without using reflect, you get an error. See https://play.golang.org/p/HU5J1av11fE. Therefore ConvertibleTo should return false: the conversion is not permitted.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Jinzhu Zhang, Russ Cox.
Gerrit Bot uploaded patch set #8 to this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: f82bffef9f25b7db22479632fe433322ff30ed1a
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
4 files changed, 25 insertions(+), 0 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Jinzhu Zhang, Russ Cox.
Gerrit Bot uploaded patch set #9 to this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 32ea60cee4c2300bc670656bd202444fa2a06bc2
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
4 files changed, 27 insertions(+), 0 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
File src/reflect/all_test.go:
Patch Set #3, Line 7242:
t.Errorf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)
What I mean is that if you write similar code in Go, without using reflect, you get an error. […]
Hi Ian,
Just rewrite the PR to fix the issue and make the ConvertibleTo returns false, thank you for your review.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #9, Line 1682:
if tf.typ.str != vf.typ.str {I believe that this will give the wrong result in some cases if cmpTags is false. See this test case: https://play.golang.org/p/tNz_qMVqU6s .
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Gerrit Bot uploaded patch set #10 to this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 163de19b47770bf613234a898d2d5082e387ead7
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
4 files changed, 40 insertions(+), 0 deletions(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #9, Line 1682:
if tf.typ.str != vf.typ.str {
I believe that this will give the wrong result in some cases if cmpTags is false. […]
Fixed the issue, thank you for pointing it out.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
File src/reflect/all_test.go:
Patch Set #3, Line 7242:
t.Errorf("(%s).ConvertibleTo(%s) = false, want true", t1, t2)
Hi Ian, […]
Done
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #10, Line 1682:
if tf.typ.Kind() != Struct && tf.typ.str != vf.typ.str {I'm sorry, I just don't see how this is the right track.
tf.typ.str is an offset to a name that must be interpreted with regard to tf.typ, as in tf.typ.nameOff(tf.typ.str). That will give an encoded type name. I don't see how comparing the offset values here is right. Even comparing the encoded type names isn't necessarily right. The point of haveIdenticalUnderlyingType is to look for identical underlying types. I don't see any reason to believe that tf.typ.str == vf.typ.str is required for underlying types.
Your example is a good one, but the key to your example is that the types look exactly the same except that they are defined in different packages.
What happens if we change haveIdenticalType to also check T.PkgPath() == V.PkgPath()?
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #10, Line 1682:
if tf.typ.Kind() != Struct && tf.typ.str != vf.typ.str {
I'm sorry, I just don't see how this is the right track. […]
Hi Ian,
> The point of haveIdenticalUnderlyingType is to look for identical underlying types. I don't see any reason to believe that tf.typ.str == vf.typ.str is required for underlying types.
`tf.typ.str == vf.typ.str` only checked if the field's type is not a `Struct`, from my understanding/testing, other kinds of the field type can't be converted unless they are using the same one?So we are checking the value of `str` here to make sure the field's type is the same one unless the field's type is a `Struct`, is it the right way?
> Your example is a good one, but the key to your example is that the types look exactly the same except that they are defined in different packages.
Stack overflow only raise when the types exactly same and defined in different packages.
> What happens if we change haveIdenticalType to also check T.PkgPath() == V.PkgPath()?
This will break the following case?```
package mainimport (
example1 "play.ground/example1"
example2 "play.ground/example2"
"reflect"
)func main() {
var v1 example1.MyStruct
_ = example2.MyStruct(v1)t2 := reflect.ValueOf(example2.MyStruct{}).Type()
_ = reflect.ValueOf(v1).Convert(t2).Interface().(example2.MyStruct)
}
-- go.mod --
module play.ground
-- example1/a.go --
package exampletype MyStruct struct {
Name string
}
-- example2/a.go --
package exampletype MyStruct struct {
Name string
}
```Thank you.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Jinzhu Zhang, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #10, Line 1682:
if tf.typ.Kind() != Struct && tf.typ.str != vf.typ.str {
Hi Ian, […]
As far as I can tell, changing haveIdenticalType to say
if T.Name() != V.Name() || T.Kind() != V.Kind() || T.PkgPath() != V.PkgPath() {
return false
}does not break that example.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Michael Knyszek, Jinzhu Zhang, Russ Cox.
Gerrit Bot uploaded patch set #11 to this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 4d4217f0c16ef14aa1f38ff7cf88c98755bb8ddd
GitHub-Pull-Request: golang/go#45543
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
4 files changed, 38 insertions(+), 1 deletion(-)
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
1 comment:
File src/reflect/type.go:
Patch Set #10, Line 1682:
if tf.typ.Kind() != Struct && tf.typ.str != vf.typ.str {
As far as I can tell, changing haveIdenticalType to say […]
Ah, sorry for the misunderstanding, I was changed the `haveIdenticalUnderlyingType`
Fixed it according to your suggestion, seems everything works now.
Thank you.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Michael Knyszek, Ian Lance Taylor, Russ Cox.
Patch set 11:Run-TryBot +1Trust +1
1 comment:
Patchset:
Thank you Jinzhu!
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Michael Knyszek, Russ Cox.
Patch set 11:Code-Review +2
1 comment:
Patchset:
Thanks for your patience with this.
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor submitted this change.
reflect: fix stack overflow panic when using haveIdenticalUnderlyingType
haveIdenticalUnderlyingType raises stack overflow when compares
self-referential structs having same structure in different packages.
Change-Id: I7c79ab988edcffadcf7e0730a50b4d31b136bb6a
GitHub-Last-Rev: 4d4217f0c16ef14aa1f38ff7cf88c98755bb8ddd
GitHub-Pull-Request: golang/go#45543
Reviewed-on: https://go-review.googlesource.com/c/go/+/309729
Trust: Emmanuel Odeke <emma...@orijtech.com>
Run-TryBot: Emmanuel Odeke <emma...@orijtech.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
---
M src/reflect/all_test.go
A src/reflect/internal/example1/example.go
A src/reflect/internal/example2/example.go
M src/reflect/type.go
4 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 241f6b0..3269f5f 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -15,6 +15,8 @@
"math/rand"
"os"
. "reflect"
+ "reflect/internal/example1"
+ "reflect/internal/example2"
"runtime"
"sort"
"strconv"
@@ -3808,6 +3810,16 @@
type MyStruct struct {
x int `some:"tag"`
}
+type MyStruct1 struct {
+ x struct {
+ int `some:"bar"`
+ }
+}
+type MyStruct2 struct {
+ x struct {
+ int `some:"foo"`
+ }
+}
type MyString string
type MyBytes []byte
type MyRunes []int32
@@ -4158,6 +4170,9 @@
x int `some:"bar"`
}{}), V(MyStruct{})},
+ {V(MyStruct1{}), V(MyStruct2{})},
+ {V(MyStruct2{}), V(MyStruct1{})},
+
// can convert *byte and *MyByte
{V((*byte)(nil)), V((*MyByte)(nil))},
{V((*MyByte)(nil)), V((*byte)(nil))},
@@ -7231,3 +7246,13 @@
sort.Strings(got)
return "[" + strings.Join(got, ", ") + "]"
}
+
+func TestConvertibleTo(t *testing.T) {
+ t1 := ValueOf(example1.MyStruct{}).Type()
+ t2 := ValueOf(example2.MyStruct{}).Type()
+
+ // Shouldn't raise stack overflow
+ if t1.ConvertibleTo(t2) {
+ t.Fatalf("(%s).ConvertibleTo(%s) = true, want false", t1, t2)
+ }
+}
diff --git a/src/reflect/internal/example1/example.go b/src/reflect/internal/example1/example.go
new file mode 100644
index 0000000..0f829a8
--- /dev/null
+++ b/src/reflect/internal/example1/example.go
@@ -0,0 +1,6 @@
+package example1
+
+type MyStruct struct {
+ MyStructs []MyStruct
+ MyStruct *MyStruct
+}
diff --git a/src/reflect/internal/example2/example.go b/src/reflect/internal/example2/example.go
new file mode 100644
index 0000000..df64ba1
--- /dev/null
+++ b/src/reflect/internal/example2/example.go
@@ -0,0 +1,6 @@
+package example2
+
+type MyStruct struct {
+ MyStructs []MyStruct
+ MyStruct *MyStruct
+}
diff --git a/src/reflect/type.go b/src/reflect/type.go
index c213b31..d50559e 100644
--- a/src/reflect/type.go
+++ b/src/reflect/type.go
@@ -1599,7 +1599,7 @@
return T == V
}
- if T.Name() != V.Name() || T.Kind() != V.Kind() {
+ if T.Name() != V.Name() || T.Kind() != V.Kind() || T.PkgPath() != V.PkgPath() {
return false
}
To view, visit change 309729. To unsubscribe, or for help writing mail filters, visit settings.