Cgo wrappers cannot safely pass uintptr to C functions

[evanj]

This package passes uintptr to C functions, which is not safe. It can cause memory corruption in rare cases. I did not try to reproduce this bug with this package, because it is subtle. However, we have a package with similar code, and we ended up with a memory corruption bug: DataDog/zstd#90 . I'm filing this issue because I noticed this package uses the same pattern, so it probably has the same problem.

The unlucky sequence is:

• Code evaluates the uintptr from a Go pointer to a variable on the stack.
• Code calls the Cgo wrapper.
• The Go runtime decides this goroutine needs a larger stack, and copies it somewhere else. It fixes the pointers to variables on the stack. It cannot fix the uintptr variable, since it does not think it is a pointer.
• Another thread uses the previous stack and changes the contents.
• The C code casts the uintptr to a pointer, then reads from the wrong memory.

One example piece of code that does this is: https://github.com/valyala/gozstd/blob/master/gozstd.go#L17 , however there are many others. In many cases, I think using unsafe.Pointer instead will not impact the performance of gozstd. For example, the Writer API already copies data into its own heap-allocated buffer, before calling Cgo.

For details, see my reproduction of this problem: https://github.com/evanj/cgouintptrbug

[valyala]

@evanj , thanks for the detailed analysis. It looks like the bug can be triggered only if the byte slice b is allocated on a stack for the call C.f(uintptr(unsafe.Pointer(&b[0]))). This issue doesn't apply if b is allocated on a heap, since the &b[0] address doesn't change during stack growth.

[evanj]

That is my analysis as well. However, in that case, I think this means the optimization of using uintptr_t is useless: my understanding is this optimization allows the compiler to be able to allocate objects on the stack. If they aren't ever allocated on the stack, you might as well use unsafe.Pointer because it is both safe and equivalent in terms of memory allocations (I think?)

[ShangguanHong]

@evanj , thanks for the detailed analysis. It looks like the bug can be triggered only if the byte slice b is allocated on a stack for the call C.f(uintptr(unsafe.Pointer(&b[0]))). This issue doesn't apply if b is allocated on a heap, since the &b[0] address doesn't change during stack growth.
Hello, I have a problem on my end that may be related to this issue.
When I use
Decompress(nil, []byte{40, 181, 47, 253, 228, 122, 118, 105, 67, 140, 234, 85, 20, 159, 67}) I get the error panic: runtime error: growslice: cap out of range when decompressing. This is actually the binary data compressed using the dictionary set mode; the other set of data ([]byte{40, 181, 47, 253, 35, 77, 62, 160, 2, 2, 17, 0, 0, 123, 125}) shows the normal error decompression error: Dictionary mismatch.
This error is reproduced on both linux and mac, using go version 1.17.8.