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

unsafe docs and SliceHeader: I am not sure what is defined here #33794

Closed
seebs opened this issue Aug 23, 2019 · 4 comments
Closed

unsafe docs and SliceHeader: I am not sure what is defined here #33794

seebs opened this issue Aug 23, 2019 · 4 comments

Comments

@seebs
Copy link
Contributor

seebs commented Aug 23, 2019

What version of Go are you using (go version)?

1.12, but N/A

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

Read the documentation for unsafe.

What did you expect to see?

Something that did not confuse me.

What did you see instead?

Something that confused me.

In general, reflect.SliceHeader and reflect.StringHeader should be used only as *reflect.SliceHeader and *reflect.StringHeader pointing at actual slices or strings, never as plain structs. A program should not declare or allocate variables of these struct types.

This has an accompanying example:

// INVALID: a directly-declared header will not hold Data as a reference.
var hdr reflect.StringHeader
hdr.Data = uintptr(unsafe.Pointer(p))
hdr.Len = n
s := *(*string)(unsafe.Pointer(&hdr)) // p possibly already lost

What's not obvious to me: Is the only reason this is "INVALID" that p might already be lost, such that a runtime.KeepAlive(p) after the last line would have saved it? Or is this also intended to cover the possibility that some future version of Go may have additional unspecified magic in a slice object which is not in the SliceHeader parts, such that simply creating a SliceHeader (or a StringHeader) doesn't actually make a thing which could be a valid object?

I've been writing code which has a pointer (which is persistent, and in a data structure which survives past the current scope, thus at no risk of being garbage collected), and which does something to the effect of:

asUint16 := *(*[]uint16)(unsafe.Pointer(&reflect.SliceHeader{Data: uintptr(unsafe.Pointer(obj.ptr)), Len: int(obj.len), Cap: int(obj.len)}))

In fact, obj.ptr isn't going away, so I'm not worried about garbage collection. But if an actual Slice were not exactly the same size as a SliceHeader, but rather, the SliceHeader were just a prefix of the actual internal slice representation... This code would in fact be just plain wrong, because other code might try to access the "rest" of the slice object, and not find it.

Instead, I'd need to do something like:

var u []uint16
h := (*reflect.SliceHeader)&u
h.Data, h.Len, h.Cap = uintptr(obj.ptr), obj.len, obj.len

Which is, I suppose, certainly no harder to read, but I'm not sure whether it offers any semantic difference, apart from u holding the reference to the pointer, in a way that a temporary slice header object wouldn't.

(Related-ish: #20171, #19367.)

@ianlancetaylor
Copy link
Contributor

In general using reflect.SliceHeader in any way other than as described is unsafe because you will be working with a pointer that has the type uintptr. If any implementation of Go ever has a moving garbage collector, that code will break since the uintptr will not be updated if the pointer moves. Even today that could happen due to stack copying if the pointer stored as a uintptr does not escape.

You don't need reflect.SliceHeader to write your asUint16 code. You can write

asUint16 := (*[1<<29]uint16)(unsafe.Pointer(obj.Ptr))[:obj.len:obj.len]

The temporary conversion to a pointer to a very large array is harmless as we immediately the array pointer into a slice of the desired length.

@seebs
Copy link
Contributor Author

seebs commented Aug 23, 2019

Ah-hah! That's the thing I was missing -- not just that the pointer could be freed, but that the pointer could become invalidated.

I have been distrustful of the "very large array" idiom because in theory that means that the subexpression before the slice expression is possibly-invalid, although i suppose if the compiler is trusted not to actually try to build the object before continuing on to slicing it, it's harmless. It still bugs me in that it seems semantically-wrong.

@ianlancetaylor
Copy link
Contributor

Note that it's a pointer to an array. There is no actual array value at any point. What we're doing is turning an unsafe.Pointer into a pointer to a very large array value, and then slicing the array pointer to return a (hopefully valid) slice. Go permits slicing an array pointer: the array pointer becomes the slice's underlying array.

So while the construct is certainly sketchy, and can fail on a 64-bit system if the underlying array is unreasonably large, it doesn't rely on trusting the compiler. The meaning is clearly defined by the language spec.

@seebs
Copy link
Contributor Author

seebs commented Aug 23, 2019

Oh! I have seen that idiom before dereferencing the array before slicing. I hadn't realized you could do it without the dereference. Thanks! That's really neat.

@golang golang locked and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants