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

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.

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?

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.

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

@randall77 the mechanism for CFTypeRef seems like the way forward, which would make gomobile correct as well. What more information do you need? Valid jobject (and its aliases j*array) values other than nil/0 can only be generated from JNI functions so the only downside is the nil => 0 conversion.

@worldiety : Thanks for the reference. Where is the code that actually makes one of these jobjects? I'm afraid I don't know my way around the android source tree.

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.

Actually, it is a requirement. If you type an integer as a pointer, bad things can happen. For instance, Go copies stacks. If one of the integers-typed-as-pointers happens to look like a pointer into the Go stack, when the stack is copied the pointer will be adjusted. If the pointer was really an integer then the Go runtime just randomly adjusted your integer for you.

@randall77 My wording was probably a bit misleading. What I wanted to say is actually the same (as you pointed already out): Go needs his managed (unsafe) pointers to be fine, however it must not use (unsafe) bad pointers from c. Lumping the ownerships of both worlds blindly together (which is what cgo does) is the design failure. Other ecosystems - like the Java-Jni - never try to detect an ownership automatically in the FFI logic like Go does. And this automatism is "not a requirement" for Go to work.

I'm not a real Android expert either, but let's try to find a way through it:

• There are a lot of ways to create jobjects, e.g. take AllocObject in line 1504 in https://android.googlesource.com/platform/dalvik/+/tools_r21/vm/Jni.cpp
• The jobject is returned from addLocalReference in line 379, same file,
• which in turn is created by the IndirectRefTable from the given thread in line 387
• Let's switch to the IndirectRefTable. The indirect reference definition is documented in https://android.googlesource.com/platform/dalvik/+/tools_r21/vm/IndirectRefTable.h line 64ff
• The implementation for the add method is in https://android.googlesource.com/platform/dalvik/+/tools_r21/vm/IndirectRefTable.cpp line 61

Please be aware that JNI is also supported by all Java(TM) VMs as well (on perhaps all cgo targets), so the workaround is not limited to Android. Also the way through the Android ART runtime looks totally different.

Proposal A - method level

• Keep everything as it is, so the default behavior is not changed
• introduce a comment annotation to optionally instruct cgo when to use uintptr
• e.g. //bytearray as uintptr

Proposal B - unit level

• Keep everything as it is, so the default behavior is not changed
• introduce a gobal comment to mark all jobject's as uintptr

As we've explained, correct identification of pointers is simply not optional. It would be nice if it were. But Go's garbage collector is designed for optimal handling of Go code, not for Go calling C.

Clearly a JNI-style like approach would be a solution, but I think most people find cgo significantly easier to use than JNI.

I've changed the title of this issue to come up with some mechanism for telling cgo that certain C pointer types must not be treated as Go pointers. If anybody has any suggestions for how to do that, I would like to hear them. Thanks.

Random thoughts, in no particular order:

IndirectKey is the base type of all the trouble here. It's the one that is defined as void* but is made from packed integer fields. It is then cast to jobject I think.

I can't find this IndirectKey in other JVMs, only Dalvik and Art. For instance, I don't see it in OpenJDK anywhere. How sure are we that other JVMs use similar techniques?

Using directives isn't really satisfactory. You'd have to include them in every cgo section of every file (or just package? not sure.) that mentions the type. That's a lot of repetition and prone to error.
To avoid repetition, you could can add the directives to the .h files themselves. Good luck getting all the JVM vendors to include a Go-specific comment.

I don't think we want to map all C pointer types to uintptr. We could do that if we had a JNI-style interface requiring a callback for each dereference. But not the way cgo is currently structured. That is, if we have

typedef Foo struct {
    int x,y,z;
}
typedef Foo *FooPtr;

Then we want to have C.FooPtr as a Go type and be able to do

   var p C.FooPtr = ...
   ... = p.x

An interesting possibility is to map void* to uintptr instead of unsafe.Pointer. Although that doesn't have the aforementioned problem, it would nevertheless still break a lot of cgo. It would also break code that stored real Go pointers in a void* on the cgo side, then passed that void* back into Go (does anyone know of such code?). It also doesn't solve the problem that, at least in the CF*Ref cases, that only the top of the class hierarchy is void*; the subclasses are real typed pointers that we also have to map to uintptr. I think the same problem happens for the JNI case - IndirectKey is a *void but jobject is a *struct _jobject.

We already "hide" pointers in some JNI types. For instance, jvalue is a union with both pointer and scalar members. On the Go side it is just a [N]byte. If you're using C.jvalue, it contains a pointer, and that pointer is to a Go object, then you're in trouble. Maybe we assume jvalues can't point to Go objects?

I also looked through the Avian VM [1] and Graal VM [2]. At first glance, I was not able to spot bad pointers for jobject, but that does not mean anything. There are also many other closed source blackbox JVMs, as listed in [3]. We should not assume, that they have no bad pointers.

[1] https://github.com/ReadyTalk/avian (btw Avian is also able to run on iOS)
[2] https://github.com/smarr/graal (probably the successor of the current OpenJDK JVM)
[3] https://en.wikipedia.org/wiki/List_of_Java_virtual_machines

Change https://golang.org/cl/81876 mentions this issue: cmd/cgo: make JNI's jobject type map to uintptr in Go

CL 81876 contains a possible fix for jobject and friends, similar to what was done for CFTypeRef and friends on darwin.
Opinions welcome on whether we should continue down this one-off route for now.

If anybody has any suggestions for how to do that, I would like to hear them.

I posted an idea to a golang-dev thread that might be worth revisiting in light of this issue, #22218, #19928, #21878, #20275, and perhaps #13467.

The idea is to define a distinguished Go type, which I'm calling unsafe.Void, that specifically refers to an object that cannot reside in Go memory.

The differences between *unsafe.Void, unsafe.Pointer, and uintptr would be:

• uintptr is an integer type: it takes the value 0 instead of nil, and arithmetic can be performed on it.
• unsafe.Pointer is either nil or a pointer to a valid (Go or non-Go) memory address.
• unsafe.Void is a zero-sized type that is not instantiable in Go.

The general rules for non-instantiable types would be:

• A struct with a field of a non-instantiable type is itself non-instantiable, as are maps, slices, channels, and arrays with uninstantiable elements.
• Any expression that produces a value of a non-instantiable type — such as declaring a variable, calling make, or dereferencing a pointer except in an address operatotion — is a compile-time error.
• A pointer-to-non-instantiable type is itself instantiable, and has nil as its zero-value, but the compiler must assume that it never points to Go memory (and thus skip it during garbage collection).
• A pointer to a non-instantiable type can be explicitly converted to and from *unsafe.Void and/or uintptr.
• A pointer to a non-instantiable type can be converted to or from unsafe.Pointer only if it does not point into the Go heap. (This rule may be enforced with a run-time check.)

To stitch that together with cgo, we would map any incomplete C struct type (and, if necessary, complete types in a whitelist) to an uninstantiable Go type.

Unfortunately, this proposal introduces a backward-incompatibility: the C type void* properly maps to *unsafe.Void rather than unsafe.Pointer, and those two types should not be equivalent or even mutually assignable. I'm not sure how to resolve that.

A simpler approach, I suppose, would be to add another structured comment to indicate the type mapping:

// #cgo TYPE_MAP: jobject=uintptr

That would give a migration path for unsafe.Void, but could be used even without it.

The only problem with the structured comment is that it has to appear in every single file that uses cgo with the troublesome type. It's not a fatal problem, but it's less than ideal.

Given that we can't control the C header files, I don't see an alternative in general. The only other option I can think of is to add a (non-portable) annotation to the declaration in the C header to say “this is a sometimes-pointer”, but if we could make changes to the C header in general we could just change the type from a pointer to the more portable uintptr_t to begin with.

On the other hand, if we could address #13467, we could at least recommend that only one Go source file should import a given C header. That would ensure consistency, but isn't feasible today.

Well, just for example, we could adopt your #cgo TYPE_MAP suggestion, and then also say that the go tool will walk up the tree from the directory holding the cgo-using file to GOROOT looking for config.cgo files, and direct cgo to add all such files to the cgo comment.

Change https://golang.org/cl/82917 mentions this issue: doc: add doc about C types that we map to uintptr instead of ptr

We are seeing the same issue as described by @worldiety. We are using a library with the same design - it stores integers in pointers. In our case, the library uses incomplete typedef struct instancePrivate* instance types where instancePrivate is never defined. They could have typed those things uintptr_t but they didn't and C won't ever force them to.
The API is pretty big, so wrapping every function is impractical, albeit not impossible (e.g. by writing a code generator which would be our last resort). Our library is not so widespread that it would be a good idea to special-case the types as done for the JNI types. A customizable solution like the proposal by @bcmills would work for us.

Aside from many instances of APIs using void* for opaque types that store non-pointers (I can add OpenGL to the existing list), there is also the case of void *data style pointers used in callback functions, used to associate callbacks with custom data. In Go, this is particularly required since we have to use generic callback functions that dispatch to concrete Go callbacks. And since we can't pass arbitrary Go pointers into C, void* tends to end up storing arbitrary integers that correspond to map keys. When the void* argument isn't part of the callback signature itself but instead stored in a struct, then invalid unsafe.Pointers end up in Go space.

One such example is wl_resource in libwayland.

@dominikh, you can always use C.malloc to obtain ranges of valid void* addresses to use as map keys.

I'm also facing this after being dinged by bad pointer in frame panics, and the "use a wrapper" solution makes for a lot of boilerplate.

The idea is to define a distinguished Go type, which I'm calling unsafe.Void, that specifically refers to an object that cannot reside in Go memory.

[details snipped]

Unfortunately, this proposal introduces a backward-incompatibility: the C type void* properly maps to *unsafe.Void rather than unsafe.Pointer, and those two types should not be equivalent or even mutually assignable. I'm not sure how to resolve that.

Why not have a special type C.uintptr_void in the C pseudo import that marks an argument as a void * pointer that contains an arbitrary integer value, not necessarily a valid memory pointer into the process' address space. Conversions to/from are enforced from uintptr.

The proposed name uintptr_void follows the existing convention of prefixes struct_, union_, and enum_ for type names. (You could extend the "int-value-in-a-pointer-type" concept to any pointer type T with uintptr_T, but that might be more complexity than we need.)

For example:

package main

/*
extern void my_go_callback(void *arg);

static void my_c_api(void *arg) {
  my_go_callback(arg);
}
*/
import "C"
import "fmt"

func main() {
  C.my_c_api(C.uintptr_void(uintptr(42)))
}

//export my_go_callback
func my_go_callback(arg C.uintptr_void) {
  fmt.Printf("got arg: %d\n", uintptr(arg))
}

(Aside from not requiring wrappers, this approach does not rely on uintptr_t and we don't need to #include <stdint.h> either.)

Is this still a problem? Later Go versions mark opaque C types as go:notinheap which are ignored by the garbage collector. See for example #41761.

See also #41337.