Mysterious error: "cgo argument has Go pointer to unpinned Go pointer"

[fiatjaf]

I'm very confused about this and even asked this question on StackOverflow, but I figured I would repeat it here in case someone has any incredible insight: https://stackoverflow.com/questions/77608920/mysterious-error-cgo-argument-has-go-pointer-to-unpinned-go-pointer

I was using bmatsuo's version but recently have switched to this fork and the problem persists unchanged and that has me very confused.

I am even more confused because I'm using bmatsuo's library in another project in which looks like to me the exact same way, but there I don't see this issue and here I do.

Anyway, I'm just looking for some hint in case anyone knows anything about what is going on.

[wojas]

Thank you for submitting this. This may actually be a bug in this library.

From your Stack Overflow post:

UPDATE: With a little more digging I've discovered that the parameter causing the trouble is c.txn.key, which is outside of my control and seems to be set to new(C.MDB_val).

I don't think it is safe to use new() to allocate memory for a C struct if this is meant to be referenced by other C code. I have a feeling we need to use C.malloc for this and ensure a corresponding C.free happens on GC.

This is used for C structs in the following places.

./lmdb/txn.go
		if flags&Readonly == 0 {
			// In a write Txn we can use the shared, C-allocated key and value
			// allocated by env, and freed when it is closed.
			txn.key = env.ckey
			txn.val = env.cval
		} else {
			// It is not easy to share C.MDB_val values in this scenario unless
			// there is a synchronized pool involved, which will increase
			// overhead.  Further, allocating these values with C will add
			// overhead both here and when the values are freed.
94:			txn.key = new(C.MDB_val)
95:			txn.val = new(C.MDB_val)

So this is only done during readonly transactions, apparently as an optimization.
Perhaps we can instead add a scratch space to the Go Txn struct and set these pointers to the internal memory. No idea if that helps. Alternatively, we can use malloc/free here, but then we may need to use runtime.SetFinalizer to free the memory, which is probably the overhead mentioned here.

./lmdb/env.go
// FD returns the open file descriptor (or Windows file handle) for the given
// environment.  An error is returned if the environment has not been
// successfully Opened (where C API just retruns an invalid handle).
//
// See mdb_env_get_fd.
func (env *Env) FD() (uintptr, error) {
	// fdInvalid is the value -1 as a uintptr, which is used by LMDB in the
	// case that env has not been opened yet.  the strange construction is done
	// to avoid constant value overflow errors at compile time.
	const fdInvalid = ^uintptr(0)
116:	mf := new(C.mdb_filehandle_t)

The handle is immediately returned. It seems like the only reason new was used is because the size of mdb_filehandle_t is not known in advance, but in practice it's at most a uint64 on current hardware. This function is not performance sensitive, so we could do explicit malloc/free here.

UPDATE: The FD usage is fine, since we do not retain the pointer.

[wojas]

Alternatively, we can use malloc/free here, but then we may need to use runtime.SetFinalizer to free the memory, which is probably the overhead mentioned here.

It looks like Txn already has a finalize that is registered when the transaction is created, so there is no good reason not to use malloc here. We could even slightly optimize it by allocating and freeing a [2]C.MDB_val in one call, one for the key and one for the value.

[shane-ns1]

Alternatively, we can use malloc/free here, but then we may need to use runtime.SetFinalizer to free the memory, which is probably the overhead mentioned here.

It looks like Txn already has a finalize that is registered when the transaction is created, so there is no good reason not to use malloc here. We could even slightly optimize it by allocating and freeing a [2]C.MDB_val in one call, one for the key and one for the value.

The most direct solution is for sure just using C.malloc(), and adding C.free() to the existing finalize when necessary. 👍

[wojas]

@fiatjaf PR #29 should resolve your error.

[wojas]

@fiatjaf I also noticed in your query.go that you are spawning goroutines within a env.View, which is not safe.

The env.View explicitly locks the goroutine to an OS thread for LMDB locking. Spawning goroutines throws all of that out of the window. LMDB transactions cannot safely be used from multiple threads, and lmdb-go reuses buffers within a transaction that will almost certainly cause data races.

This spawning of goroutines may have prevented certain Go compiler optimizations that have previously hidden this allocation bug.

If you need concurrency, spawn multiple LMDB read transactions, they are cheap.

[wojas]

For future reference (emphasis mine):

All Go pointers passed to C must point to pinned Go memory. Go pointers passed as function arguments to C functions have the memory they point to implicitly pinned for the duration of the call. Go memory reachable from these function arguments must be pinned as long as the C code has access to it. Whether Go memory is pinned is a dynamic property of that memory region; it has nothing to do with the type of the pointer.

Go values created by calling new, by taking the address of a composite literal, or by taking the address of a local variable may also have their memory pinned using runtime.Pinner. This type may be used to manage the duration of the memory's pinned status, potentially beyond the duration of a C function call. Memory may be pinned more than once and must be unpinned exactly the same number of times it has been pinned.

Go code may pass a Go pointer to C provided the memory to which it points does not contain any Go pointers to memory that is unpinned. When passing a pointer to a field in a struct, the Go memory in question is the memory occupied by the field, not the entire struct. When passing a pointer to an element in an array or slice, the Go memory in question is the entire array or the entire backing array of the slice.

Source: https://pkg.go.dev/cmd/cgo#hdr-Passing_pointers

The issue is caused by violation of item 3. The FD() usage, on the other hand, is allowed because of item 1.

[fiatjaf]

@fiatjaf I also noticed in your query.go that you are spawning goroutines within a env.View, which is not safe.

The env.View explicitly locks the goroutine to an OS thread for LMDB locking. Spawning goroutines throws all of that out of the window. LMDB transactions cannot safely be used from multiple threads, and lmdb-go reuses buffers within a transaction that will almost certainly cause data races.

This spawning of goroutines may have prevented certain Go compiler optimizations that have previously hidden this allocation bug.

This was a very helpful comment. The other project I was using the library in didn't have the goroutine stuff, that's why it wasn't panicking like the current. Thanks.

[DeeptimanQlik]

We also see similar error with boringcrypto library. When we try to build the Go repository with the build variants //go:build boringcrypto application fails to start with the exact same error shown in this issues. Any suggestions how can we resolve the cgo runtime error with boringcrypto?

//go:build boringcrypto
// +build boringcrypto

package main

import (
	_ "crypto/tls/fipsonly"
	"log"
     
       ------
       "libraries imports [mongo-go-driver, solace-event-messaging]"
)

func init() { //nolint:gochecknoinits
	log.Println("fips-compliant crypto enforced")
}

func main() {
  // application code runs here
}

CGO_ENABLED=1 GOEXPERIMENT=boringcrypto go build -o example main.go

Error Message

panic: runtime error: cgo argument has Go pointer to unpinned Go pointer
goroutine 1 [running]:
solace.dev/go/messaging/internal/ccsmp.(*SolClientContext).SolClientSessionCreate.func1.1(0x7f78f10cc5b8?, 0x30?, 0xc000640000?, 0xc000aca7e0?)	
/go/src/github.com/example/vendor/solace.dev/go/messaging/internal/ccsmp/ccsmp_core.go:265 +0x51solace.dev
/go/messaging/internal/ccsmp.(*SolClientContext).SolClientSessionCreate.func1()	
/go/src/github.com/example/vendor/solace.dev/go/messaging/internal/ccsmp/ccsmp_core.go:265 +0x25