cmd/cgo: provide some mechanism for treating certain C pointer types as uintptr

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tschinke/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bv/6rt5ck194ls704dz9p4c0hlh0000gn/T/go-build985720357=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
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"

What did you do?

I'm not using goMobile but using jni + cgo directly to get maximum control. The code works without any problems on ARM but fails on x86 randomly. At first it looks like the usual cgo check, however I'm quite confident that this is a bug. One variant is this:

E/Go: panic: runtime error: cgo argument has Go pointer to Go pointer
E/Go: goroutine 17 [running, locked to thread]:
E/Go: main.GetPrimitiveArrayCritical.func1(0x5d12ef60, 0x72d00005, 0x72c68a80)
E/Go: /Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x79
E/Go: main.GetPrimitiveArrayCritical(0x5d12ef60, 0x72d00005, 0x62811b64)
E/Go: /Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x23
E/Go: main.Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d12ef60, 0x7c700001, 0x72d00005, 0x0, 0x8, 0x18)
E/Go: /Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/math.go:79 +0x27
E/Go: main._cgoexpwrap_3ee6d16a6a1f_Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d12ef60, 0x7c700001, 0x72d00005, 0x0, 0x8, 0x0)
E/Go: command-line-arguments/_obj/_cgo_gotypes.go:3055 +0x6a

But the cgo statement is wrong, math.go:79 looks like this:

77: //export Java_de_worldiety_snappy_go_GoSnappy_uncompress2
78: func Java_de_worldiety_snappy_go_GoSnappy_uncompress2(env *C.JNIEnv, clazz C.jclass, compressed C.jbyteArray, compressedOffset C.jint, compressedSize C.jint) C.jbyteArray {
79: ptrComp := GetPrimitiveArrayCritical(env, C.jarray(compressed))

To be complete, the GetPrimitiveArrayCritical method is declared like this:
// jni.h:
// void * (JNICALL *GetPrimitiveArrayCritical)(JNIEnv *env, jarray array, jboolean *isCopy);
func GetPrimitiveArrayCritical(env *C.JNIEnv, array C.jarray) unsafe.Pointer {
return C._GoJniGetPrimitiveArrayCritical(env, array)
}

And this:
static void* _GoJniGetPrimitiveArrayCritical(JNIEnv* env, jarray array)
{
return (*env)->GetPrimitiveArrayCritical(env, array, JNI_FALSE);
}

Please correct me, but I can't see any Go-Pointer at all.

What did you expect to see?

a) cgo check should not randomly fail
b) cgo check should be consistent across targets (x86 and arm)
c) a possibility to disable the cgo check at runtime, which works for an android library. Setting the environment variable through Android (Os.setenv or libcore) has no effect, and to set it from Go itself is to late. Give us some public variable like GOGC.

What did you see instead?

Random panics on Dell Venue 8, running Android 4.4 on x86 (32bit), but works on ARMs without problems.

[bradfitz]

There already exists GODEBUG=cgocheck=0 as documented first at https://golang.org/doc/go1.6#cgo.

But if the check is wrong, the check should be fixed.

And if the code is wrong, the code should be fixed.

You shouldn't just set GODEBUG=cgocheck=0 and assume the problem is fixed. You'll likely just crash mysteriously elsewhere instead.

/cc @ianlancetaylor @eliasnaur

[ianlancetaylor]

The definition of a Go pointer is a pointer into Go memory. A Go pointer can have a *C.p type, and indeed this is common. While the cgo checks can be over-broad, they are never wrong about whether a value is a Go pointer or not. The closest they can come to being wrong are declaring a value as a pointer when it should really be an integer, or accidentally using uninitialized values from C.

The checks can be over broad as described at #14210.

In order to find the problem, somebody who can recreate it is going to have to look at the actual values in question, to find the Go pointers.

Thanks for the comments, and I've read a lot of tickets before I've opened this issue.

What I don't get is, that I just receive c pointers as parameters and just pass them to another c function, so I expect that Go should not inspect these values at all. (the *C.JNIEnv and C.jarray which are just forwarded back right into the c world)

And the fact, that it works reliable on other devices.

Btw, we are using good old c with JNI on the same device, which does the same, without ever encountering any crash or unwanted behavior.

@bradfitz Setting the GODEBUG environment variable does not work on Android. There is no possibility to define any Environment variable for an Android App - it is just not designed to support that. I tried to workaround this by using setenv from https://developer.android.com/reference/android/system/Os.html (and other hacks) before the library is loaded into the App's process using System.loadLibrary (the way the Java/Art-VM loads shared libraries) but the environment variables are always empty (os.Environ()) at the Go side.

[ianlancetaylor]

We need to be clear on terminology. When you say you just receive C pointers as parameters, do you mean that those parameters have C types? Or do you mean that they are allocated by calling C.malloc or returned by C functions. Because in the terminology that matters here, only the latter are C pointers.

That is, when you say that you receive C pointers, where do those pointers come from? What allocates the memory to which they point?

The fact that the code works on other platforms is interesting but it doesn't really prove anything. There is evidently some difference on this platform. You need to understand that difference before you can conclude that the right decision is to disable the check.

Code snippet (math.go):

78: var counter = 0
79: //export Java_de_worldiety_snappy_go_GoSnappy_uncompress2
80: func Java_de_worldiety_snappy_go_GoSnappy_uncompress2(env *C.JNIEnv, clazz C.jclass, compressed C.jbyteArray, compressedOffset C.jint, compressedSize C.jint) C.jbyteArray {
81:		//print our loop number
82:		C._GoSyslog(C.CString("run #"+strconv.Itoa(counter)))
83:		counter++
84:
85:		runPart1 := true
86:		runPart2 := true
87:	
88:		if runPart1{
89:			//this has no Go pointers involved, we just pass pointers from malloc/gralloc/ashmem or whatever through, I don't know the internals from Dalvik/ART/Java-VM
90:			//env is a pointer to the jni environment and compressed
91:			ptrComp := GetPrimitiveArrayCritical(env, C.jarray(compressed))
92:			ReleasePrimitiveArrayCritical(env, C.jarray(compressed), ptrComp, 0)
93:		}
94:
95:	if runPart2{
96:		//create some go garbage, the underlying array is managed by Go
97:		dstSlice := make([]byte,5)
98:		//allocate a java array of the same size, this is managed by the Dalvik/ART/Java-VM
99:		jByteArray := NewByteArray(env, C.jsize(len(dstSlice)))
100:	//tell the Dalvik/ART/Java-VM not to move it's array pointer, so that Go can work with it
101:	pinnedPtr := GetPrimitiveArrayCritical(env, C.jarray(jByteArray))
102:	//copy the bytes from the slice into the pinned java array (dst, src, num) using 'workaround' from https://github.com/golang/go/issues/14210
103:	C.memcpy(pinnedPtr, unsafe.Pointer(&dstSlice[0]), C.size_t(len(dstSlice)))
104:	//release the pinning on the java array
105:	ReleasePrimitiveArrayCritical(env, C.jarray(jByteArray), pinnedPtr,0)
106: }
107:
108: return nil
109:}

Code snippet (jni.go)

790: static void* _GoJniGetPrimitiveArrayCritical(JNIEnv* env, jarray array)
791: {
792: 	return (*env)->GetPrimitiveArrayCritical(env, array, JNI_FALSE);
793: }
...
1849: // jni.h:
1850: //     void * (JNICALL *GetPrimitiveArrayCritical)(JNIEnv *env, jarray array, jboolean *isCopy);
1851: func GetPrimitiveArrayCritical(env *C.JNIEnv, array C.jarray) unsafe.Pointer {
1852: 	return C._GoJniGetPrimitiveArrayCritical(env, array)
1853: }

Oberservation 1: runPart1 == true and runPart2 == true

After a few runs (~ 40 - 800) I get "always"

panic: runtime error: cgo argument has Go pointer to Go pointer
 E/Go: goroutine 17 [running, locked to thread]:
 E/Go: main.GetPrimitiveArrayCritical.func1(0x5d12ef70, 0x73000005, 0x72c84ee0)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x79
 E/Go: main.GetPrimitiveArrayCritical(0x5d12ef70, 0x73000005, 0x933a44c0)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x23
 E/Go: main.Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d12ef70, 0x7ca00001, 0x73000005, 0x0, 0x26, 0x18)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/math.go:91 +0x9f
 E/Go: main._cgoexpwrap_e45740c9f64c_Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d12ef70, 0x7ca00001, 0x73000005, 0x0, 0x26, 0x0)
 E/Go: 	command-line-arguments/_obj/_cgo_gotypes.go:3055 +0x6a

This indicates that sending a non-Go-allocated pointer is not allowed, but wait...

Oberservation 1: runPart1 == true and runPart2 == false

just running part 1 is fine, cancelled test after more than 30min, "no" crash at all.

Oberservation 2: runPart1 == false and runPart2 == true

After a few runs (~ 40 - 700) I get "always"

panic: runtime error: cgo argument has Go pointer to Go pointer
 E/Go:goroutine 17 [running, locked to thread]:
 E/Go: main.GetPrimitiveArrayCritical.func1(0x5d1468d0, 0x72d00009, 0x72d00009)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x79
 E/Go: main.GetPrimitiveArrayCritical(0x5d1468d0, 0x72d00009, 0x72d00009)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/jni.go:1852 +0x23
 E/Go: main.Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d1468d0, 0x7f200001, 0x75800005, 0x0, 0x8, 0x18)
 E/Go: 	/Users/tschinke/repos/libs_wdy/go_snappy/go/src/wdy/math.go:101 +0xee
 E/Go: main._cgoexpwrap_69b159f8cea9_Java_de_worldiety_snappy_go_GoSnappy_uncompress2(0x5d1468d0, 0x7f200001, 0x75800005, 0x0, 0x8, 0x0)
 E/Go: 	command-line-arguments/_obj/_cgo_gotypes.go:3055 +0x6a

Observation 3: runPart == false and runPart2 == true and comment memcpy line 103(!)

just running part 1 is fine, cancelled test after more than 30min, "no" crash at all.
Update: crashed after more than 2h

Feelings

• the code lines which cgo complains about, seems not to relate to the real problem
• for me, as someone who writes java-jni-c code bindings regularly, this cgo check behavior does not make sense. Actually they hinder me to get the job done. It feels like "magic", which I don't want at this point.
• Go seems not to "scale" well on FFI

Cause analysis

I used your comment about the pointer terminology as an occasion to find out what "jbytearray" is. In the end, the typedef resolves over jobject to a void*.
E.g. in [1] one can see the method "addLocalReference" which uses "IndirectRefTable.add()"[2] of a thread.
And here we see the crux of the matter: jobject is not a pointer at all in the Dalvik VM, but an artificial generated number, containing bits for table lookups, reference type etc.

[1] https://android.googlesource.com/platform/dalvik/+/tools_r21/vm/Jni.cpp
[2] https://android.googlesource.com/platform/dalvik/+/eclair-release/vm/IndirectRefTable.h

My conclusion / my wish

The cgo checks should never be enabled by default, because Go cannot decide if a pointer is actually a pointer which is allowed to be checked.
Even the developer, who glues libraries together, cannot be made responsible of checking if a pointer is actually a pointer or just a random number.
The implementation difference between Dalvik and ART, which is not expressly specified, but deployed to a billion devices, illustrates such scenario. I don't know how a library will change in the future (e.g. due to a device update), so my cgo code will be fragile in general and is prone to fail because my code is vulnerable to the internals of external bits.

What else can I do, besides disabling or circumvent "any" cgochecks, to write a reliable cgo program?

[ianlancetaylor]

OK, if you a generated number that is not a pointer, then in Go it must not have a pointer type. In Go it must have a type such as uintptr.

I understand your desire, but it can't be done. Go is not C. Go is a garbage collected language, and the garbage collector runs concurrently while the rest of the program is running. The cgo rules were not created to make people's lives difficult; they were created because they are required by the garbage collector. The garbage collector treats every variable with a pointer type as a potential root. If the variable does not hold an actual pointer, if it holds a value that happens to be the same as a pointer to various internal data structures, then the garbage collector will crash. It will crash at a random time during your program's execution.

Disabling the cgo checks will hide the problem for a while, until the garbage collector crashes.

You need to change your code to not use a pointer type for a value that is not a valid pointer.

[randall77]

It might be possible to extend the mechanism in https://go-review.googlesource.com/c/go/+/66332 to handle your case. That fix is for Darwin's CFTypeRef types which are declared as pointers, but sometimes contain integers.

I'd need more info about the C types, their declarations, and their constructors to be sure.

@ianlancetaylor Thanks for giving more insight into this. Perhaps it would be a good idea to have a chapter for cgo and unsafe in the https://golang.org/ref/spec.

However, Keith recognized the problem, which is indeed the same as in https://go-review.googlesource.com/c/go/+/66332.

So I have to insist that the cgo decision - to treat every pointer crossing the FFI to be an unsafe.Pointer - is a fundamental design flaw. Currently there is no way to tell cgo that a c pointer should be an uintptr instead. The approach in gcc.go to hardcode pointer categorization in "badPointerTypedef" can never be a generic solution. Bluntly said, yesterday it was CF*Ref on Darwin, today it is jobject on Dalvik and tomorrow it will be an endless story. We definitely need a change here, so that developers have at least the possibility to have a customizable "badPointerTypedef" somehow.

A tedious workaround may be to create wrappers to hide the bad pointers from cgo (if you ever know them before suffering under random crashes) but it costs a lot of effort. I understand that the intention is to protect the runtime's invariance but it is not a requirement for it to work properly - at least for c-owned pointers and in particular these bad pointers. Other ecosystems with GC do not care either, and they are fine.

@randall77 Thanks for looking into this. jobject is declared in jni.h, which is more or less the same for all NDKs and for Java SE, e.g. https://github.com/aosp-mirror/platform_prebuilt/blob/master/ndk/android-ndk-r8/platforms/android-5/arch-arm/usr/include/jni.h. jni.h defines also various methods to create jobjects, e.g. NewByteArray