Stack based slice usage and unsafe

[Sergey Kamardin]

Hello gophers,

I have a question which relates mostly to the ideology of unsafe usage.

In `github.com/gobwas/ws` WebSocket library I used an optimization for
reading WebSocket frame headers into stack based slices (to reduce the
number of heap allocations):

func ReadHeader(r io.Reader) (Header, error) {
var (
b [16]byte
bts []byte
)
h := (*reflect.SliceHeader)(unsafe.Pointer(&bts))
*h = reflect.SliceHeader{
Data: uintptr(unsafe.Pointer(&b)),
Len: len(b),
Cap: len(b),
}
_, err = io.ReadFull(r, bts)

// process bytes and return result.
}

This works fine on Linux for a long time and under the high load, but on
Windows it sometimes crashes or has an unexpected behaviour.

Crashes are happened with the message like:
> fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

I may assume that the bad pointer errors are relate to the
internal/poll/fd_windows.go operation buffer implementation, which
stores pointer to the first byte of given destination buffer – and since
the buffer is stack based GC throws such error.

I may assume that the unexpected behaviour happens after goroutine stack
moving/copying but can not reproduce it yet during tests.

But nevertheless, my question is: is it legal to use unsafe in that way
in general? Should I split the implementation of ReadHeader() to support
windows separately without stack based slices?


References:
https://github.com/golang/go/blob/d97bd5d07ac4e7b342053b335428ff9c97212f9f/src/internal/poll/fd_windows.go#L526
https://github.com/gobwas/ws/blob/89d0ae05650f2cc04354d7fef282df0db5acff80/read.go#L18
https://github.com/gobwas/ws/issues/73


--
Regards,
Sergey.

[Jan Mercl]

On Sun, May 26, 2019 at 5:30 PM Sergey Kamardin <gob...@gmail.com> wrote:

I'm not sure what the goal is, but the code looks like doing the same
as just `bts := b[:]`. But then it's equal to just `bts :=
make([]byte, 16)`. Remember, there si no stack nor heap wrt the
language specification, so there is no guaranteed way to have
something allocated in the stack. If the compiler decides to put it
(in reality) in the heap, you cannot change that decision.

[peterGo]

Sergey Kamardin,

Your code is invalid. A uintptr variable is an integer, not a pointer.

type SliceHeader:  https://golang.org/pkg/reflect/#SliceHeader

SliceHeader is the runtime representation of a slice. It cannot be used safely or portably and its representation may change in a later release. Moreover, the Data field is not sufficient to guarantee the data it references will not be garbage collected, so programs must keep a separate, correctly typed pointer to the underlying data.

type SliceHeader struct {
        Data uintptr
        Len  int
        Cap  int
}

The Go Programming Language Specification:  https://golang.org/ref/spec

uintptr  an unsigned integer large enough to store the uninterpreted bits of a pointer value

Peter

[Sergey Kamardin]

Hi Peter,

Thank you for your answer.

Actually it is not so – please see the rule (6) of the unsafe package
documentation:

https://golang.org/pkg/unsafe/

--
Regards,
Sergey.

On Sun, 05/26/19, May 26, 2019 at 09:43:13AM -0700, peterGo wrote:
> Sergey Kamardin,
>
> Your code is invalid. A uintptr variable is an integer, not a pointer.
>
> type SliceHeader: https://golang.org/pkg/reflect/#SliceHeader
>
> SliceHeader is the runtime representation of a slice. It cannot be used
> safely or portably and its representation may change in a later release.
> Moreover, the Data field is not sufficient to guarantee the data it
> references will not be garbage collected, so programs must keep a separate,
> correctly typed pointer to the underlying data.
>
> type SliceHeader struct {
> Data uintptr
> Len int
> Cap int
> }
>
> The Go Programming Language Specification: https://golang.org/ref/spec
>
> uintptr an unsigned integer large enough to store the uninterpreted bits
> of a pointer value
>
> Peter
>
> On Sunday, May 26, 2019 at 11:30:22 AM UTC-4, Sergey Kamardin wrote:
> >
> > Hello gophers,
> >
> > I have a question which relates mostly to the ideology of unsafe usage.
> >

> > In `github.com/gobwas/ws` <http://github.com/gobwas/ws> WebSocket library

> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/a7a2ad2d-a14d-4538-a90e-cf5a4363d467%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Ian Lance Taylor]

On Sun, May 26, 2019 at 11:30 AM Sergey Kamardin <gob...@gmail.com> wrote:
>
> func ReadHeader(r io.Reader) (Header, error) {
> var (
> b [16]byte
> bts []byte
> )
> h := (*reflect.SliceHeader)(unsafe.Pointer(&bts))
> *h = reflect.SliceHeader{
> Data: uintptr(unsafe.Pointer(&b)),
> Len: len(b),
> Cap: len(b),
> }

This is not valid. The rule is that SliceHeader is only valid when
inspecting an actual slice header. You have to write

h.Data = uintptr(unsafe.Pointer(&b))
h.Len = len(b)
h.Cap = len(b)

I assume this is reduced greatly from the real code, as you could also
just write

bts = b[:]

Ian

[Sergey Kamardin]

Hello Ian,

Thank you for your answer.

On Sun, 05/26/19, May 26, 2019 at 07:59:07PM -0400, Ian Lance Taylor wrote:
> This is not valid. The rule is that SliceHeader is only valid when
> inspecting an actual slice header. You have to write
>
> h.Data = uintptr(unsafe.Pointer(&b))
> h.Len = len(b)
> h.Cap = len(b)
>
> I assume this is reduced greatly from the real code, as you could also
> just write
>
> bts = b[:]

Does this mean that it is not possible to get rid of heap allocation for
passing slice of bytes to some method interface like io.Reader.Read()?

Both code examples above are leading b to escape to the heap when
passing to the interface method.

--
Regards,
Sergey.

[roger peppe]

On Mon, 27 May 2019, 07:18 Sergey Kamardin, <gob...@gmail.com> wrote:

Hello Ian,

Thank you for your answer.

On Sun, 05/26/19, May 26, 2019 at 07:59:07PM -0400, Ian Lance Taylor wrote:
> This is not valid.  The rule is that SliceHeader is only valid when
> inspecting an actual slice header.  You have to write
>
>     h.Data = uintptr(unsafe.Pointer(&b))
>     h.Len = len(b)
>     h.Cap = len(b)
>
> I assume this is reduced greatly from the real code, as you could also
> just write
>
>     bts = b[:]

Does this mean that it is not possible to get rid of heap allocation for
passing slice of bytes to some method interface like io.Reader.Read()?

In general heap allocation is needed for this case because r is an interface and there is no guarantee that it won't store the slice argument somewhere and then return.

If you were to use a static type, the compiler might be able to determine that b doesn't escape and allocate it on the stack.

You might find sync.Pool helps.

Both code examples above are leading b to escape to the heap when
passing to the interface method.

--
Regards,
Sergey.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/20190527061838.GA8345%40MacBook-Pro-Sergej.local.

[Ian Lance Taylor]

Yes, with the current implementation, as far as I know, there is no
way to avoid the slice escaping to the heap when calling Read through
an interface value. With the current implementation, any successful
attempt to avoid the slice escaping to the heap can cause the garbage
collector to fail.

Ian