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: should have a length-limited version of C.GoString #12428

Closed
siebenmann opened this issue Sep 1, 2015 · 6 comments

Comments

@siebenmann
Copy link

commented Sep 1, 2015

Right now it's quite tempting and perhaps obvious to use C.GoString() on C struct fields that are declared as char field[N] (especially in light of issue #12427). However this usage is wrong in a subtle and dangerous way, because the traditional common definition of such fields is that their contents are not null-terminated if they contain something exactly N characters long. Use of C.GoString() thus risks running off the end of the field and into unrelated and possibly unallocated memory under rare circumstances.

If you know about this issue it is possible to work around it in Go code (call C.GoStringN() and then remove everything after the first 0 byte). However I think it would be better to provide a support function for this both to make the issue visible to people using cgo and to make it easy to do the right and safe thing.

@adg adg changed the title RFE: cmd/cgo should have a length-limited version of C.GoString() cmd/cgo should have a length-limited version of C.GoString() Sep 1, 2015

@adg

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2015

@ianlancetaylor ianlancetaylor changed the title cmd/cgo should have a length-limited version of C.GoString() cmd/cgo should have a length-limited version of C.GoString Sep 1, 2015

@ianlancetaylor ianlancetaylor changed the title cmd/cgo should have a length-limited version of C.GoString cmd/cgo: should have a length-limited version of C.GoString Sep 1, 2015

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Sep 1, 2015

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2015

That's a fairly specific request. It's not a goal of cgo to provide a full set of C string manipulation functions. If I understand correctly, you can do what you want with

C.GoStringN(p, C.strnlen(p, N))

I don't think we need an additional function for this specific use.

@minux

This comment has been minimized.

Copy link
Member

commented Sep 1, 2015

@siebenmann

This comment has been minimized.

Copy link
Author

commented Sep 1, 2015

With @ianlancetaylor showing the simple way to deal with this, I agree that no additional function is needed. My concern can be dealt with with a documentation note telling people to do this for copying such struct fields instead of just using C.GoString() (and not necessarily in cmd/cgo's godoc; there's the wiki and other supplementary materials on cgo).

(If left alone with no guidance I think that many people will just use C.GoString(), especially if they are not experienced C programmers but are instead just using cgo to connect Go to some C library or the like. Even relatively experienced C programmers sometimes commit this mistake in C code, after all, and it slipped my mind recently when I was writing cgo code. Treating struct fields as C strings is easy and really tempting and it works almost all of the time.)

@minux

This comment has been minimized.

Copy link
Member

commented Sep 1, 2015

@siebenmann

This comment has been minimized.

Copy link
Author

commented Sep 1, 2015

It is a bad API. But it's unfortunately a common one, either explicitly (you're told that this is the case) or implicitly (some struct has a char field[N] field and documentation doesn't say it's always null-terminated). People creating certain sorts of C APIs love embedded fixed-size char fields instead of out of line char * pointers because it requires less allocation and simplifies things like moving these structs across user space/kernel boundaries. And then people writing Go packages to interface with these C APIs get to deal with the mess.

@golang golang locked and limited conversation to collaborators Sep 4, 2016

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