Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cgo wrappers cannot safely pass uintptr to C functions #33

Open
evanj opened this issue Feb 7, 2021 · 2 comments
Open

Cgo wrappers cannot safely pass uintptr to C functions #33

evanj opened this issue Feb 7, 2021 · 2 comments

Comments

@evanj
Copy link

evanj commented Feb 7, 2021

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
Copy link
Owner

valyala commented Mar 31, 2021

@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
Copy link
Author

evanj commented Mar 31, 2021

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?)

@mhr3 mhr3 mentioned this issue Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants