Go pointer stored into non-Go memory crashes

[davidmanzanares]

Hi, I tried to enable the debug reporting callback on the go vulkan demo (vulkancube) by setting VulkanDebug() to return true:

func (a *Application) VulkanDebug() bool {
	return true
}

It crashes with:

platform.go:59: vulkan: enabling 3 instance extensions
closer.go:137: run time panic: runtime error: cgo argument has Go pointer to Go pointer
/usr/local/go/src/runtime/cgocall.go:565 (0x414f09)
        cgoCheckArg: cgoCheckUnknownPointer(p, msg)
/usr/local/go/src/runtime/cgocall.go:452 (0x414a74)
        cgoCheckPointer: cgoCheckArg(t, ep.data, t.kind&kindDirectIface == 0, top, cgoCheckPointerFail)
/home/dv/go/src/github.com/vulkan-go/vulkan/vulkan.go:1556 (0x4e4e4c)
        CreateDebugReportCallback.func1: __ret := C.callVkCreateDebugReportCallbackEXT(cinstance, cpCreateInfo, cpAllocator, cpCallback)
/home/dv/go/src/github.com/vulkan-go/vulkan/vulkan.go:1556 (0x4dfad6)
        CreateDebugReportCallback: __ret := C.callVkCreateDebugReportCallbackEXT(cinstance, cpCreateInfo, cpAllocator, cpCallback)
/home/dv/go/src/github.com/vulkan-go/asche/platform.go:95 (0x4f891d)
        NewPlatform: ret := vk.CreateDebugReportCallback(instance, &vk.DebugReportCallbackCreateInfo{
/home/dv/go/src/vulkancube/vulkancube_glfw/main.go:97 (0x523993)
        main: platform, err := as.NewPlatform(app)
/usr/local/go/src/runtime/proc.go:203 (0x43dc3d)
        main: fn()
/usr/local/go/src/runtime/asm_amd64.s:1357 (0x468360)
        goexit: BYTE    $0x90   // NOP
exit status 1

I'm pretty worried about this issue because, when I restored the original code to disable the reporting functionality, but enabled Go's CGO checker to the maximum with GODEBUG=cgocheck=2, vk.CreateInstance crashed hard due to the same problem:

platform.go:59: vulkan: enabling 3 instance extensions
write of Go pointer 0xc00001c1b0 to non-Go memory 0x14acf70
fatal error: Go pointer stored into non-Go memory

runtime stack:
runtime.throw(0x5b3d97, 0x24)
        /usr/local/go/src/runtime/panic.go:774 +0x72
runtime.cgoCheckWriteBarrier.func1()
        /usr/local/go/src/runtime/cgocheck.go:55 +0xa9
runtime.systemstack(0x466194)
        /usr/local/go/src/runtime/asm_amd64.s:370 +0x66
runtime.mstart()
        /usr/local/go/src/runtime/proc.go:1146

goroutine 1 [running, locked to thread]:
runtime.systemstack_switch()
        /usr/local/go/src/runtime/asm_amd64.s:330 fp=0xc0000db7a0 sp=0xc0000db798 pc=0x466290
runtime.cgoCheckWriteBarrier(0x14acf70, 0xc00001c1b0)
        /usr/local/go/src/runtime/cgocheck.go:53 +0xc9 fp=0xc0000db7d8 sp=0xc0000db7a0 pc=0x4157d9
runtime.wbBufFlush(0x14acf70, 0xc00001c1b0)
        /usr/local/go/src/runtime/mwbbuf.go:196 +0x83 fp=0xc0000db808 sp=0xc0000db7d8 pc=0x438913
runtime.gcWriteBarrier(0xc000010900, 0xc000010060, 0xc000010900, 0x14acf60, 0x0, 0x0, 0xc00001c1b0, 0xb, 0xc0000db918, 0x4ba6ad, ...)
        /usr/local/go/src/runtime/asm_amd64.s:1444 +0xae fp=0xc0000db890 sp=0xc0000db808 pc=0x46841e
github.com/vulkan-go/vulkan.(*ApplicationInfo).PassRef(0xc000080300, 0xc000010060, 0x8c4cbc)
        /home/dv/go/src/github.com/vulkan-go/vulkan/cgo_helpers.go:171 +0x28e fp=0xc0000db8e0 sp=0xc0000db890 pc=0x4b9fde
github.com/vulkan-go/vulkan.(*InstanceCreateInfo).PassRef(0xc0000dbc90, 0x0, 0x5cefda)
        /home/dv/go/src/github.com/vulkan-go/vulkan/cgo_helpers.go:342 +0x11d fp=0xc0000db928 sp=0xc0000db8e0 pc=0x4ba6ad
github.com/vulkan-go/vulkan.CreateInstance(0xc0000dbc90, 0x0, 0xc0000120d0, 0x5ad96a)
        /home/dv/go/src/github.com/vulkan-go/vulkan/vulkan.go:21 +0x2b fp=0xc0000db958 sp=0xc0000db928 pc=0x4dd4eb
github.com/vulkan-go/asche.NewPlatform(0x5d8500, 0xc0000100a0, 0x5af90d, 0x11, 0x0, 0x0)
        /home/dv/go/src/github.com/vulkan-go/asche/platform.go:75 +0x4a5 fp=0xc0000dbdb8 sp=0xc0000db958 pc=0x4f7415
main.main()
        /home/dv/go/src/vulkancube/vulkancube_glfw/main.go:97 +0x1d4 fp=0xc0000dbf60 sp=0xc0000dbdb8 pc=0x523994
runtime.main()
        /usr/local/go/src/runtime/proc.go:203 +0x21e fp=0xc0000dbfe0 sp=0xc0000dbf60 pc=0x43dc3e
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:1357 +0x1 fp=0xc0000dbfe8 sp=0xc0000dbfe0 pc=0x468361

goroutine 6 [syscall]:
os/signal.signal_recv(0x0)
        /usr/local/go/src/runtime/sigqueue.go:147 +0x9c
os/signal.loop()
        /usr/local/go/src/os/signal/signal_unix.go:23 +0x22
created by os/signal.init.0
        /usr/local/go/src/os/signal/signal_unix.go:29 +0x41

goroutine 8 [select]:
github.com/xlab/closer.(*closer).wait(0xc0000bc000)
        /home/dv/go/src/github.com/xlab/closer/closer.go:104 +0xe8
created by github.com/xlab/closer.newCloser
        /home/dv/go/src/github.com/xlab/closer/closer.go:96 +0x1cf
exit status 2

Is this a false positive in the go tooling? Is this a real issue with these bindings? If so, I'd gladly try to help

[jclc]

Are you using the Go race detector? It caused similar errors for me when I knew for sure that there was nothing wrong.

[xlab]

I'm pretty sure this is the issue:

1. github.com/vulkan-go/vulkan/cgo_helpers.go:171:

	var cpApplicationName_allocs *cgoAllocMap
	refb0af7378.pApplicationName, cpApplicationName_allocs = unpackPCharString(x.PApplicationName)
	allocsb0af7378.Borrow(cpApplicationName_allocs)

2. github.com/vulkan-go/vulkan/cgo_helpers.go:342:

// unpackPCharString represents the data from Go string as *C.char and avoids copying.
func unpackPCharString(str string) (*C.char, *cgoAllocMap) {
	h := (*stringHeader)(unsafe.Pointer(&str))
	return (*C.char)(h.Data), cgoAllocsUnknown
}

When c-for-go project started in 2015, it was relatively safe practice to pass memory behind string into C land without copying. Nowadays it's considered not safe, moreover, this particular pointer to Go memory is stored inside C struct memory. Strings are immutable by definition, but in practice who knows how Go memory is organized.

I'll need to fix vulkan-go bindings to so unpackPCharString would copy strings. And add \x00 too.

[jclc]

Perhaps a rewrite for the bindings would be in order? The bindings have worked fine for me but there are small issues like this and also some performance and memory safety questions I'm worried about, particularly with regards to wrapped structs.

[xlab]

@jclc as been discussed in xlab/c-for-go#86 performance and memory safety questions with wrapped structs is a phantom problem — I've never seen any reasonable numbers, and for my use cases there is no issues at all..

There are some tricky parts in binding generation, like callbacks, especially with custom memory allocators for Vulkan. I think this is a standalone feature that needs to be improved. But there is no point in debates about performance vs safety. If you need both, then one should use Rust.

Regarding this particular case with zero-alloc string — initially this has beed implemented as a way to avoid allocation pressure. Which turns out to be not so safe and not so useful in 2020. So I'll add additional mem copy for (unnoticeable) sacrifice of performance.

[davidmanzanares]

Thank you!

No, I haven't used the race detector.

I'll need to fix vulkan-go bindings to so unpackPCharString would copy strings. And add \x00 too.

That would be amazing, I noticed the \x00 too, it would be a nice plus.

Btw, thank you very much for doing this project, despite GC, runtime, and all of that, I've had great experiences with Go and OpenGL in the past, it's really nice to be able to use vulkan now.

[mrjoshuak]

I tried the naive approach of simply replacing unpackPCharString and packPCharString with the cgo equivalents C.CString and C.GoString respectively but this did not seem to fix the issue.