argumentcomputer / argumentcomputer/sphinx
`recursion/gnark-ffi` frees C strings allocated by Go from Rust code
- Dominant language
- Rust
- Stars
- 80
- Forks
- 8
- PR merge metrics
- No merged PRs in 30d
Description
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](https://doc.rust-lang.org/stable/std/ffi/struct.CString.html#method.from_raw) says about ownership. Specifically:
* As far as I can tell, these strings are allocated by the Go code [here](https://github.com/wormhole-foundation/wp1/blob/a4f06e87cfc40356fb0d2c6c8857d7974494505a/recursion/gnark-ffi/go/main.go#L41-L44)
* 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](https://doc.rust-lang.org/stable/std/ffi/struct.CStr.html). 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](https://pkg.go.dev/cmd/cgo) 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
Contributor guide
Research direction
Start with recursion/gnark-ffi/go/main.go at lines 41–44 and the Rust FFI code that parses the returned error strings; read the linked Rust CString/from_raw and cgo string documentation first. Confirm each allocation’s owner and ensure both normal and error-string paths use the allocator’s supported release mechanism; done means no Rust CString takes ownership of Go-allocated pointers.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100