reflect: fix panic in DeepEqual when checking a cycle

Before this change, when DeepEqual checks values with cycle, it may
panic due to stack overflow.

Here is a sample to reproduce the issue.

    makeCycleMap := func() interface{} {
        cycleMap := map[string]interface{}{}
        cycleMap["foo"] = cycleMap
        return cycleMap
    }

    m1 := makeCycleMap()
    m2 := makeCycleMap()
    reflect.DeepEqual(m1, m2) // stack overflow

The root cause is that DeepEqual fails to cache interface values
in visited map, which is used to detect cycle. DeepEqual calls
CanAddr to check whether a value should be cached or not. However,
all values referenced by interface don't have flagAddr thus all these
values are not cached.

THe fix is to remove CanAddr calls and use underlying ptr in value
directly. As ptr is only read-only in DeepEqual for caching, it's
safe to do so. We don't use UnsafeAddr this time, because this method
panics when CanAddr returns false.

Fixes #33907

Change-Id: I2aa88cc060a2c2192b1d34c129c0aad4bd5597e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/191940
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 4431ce2391096275f4b17e80c560f21f0fefba66..fbb6feb0d98f21b0bdff7437b117d08a4171ee2d 100644 (file)
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -787,6 +787,7 @@ type Loopy interface{}
 
 var loop1, loop2 Loop
 var loopy1, loopy2 Loopy
+var cycleMap1, cycleMap2, cycleMap3 map[string]interface{}
 
 func init() {
        loop1 = &loop2
@@ -794,6 +795,13 @@ func init() {
 
        loopy1 = &loopy2
        loopy2 = &loopy1
+
+       cycleMap1 = map[string]interface{}{}
+       cycleMap1["cycle"] = cycleMap1
+       cycleMap2 = map[string]interface{}{}
+       cycleMap2["cycle"] = cycleMap2
+       cycleMap3 = map[string]interface{}{}
+       cycleMap3["different"] = cycleMap3
 }
 
 var deepEqualTests = []DeepEqualTest{
@@ -860,6 +868,8 @@ var deepEqualTests = []DeepEqualTest{
        {&loop1, &loop2, true},
        {&loopy1, &loopy1, true},
        {&loopy1, &loopy2, true},
+       {&cycleMap1, &cycleMap2, true},
+       {&cycleMap1, &cycleMap3, false},
 }
 
 func TestDeepEqual(t *testing.T) {
@@ -868,7 +878,7 @@ func TestDeepEqual(t *testing.T) {
                        test.b = test.a
                }
                if r := DeepEqual(test.a, test.b); r != test.eq {
-                       t.Errorf("DeepEqual(%v, %v) = %v, want %v", test.a, test.b, r, test.eq)
+                       t.Errorf("DeepEqual(%#v, %#v) = %v, want %v", test.a, test.b, r, test.eq)
                }
        }
 }

diff --git a/src/reflect/deepequal.go b/src/reflect/deepequal.go
index 5b6694d3f0bebfa8b5a25e9e5931438b589f4bf1..f2d46165b50fdc44210d86cad614b7372ae19364 100644 (file)
--- a/src/reflect/deepequal.go
+++ b/src/reflect/deepequal.go
@@ -33,18 +33,20 @@ func deepValueEqual(v1, v2 Value, visited map[visit]bool, depth int) bool {
 
        // We want to avoid putting more in the visited map than we need to.
        // For any possible reference cycle that might be encountered,
-       // hard(t) needs to return true for at least one of the types in the cycle.
-       hard := func(k Kind) bool {
-               switch k {
+       // hard(v1, v2) needs to return true for at least one of the types in the cycle,
+       // and it's safe and valid to get Value's internal pointer.
+       hard := func(v1, v2 Value) bool {
+               switch v1.Kind() {
                case Map, Slice, Ptr, Interface:
-                       return true
+                       // Nil pointers cannot be cyclic. Avoid putting them in the visited map.
+                       return !v1.IsNil() && !v2.IsNil()
                }
                return false
        }
 
-       if v1.CanAddr() && v2.CanAddr() && hard(v1.Kind()) {
-               addr1 := unsafe.Pointer(v1.UnsafeAddr())
-               addr2 := unsafe.Pointer(v2.UnsafeAddr())
+       if hard(v1, v2) {
+               addr1 := v1.ptr
+               addr2 := v2.ptr
                if uintptr(addr1) > uintptr(addr2) {
                        // Canonicalize order to reduce number of entries in visited.
                        // Assumes non-moving garbage collector.