Go: reflect: DeepEqual can return true for values that are not equal

[ailurarctos]

What version of Go are you using (go version)?

$ go version
go version go1.14.4 linux/amd64

Does this issue reproduce with the latest release?

Yes, go1.14.4. It does not happen with go1.13.12.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/me/.cache/go-build"

GOENV="/home/me/.config/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GOINSECURE=""

GONOPROXY=""

GONOSUMDB=""

GOOS="linux"

GOPATH="/home/me/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/usr/lib/go"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"

GCCGO="gccgo"

AR="ar"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build795938586=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I ran https://play.golang.org/p/h2Wt6eYJB0B:

package main

import (
    "fmt"
    "reflect"
)

type testingStruct struct {
    f1 *testingStruct
    f2 string
}

func main() {
    s1 := &testingStruct{
        f1: &testingStruct{
            f2: "foo",
        },
    }
    s2 := &testingStruct{
        f1: &testingStruct{
            f2: "foobar",
        },
    }
    fmt.Println(reflect.DeepEqual(s1, s2))
}

What did you expect to see?

I expected it to print "false".

What did you see instead?

It printed "true".

[deadok22]

deepValueEqual uses reflect.Value.ptr directly, which results in the same value of visit struct for (s1, s2) and (s1.f1, s2.f1). It seems deepValueEqual should call reflect.Value.Pointer instead.

Have a look at the addresses printed by this program

[mvdan]

This is working as intended. Please read the docs:

Pointer values are deeply equal if they are equal using Go's == operator or if they point to deeply equal values.

If you still think there's a bug, please explain why.

[networkimprov]

This was "false" in 1.13, which is what I'd expect from the docs you quoted. It's "true" now.

[mvdan]

Apologies, I read the code too fast and didn't notice the difference in the strings "foo" and "foobar".

[go101]

It looks it doesn't matter what is the type of f2.

[davecheney]

Paging @randall77 and @aclements in case this is code gen or linker data structure related.

[ianlancetaylor]

It's a bug in reflect.DeepEqual. It doesn't correctly handle comparing two struct values when the first elements of the structs are pointers.

[ianlancetaylor]

I think the bug was introduced by https://golang.org/cl/191940 for #33907. It's broken in 1.14 and tip but works in 1.13 and earlier.

CC @huandu

@gopherbot Please open backport to 1.14.

[gopherbot]

Backport issue(s) opened: #39635 (for 1.13), #39636 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/238361 mentions this issue: reflect: handling flagIndir in DeepEqual potential cycles

[huandu]

I think the bug was introduced by https://golang.org/cl/191940 for #33907. It's broken in 1.14 and tip but works in 1.13 and earlier.

Yes. This bug is introduced by that change.

As #33907 is still a valid issue and only cyclic interface value can cause stack overflow, I suggest to get ptr value by UnsafeAddr and get interface value by v1.ptr.

[gopherbot]

Change https://golang.org/cl/238626 mentions this issue: [release-branch.go1.14] reflect: handling flagIndir in DeepEqual potential cycles

[davecheney]

@ianlancetaylor @dmitshur did this change make it into the 1.14.5 release? If not, will there be a 1.14.6 and when might we see it?

[dmitshur]

@davecheney 1.14.5 was a security release. 1.14.6 is being released right now (see #40256) and it does include the fix. See backport issue #39636 for more details.

[davecheney]

@dmitshur superb, thanks for your quick response, we have a few folks here at $DAYJOB who are waiting on this fix.