fix: panic caused by uintptr #60
Closed
+14
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change-Id: Iddd9c07d7c1c1d3f1cf5239b6524e9399b49725d
|
FYI: the panic logs 2018/10/29 00:44:20 connection.ping: uid= 0YP62KHlVrCntuDG3T9T runtime stack: |
|
@jxskiss hi. Could you please provide more info on how pointers are used incorrectly? The lot of changes you provide simply bring in performance degradation and thats all. |
|
Also could you paste more stack trace? How can I reproduce this? |
Hi, nice to talk about this. First thing, from
thus, the uintptr variable usage is considered invalid, and is problematic to cause panic. Another thing, in the PR, I keeped the bytes allocated as fixed length array, which I think will As a reference, there's an post explaining Golang's memory allocation: |
The massive stack trace logs does not provide much helpful info, so I did not post them. Regard of reproducing the issue, I will try to make an minimal example :-) |
|
Hi @jxskiss. First, few sentences above your cite:
I agree, in general it is not valid, but how the stack may be moved while running
It does allocate new memory. When you passing stack-allocated array via Currently I may agree only with Again, we need to reproduce this to get an ideas of how to solve the problem. |
|
Well, I failed to construct a minimal example reproduce the problem. And for some reason, I can't publish the code which can easily trigger the issue (it's a lot of code running many goroutines). But I have found an old thread which may explain the issue: https://groups.google.com/d/msg/golang-nuts/W_Up0e4TOfs/bt9UjGCXyk4J The problem was observed in And after removing the uintptr usage within read.go my program works just as expected. Update: consider the memory allocation, you are right, pass a slice to another function will make the slice escapes to heap, I confirmed this using |
|
Thank you for the link. As an experiment, could you put |
Hi, @gobwas , I have tried nosplit, the zero-value header and panic is still there (the zero-value header happens more often than panic). The stacktrace is basically same with previous post: |
|
The program is briefly like following (it did not reproduce the problem): package main
import (
"github.com/gobwas/ws"
"github.com/gobwas/ws/wsutil"
"io"
"log"
"net"
"sync"
"sync/atomic"
"time"
)
var (
counter int64
pingCh chan *connWrapper
mu sync.Mutex
hub map[int64]*connWrapper
)
func init() {
pingCh = make(chan *connWrapper, 100)
hub = make(map[int64]*connWrapper)
for i := 0; i < 10; i++ {
go ping()
}
go func() {
ticker := time.NewTicker(30 * time.Second)
for range ticker.C {
mu.Lock()
for _, cc := range hub {
pingCh <- cc
}
mu.Unlock()
}
}()
}
func main() {
// run some dummy goroutines
for i := 0; i < 100; i++ {
go func() {
for {
time.Sleep(time.Second)
}
}()
}
svr := NewWebSocketServer()
log.Fatalln(svr.ListenAndServe())
}
type WebsocketServer struct {
ln net.Listener
}
func NewWebSocketServer() *WebsocketServer {
return &WebsocketServer{}
}
func (s *WebsocketServer) ListenAndServe() error {
ln, err := net.Listen("tcp", "127.0.0.1:8888")
if err != nil {
log.Fatalln("error listen socket:", err)
}
s.ln = ln
for {
conn, err := s.ln.Accept()
if err != nil {
return err
}
go handle(conn)
}
}
func ping() {
for cc := range pingCh {
log.Println("ping: id=", cc.id)
cc.io.Lock()
_, err := cc.Write(ws.CompiledPing)
cc.io.Unlock()
if err != nil {
log.Println("ping error:", err)
cc.Close()
}
}
}
func handle(conn net.Conn) {
cc := &connWrapper{
id: atomic.AddInt64(&counter, 1),
Conn: conn,
ctl: wsutil.ControlHandler{
Src: conn,
Dst: conn,
State: ws.StateServerSide,
},
}
_, err := ws.Upgrade(cc)
if err != nil {
log.Println("upgrade error:", err)
cc.Conn.Close()
return
}
mu.Lock()
hub[cc.id] = cc
mu.Unlock()
for {
err := cc.Receive()
if err != nil {
log.Println("receive error:", err)
cc.Close()
return
}
}
}
type connWrapper struct {
id int64
io sync.Mutex
net.Conn
ctl wsutil.ControlHandler
}
func (c *connWrapper) Receive() error {
header, err := ws.ReadHeader(c)
log.Println("receive: header=", header)
if err != nil {
return err
}
if err := ws.CheckHeader(header, ws.StateServerSide); err != nil {
return err
}
c.io.Lock()
defer c.io.Unlock()
if header.OpCode.IsControl() {
return c.ctl.Handle(header)
}
payload := make([]byte, header.Length)
if _, err := io.ReadFull(c, payload); err != nil {
return err
}
resp := ws.NewFrame(header.OpCode, true, payload)
if err := ws.WriteFrame(c, resp); err != nil {
return err
}
return nil
}
func (c *connWrapper) Close() error {
mu.Lock()
delete(hub, c.id)
mu.Unlock()
return c.Conn.Close()
} |
|
Hi @jxskiss. Am I right, that the failed experiment with Do you really check errors returned by |
|
By the way, for the clear experiment, |
Yes, I do checked errors from ReadHeader().
Have tried this but no lucky. And this time it fails after about 5 minutes, the complete log size is small, so I will post them below, hope that helps. diff --git a/cipher.go b/cipher.go
index 6620254..f50bfcb 100644
--- a/cipher.go
+++ b/cipher.go
@@ -2,6 +2,8 @@ package ws
import "unsafe"
+//go:nosplit
+
// Cipher applies XOR cipher to the payload using mask.
// Offset is used to cipher chunked data (e.g. in io.Reader implementations).
//
diff --git a/nonce.go b/nonce.go
index 114e0cb..00b1b92 100644
--- a/nonce.go
+++ b/nonce.go
@@ -38,9 +38,10 @@ var sha1Pool sync.Pool
type nonce [nonceSize]byte
func (n *nonce) bytes() []byte {
- h := uintptr(unsafe.Pointer(n))
- b := &reflect.SliceHeader{Data: h, Len: nonceSize, Cap: nonceSize}
- return *(*[]byte)(unsafe.Pointer(b))
+ //h := uintptr(unsafe.Pointer(n))
+ //b := &reflect.SliceHeader{Data: h, Len: nonceSize, Cap: nonceSize}
+ //return *(*[]byte)(unsafe.Pointer(b))
+ return n[:]
}
func acquireSha1() hash.Hash {
diff --git a/read.go b/read.go
index 2ea33fe..c96c3bb 100644
--- a/read.go
+++ b/read.go
@@ -14,6 +14,8 @@ var (
ErrHeaderLengthUnexpected = fmt.Errorf("header error: unexpected payload length bits")
)
+//go:nosplit
+
// ReadHeader reads a frame header from r.
func ReadHeader(r io.Reader) (h Header, err error) {
// Make slice of bytes with capacity 12 that could hold any header.
diff --git a/util.go b/util.go
index f86cd24..5075149 100644
--- a/util.go
+++ b/util.go
@@ -198,6 +198,8 @@ func readLine(br *bufio.Reader) ([]byte, error) {
}
}
+//go:nosplit
+
// strEqualFold checks s to be case insensitive equal to p.
// Note that p must be only ascii letters. That is, every byte in p belongs to
// range ['a','z'] or ['A','Z'].
@@ -236,6 +238,8 @@ func strEqualFold(s, p string) bool {
return true
}
+//go:nosplit
+
// btsEqualFold checks s to be case insensitive equal to p.
// Note that p must be only ascii letters. That is, every byte in p belongs to
// range ['a','z'] or ['A','Z'].
diff --git a/write.go b/write.go
index 83142a5..950c2dd 100644
--- a/write.go
+++ b/write.go
@@ -47,6 +47,8 @@ func HeaderSize(h Header) (n int) {
return n
}
+//go:nosplit
+
// WriteHeader writes header binary representation into w.
func WriteHeader(w io.Writer, h Header) error {
// Make slice of bytes with capacity 14 that could hold any header.My environment: Microsoft Windows [Version 10.0.17134.345] |
jxskiss
changed the title
|
Hi @jxskiss, Long time ago you have opened this issue, and today I have pushed changes that may potentially fix your problem too. Are you still able to try out new Also, travis.ci now allows to run tests against windows servers, so we are able now to write some close to your use case test and run it during CI. Cheers. |
gobwas
added a commit
that referenced
this pull request
Jun 1, 2019
This commit removes stack-based slice usage from from ReadHeader(), WriteHeader() and nonce type manipulation. That stack-based hacks were working well on Linux but were crashing applications on Windows with errors indicating that GC found pointer not to the allocated span. For sure, allocation on heap will bring some penalty to the performance, but since Go uses TCMalloc under the hood that penalties could be not significant. Fixes #73 #63 #60
gobwas
added a commit
that referenced
this pull request
Jun 1, 2019
This commit removes stack-based slice usage from from ReadHeader(), WriteHeader() and nonce type manipulation. That stack-based hacks were working well on Linux but were crashing applications on Windows with errors indicating that GC found pointer not to the allocated span. For sure, allocation on heap will bring some penalty to the performance, but since Go uses TCMalloc under the hood that penalties could be not significant. Fixes #73 #63 #60
|
@gobwas Sorry for the late reply, I didn't noticed the updates. I will give it a try later soon. Thx! |
@gobwas I have tried the latest master, it did not panic, but it reports error ""unexpected continuation data frame" for concurrent connections test, so I guess there is still some problems. FYI, the program runs as expected using the changes I proposed in this PR (use slice instead of uintptr). |
|
Could you please try it against appropriate pull request branch, not against master? It is not yet merged. Thanks! |
|
@gobwas Sorry for the mistake -_- |
|
@gobwas Thank you all |
gobwas
added a commit
that referenced
this pull request
Jun 4, 2019
This commit removes stack-based slice usage from from ReadHeader(), WriteHeader() and nonce type manipulation. That stack-based hacks were working well on Linux but were crashing applications on Windows with errors indicating that GC found pointer not to the allocated span. For sure, allocation on heap will bring some penalty to the performance, but since Go uses TCMalloc under the hood that penalties could be not significant. Fixes #73 #63 #60
gobwas
added a commit
that referenced
this pull request
Jun 4, 2019
This commit removes stack-based slice usage from from ReadHeader(), WriteHeader() and nonce type manipulation. That stack-based hacks were working well on Linux but were crashing applications on Windows with errors indicating that GC found pointer not to the allocated span. For sure, allocation on heap will bring some penalty to the performance, but since Go uses TCMalloc under the hood that penalties could be not significant. Fixes #73 #63 #60
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When I used this library on windows, ReadHeader returns full zero Header after about 3-5 calls, and sometimes the program panic. After some debug I found it's caused by the usage of uintptr.
According to doc of
unsafe.Pointer, most of the previous uintptr usage are incorrect. This PR fixes the issue.Change-Id: Iddd9c07d7c1c1d3f1cf5239b6524e9399b49725d