reflect: Value.Interface "panic: bad indir" after go1.15.4+

[Robbie-Perry]

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

$ go version
go version go1.16.3 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\REDACTED\AppData\Local\go-build
set GOENV=C:\Users\REDACTED\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\REDACTED\go\pkg\mod
set GONOPROXY=none
set GONOSUMDB=gitscm.REDACTED.ca
set GOOS=windows
set GOPATH=C:\Users\REDACTED\go
set GOPRIVATE=gitscm.REDACTED.ca
set GOPROXY=direct
set GOROOT=C:\Go
set GOSUMDB=off
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\ro006714\go\src\gitscm.REDACTED.ca\REDACTED\gorm-issues\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\REDACTED\AppData\Local\Temp\go-build049852886=/tmp/go-build -gno-record-gcc-switches

What did you do?

I'm using a C Library called GEOS which provides a C-backed implementation for handling Geometric operations. A struct containing this C type was used during some reflection operations, but upon upgrading to Go v1.15.4 (Or any version after, including latest) this results in a panic.

I've isolated the problem, and it appears that when using a reflect Type to create a new reflect Value, a subsequent call of Interface() results in the panic: bad indir:

var test *C.GEOSGeometry

reflectType := reflect.ValueOf(test).Type().Elem()
value := reflect.New(reflectType)

value.Interface() // Panics in 1.15.4+

What did you expect to see?

No code within the underlying C library nor the use of the reflection changed, only the output of Interface(). I'd have expected to see the value being returned the same as before the update.

What did you see instead?

panic: bad indir

This is coming from func packEface inside reflect/value.go where the following occurs:

        ...
switch {
case ifaceIndir(t):
	if v.flag&flagIndir == 0 {
		panic("bad indir")
	}
        ...

Note: Here v.flag resolves to 22 (flagIndir is 128)

[ianlancetaylor]

Does your program work with the 1.15.3 release? Or, which releases does your program work with?

What is the GEOSGeometry type in the C code?

[Robbie-Perry]

Does your program work with the 1.15.3 release? Or, which releases does your program work with?

@ianlancetaylor Yes, 1.15.3 was the last release that worked. In that version, in the switch statement, ifaceIndir(t) returns false instead, as t.kind = 54 (and kindDirectIface = 32), resulting in the default section of the switch to be executed:

default:
	// Value is direct, and so is the interface.
	e.word = v.ptr

In v1.16.3, t.kind = 22, resulting in ifaceIndir(t) returning true.

What is the GEOSGeometry type in the C code?

It's coming from the GEOS library, and I think it's being created somewhere in preprocessing, so as such I'm not sure exactly what it looks like. What I can do is provide steps to reproduce this:

This example requires the libgeos-dev package which can be installed on most systems using a package manager.. on Ubuntu for example:

sudo apt install -y libgeos-dev

Sample application:

package main

/*
#include "geos_c.h"
*/
import "C"
import (
	"fmt"
	"reflect"
)

func main() {
	var geom *C.GEOSGeometry
	t := reflect.ValueOf(geom).Type().Elem()
	v := reflect.New(t)
	fmt.Println("v.CanInterface(): ", v.CanInterface())
	v.Interface() // Panics in 1.15.4+
}

The program completes successfully when compiled with go1.15.3 and panics with go1.15.4+.
Examples are provided for 1.15.3, 1.15.4 and 1.16.3
Note: in all cases v.CanInterface() returns true.

go version
go version go1.15.3 linux/amd64

go run main ; echo EXITED $?
v.CanInterface():  true
EXITED 0

--

go version
go version go1.15.4 linux/amd64

go run main ; echo EXITED $?
v.CanInterface():  true
panic: bad indir

goroutine 1 [running]:
reflect.packEface(0x4a2aa0, 0x588650, 0x16, 0x0, 0x0)
	/usr/local/go/src/reflect/value.go:113 +0xed
reflect.valueInterface(0x4a2aa0, 0x588650, 0x16, 0x1, 0x2, 0x18)
	/usr/local/go/src/reflect/value.go:1046 +0xe9
reflect.Value.Interface(...)
	/usr/local/go/src/reflect/value.go:1016
main.main()
	/home/user/src/main/main.go:17 +0x1bb
exit status 2
EXITED 1

--

go version
go version go1.16.3 linux/amd64

go run main ; echo EXITED $?
v.CanInterface():  true
panic: bad indir

goroutine 1 [running]:
reflect.packEface(0x4a0a40, 0x57c660, 0x16, 0x0, 0x18)
	/usr/local/go/src/reflect/value.go:113 +0xed
reflect.valueInterface(0x4a0a40, 0x57c660, 0x16, 0x1, 0x2, 0x18)
	/usr/local/go/src/reflect/value.go:1046 +0xe9
reflect.Value.Interface(...)
	/usr/local/go/src/reflect/value.go:1016
main.main()
	/home/user/src/main/main.go:17 +0x1bb
exit status 2
EXITED 1

@ianlancetaylor let me know if there's anything else I can provide. Thanks

[randall77]

I believe this is due to the go:notinheap changes we made. In a copy of geos_c.h I found, we have:

typedef struct GEOSGeom_t GEOSGeometry;

GEOSGeometry is a type defined as an incomplete struct type. I believe we would make that a go:notinheap type.
Allocating such a type in Go is certain not to work - we don't know how big it is, for example. This program attempts to allocate such a type.

Did this ever work? It used to compile, probably, but did the thing you allocated here actually work correctly? Try printing t.Size() for the t in your program and tell us what it prints. Likely, you're allocating a zero-sized thing in Go. C thinks it is not zero-sized, so it is reading and writing to some random place in the Go runtime that happens to not crash everything.

I think there is something to fix here, though. The allocation shouldn't succeed - we should fail there, instead of a random later spot.

[randall77]

Standalone repro:

package main

// typedef struct S T;
import "C"
import "reflect"

func main() {
	var x *C.T
	t := reflect.ValueOf(x).Type().Elem()
	v := reflect.New(t)
	v.Interface()
}

Fails from 1.15.4 through tip.

[randall77]

Note that if you add _ = new(C.T), you get an appropriate allocation error:

./issue45451.go:12:9: _Ctype_struct_S can't be allocated in Go; it is incomplete (or unallocatable)

I think it is just the reflect allocation path that isn't being checked (because the error above comes from the compiler - we probably need a runtime check in reflect).

[ianlancetaylor]

Thanks. I think this was caused by the fix for #42076.

Here is a standalone test case:

package main

// typedef struct unknown u;
import "C"

import "reflect"

func main() {
	var u *C.u
	t := reflect.ValueOf(u).Type().Elem()
	v := reflect.New(t)
	v.Interface()
}

I have a patch.

CC @randall77

I want to note that your example code is very unlikely to do what you want. Presumably the real code is different. This code is allocating a struct value, where the struct type is left undefined in the C header file and as such is not visible to Go. The effect is going to be to allocate a zero-typed struct, which is almost certainly not the C definition. Using this struct for anything, such as passing it to C, is quite likely to fail. It's basically the equivalent of writing, in C, "GEOSGeometry g;", for which the C compiler will give an error storage size of ‘g’ isn’t known. Since you are using reflection here, there is no facility to see a similar error in your dynamic Go code.

Because this code is unlikely to work correctly, it's not clear to me whether we should backport the fix or not. Can you say more about how your code uses the value created by reflect.New(t).Interface()? If the code, for example, passes that value to C, it's not going to work. Thanks.

[gopherbot]

Change https://golang.org/cl/308970 mentions this issue: reflect: make go:notinheap pointers indirect in New

[ianlancetaylor]

@randall77 Sorry for the overlapping messages, for some reason I didn't see your comments.