Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: promoted pointer-methods created by StructOf corrupt memory #24782

Closed
Merovius opened this issue Apr 9, 2018 · 9 comments
Closed

reflect: promoted pointer-methods created by StructOf corrupt memory #24782

Merovius opened this issue Apr 9, 2018 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Merovius
Copy link
Contributor

Merovius commented Apr 9, 2018

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
Copy link
Contributor

bcmills commented Apr 9, 2018

See also #18617.

@bcmills bcmills added this to the Go1.11 milestone Apr 9, 2018
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 9, 2018
@heschi
Copy link
Contributor

heschi commented Apr 9, 2018

"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
Copy link
Contributor

bcmills commented Apr 9, 2018

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
Copy link
Contributor

heschi commented Apr 9, 2018

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
Copy link
Contributor Author

Merovius commented Apr 9, 2018

@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).

@Merovius Merovius changed the title reflect: Embedded pointer fields created by StructOf get magically allocated reflect: promoted pointer-methods created by StructOf corrupt memory Apr 10, 2018
@mdempsky
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

heschi commented Apr 10, 2018

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
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants