reflect: promoted pointer-methods created by StructOf corrupt memory

[Merovius]

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

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mero/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mero"
GORACE=""
GOROOT="/home/mero/go"
GOTMPDIR=""
GOTOOLDIR="/home/mero/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build038207887=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is from reddit. I reduced it to this example:

https://play.golang.org/p/vkdabA1W6mI

What did you expect to see?

struct { *main.X; main.Y }{X:(*main.X)(nil), Y:main.Y{}}
runtime error: invalid memory address or nil pointer dereference
struct { X *main.X; Y main.Y }{X:(*main.X)(nil), Y:main.Y{}}
runtime error: invalid memory address or nil pointer dereference

What did you see instead?

struct { *main.X; main.Y }{X:(*main.X)(nil), Y:main.Y{}}
runtime error: invalid memory address or nil pointer dereference
struct { X *main.X; Y main.Y }{X:(*main.X)(nil), Y:main.Y{}}
struct { X *main.X; Y main.Y }{X:(*main.X)(0x2a), Y:main.Y{}}

---

The second two lines show the magic allocation: The field (containing an *X) is first nil, then v.Set is called, referring to the embedded field, which should panic, as it uses the receiver (which should be nil), but doesn't. Afterwards, the field is magically allocated.

It seems, this is related to there only being a single field - if we add a second field, even of zero size, the program behaves as expected: https://play.golang.org/p/Kd4igw-vOp7. I assume this is in part related to the work on #15924 and similar issues.

[bcmills]

See also #18617.

[heschi]

"Afterwards, the field is magically allocated."

I think OP misread the output. It hasn't allocated an X and put 42 in it, it wrote 42 to the *main.X field, which is total nonsense. Maybe the compiler forwarded the implementation for (main.X).Set instead of (*main.X).Set?

[bcmills]

Maybe the compiler forwarded the implementation for (main.X).Set instead of (*main.X).Set?

Probably the compiler passed the address of the outer struct as the receiver to (*X).Set instead of dereferencing the X field.

[heschi]

Yeah, I was about to edit my comment. If you add some logging it's clear that's exactly what's happening. I bet this has to do with direct ifaces somehow...

[Merovius]

@heschik You are right, I misread that. That makes it even worse, though, as it allows doing unsafe operations without importing unsafe (somewhat crude PoC, it's probably possible to do it more elegantly).

[mdempsky]

@heschik and I looked into this briefly. I'm pretty sure this is because StructOf directly promotes the methods of embedded pointer types, but in general those require wrappers, which it's not creating.

[mdempsky]

For

type T struct { *X; Y }

the promoted (*T).Set method needs to be:

func (t *T) Set() {
    (*(*t).X).Foo = 42
}

I.e., two pointer indirections.

But because it's directly reusing the underlying

func (x *X) Set() {
    (*x).Foo = 42
}

method, the result is effectively instead:

func (t *T) Set() {
    (*t).X = (*X)(unsafe.Pointer(uintptr(42)))
}

[heschi]

Specifically, I think the problem is here:

go/src/reflect/type.go

Lines 2476 to 2481 in 95e6a9f

	methods = append(methods, method{
	name: resolveReflectName(mname),
	mtyp: resolveReflectType(ptr.typeOff(m.mtyp)),
	ifn: resolveReflectText(ptr.textOff(m.ifn)),
	tfn: resolveReflectText(ptr.textOff(m.tfn)),
	})

It's taking the interface method from *X and using it in the generated type. But *X is a direct interface type, and skips one level of indirection. The runtime-generated type is not direct and needs the second level. When we do this operation for an embedded interface, we check whether the embedded type was indirect and generate a wrapper with an extra indirection if so:

go/src/reflect/type.go

Lines 2419 to 2436 in 95e6a9f

	if ft.kind&kindDirectIface != 0 {
	tfn = MakeFunc(mtyp, func(in []Value) []Value {
	var args []Value
	var recv = in[0]
	if len(in) > 1 {
	args = in[1:]
	}
	return recv.Field(ifield).Method(imethod).Call(args)
	})
	ifn = MakeFunc(mtyp, func(in []Value) []Value {
	var args []Value
	var recv = in[0]
	if len(in) > 1 {
	args = in[1:]
	}
	return recv.Field(ifield).Method(imethod).Call(args)
	})
	} else {

We probably need something like that.

[gopherbot]

Change https://golang.org/cl/121316 mentions this issue: reflect: prevent additional StructOf embedded method cases