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

recursion/gnark-ffi frees C strings allocated by Go from Rust code #14

Open
huitseeker opened this issue Jun 5, 2024 · 0 comments
Open

Comments

@huitseeker
Copy link
Contributor

From https://github.com/wormhole-foundation/wp1/pull/222#discussion_r1613607167:

The usage of CString by recursion/gnark-ffi seems incorrect when compared to what the Rust documentation says about ownership. Specifically:

  • As far as I can tell, these strings are allocated by the Go code here
  • The CString documentation explicitly says "Other usage (e.g., trying to take ownership of a string that was allocated by foreign code) is likely to lead to undefined behavior or allocator corruption."
  • This means the string is allocated by Go, but being freed by Rust. This seems possibly unsafe.
  • The documentation also says: "Note: If you need to borrow a string that was allocated by foreign code, use CStr. If you need to take ownership of a string that was allocated by foreign code, you will need to make your own provisions for freeing it appropriately, likely with the foreign code’s API to do that."
  • The correct solution would be to return these pointers and have the Go side free them from Go
  • The CGo documentation for strings says:
    // Go string to C string
    // The C string is allocated in the C heap using malloc.
    // It is the caller's responsibility to arrange for it to be
    // freed, such as by calling C.free (be sure to include stdlib.h
    // if C.free is needed).
    func C.CString(string) *C.char
    
  • CString in Rust is actually allocated from the Rust global allocator and not C's malloc/free functions. So this code is freeing something allocated by malloc with Rust's allocator, which is just incorrect in terms of safety
  • Since this code does seem to work, in some cases where Rust and Go are using the same libc on the same system, this is likely to be incidentally fine
  • Further, in some cases, such as when binaries are distributed in pre-compiled formats, it is possible for there to be a version mismatch between libcs used, leading to further issues even if the Rust global allocator ends up using malloc/free
  • This same mishandling is present when parsing the error strings returned by Go
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

1 participant