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

cmd/cgo: document that C.CString should be matched with C.free #18457

Closed
danderson opened this issue Dec 28, 2016 · 4 comments

Comments

@danderson
Copy link

commented Dec 28, 2016

Someone in the Go Slack channel had a memory leak in their program, that turned out to be a misunderstanding of how CStrings work. Their code did:

C.myfunc(C.CString(goStr))

They expected that C.CString would be a normal GC-able object, and so lost some time tracking down the non-obvious memory leak. The cgo documentation states that C.CString must be accompanied by a C.free, but if that docstring gets glossed over, the result is a hard to find bug.

It would be nice if go vet could warn on the pattern of calling a C function with a CString allocated inline in the function call. Such a call is most often wrong, since there's no way to free the resulting *C.char. One exception is if the C function itself frees the memory, but that is relatively unusual pattern of memory ownership in C APIs.

@minux

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

@elubow

This comment has been minimized.

Copy link

commented Dec 29, 2016

I was the person who came in with the issue. My initial request was actually documentation since I found the docs that said I should be running the C.free() myself anytime a C.CString() is allocated. The confusion came when I expected the GC to free that on it's own since I was running it in a similar fashion to an inline cast (term used loosely here). So a note in the docs would be incredibly helpful. Additionally, the docs would also be more helpful if they explain that C.free() doesn't just show up when one does an import "C" and that they will likely need to include <stdlib.h> inside the comment section with compilation arguments. Both of those things would have been incredible time savers.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

Let's start with documentation and move to vet if someone can show this meets the frequency bar.

@rsc rsc added this to the Go1.9 milestone Jan 4, 2017

@rsc rsc changed the title Add a `go vet` check for calling C.CString() inline within a function call cmd/cgo: document that C.CString should be matched with C.free Jan 4, 2017

@ALTree

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

This looks fixed. In golang.org/cmd/cgo I see:

// 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

So both the fact that the caller must free and the note about stdlib.h are already there.

Closing.

@ALTree ALTree closed this Jun 2, 2017

@golang golang locked and limited conversation to collaborators Jun 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.