argumentcomputer / argumentcomputer/sphinx

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

Open
#14 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.