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

x/net/websocket: fails to Receive a single frame sent in chunks from Browser #16945

Open
mattbaird opened this issue Sep 1, 2016 · 6 comments

Comments

@mattbaird
Copy link

commented Sep 1, 2016

Please answer these questions before submitting your issue. Thanks!

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

1.5 to 1.7

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/matthew/code/go:/home/matthew/code/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build629490277=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

This is a very difficult issue to reproduce, as the browsers themselves don't always exhibit the problem.
Reproducing in our environment means sending a large >120k websocket message from the Browser (tested on Chrome) to the Go program from a "clean" browser - meaning either all caches, cookies cleared, or a new Incognito mode browser.
When sent to the server, the browser will chunk the frame into multiple parts, setting the hybiFrameReader.header.Fin bit to false

What did you expect to see?

I expected the whole frame to arrive.

What did you see instead?

arbitrary cut off of string frame.

My fix was to change the Receive function to check if the frame Fin was false, and if so, goto Again:

// Receive receives single frame from ws, unmarshaled by cd.Unmarshal and stores in v.
func (cd Codec) Receive(ws *Conn, v interface{}) (err error) {
    ws.rio.Lock()
    defer ws.rio.Unlock()
    if ws.frameReader != nil {
        _, err = io.Copy(ioutil.Discard, ws.frameReader)
        if err != nil {
            return err
        }
        ws.frameReader = nil
    }
    var b bytes.Buffer
again:
    frame, err := ws.frameReaderFactory.NewFrameReader()
    if err != nil {
        return err
    }
    frame, err = ws.frameHandler.HandleFrame(frame)
    if err != nil {
        return err
    }
    if frame == nil {
        goto again
    }
    payloadType := frame.PayloadType()
    data, err := ioutil.ReadAll(frame)
    if err != nil {
        return err
    }
    _, err = b.Write(data)
    if err != nil {
        return err
    }
    if !frame.(*hybiFrameReader).header.Fin {
        goto again
    }
    return cd.Unmarshal(b.Bytes(), payloadType, v)
}

I have a feeling Read(...) also suffers from this problem.

@quentinmit quentinmit changed the title net/websocket fails to Receive a single frame sent in chunks from Browser x/net/websocket: fails to Receive a single frame sent in chunks from Browser Sep 6, 2016

@quentinmit quentinmit added this to the Unreleased milestone Sep 6, 2016

@mattbaird

This comment has been minimized.

Copy link
Author

commented Sep 8, 2016

hi @quentinmit is x/net/ being migrated to just net? I thought I saw something about that, but cannot find it now.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

@mattbaird, no. Parts of x/net are privately vendored into the standard library when needed, but nothing's moving.

@mattbaird

This comment has been minimized.

Copy link
Author

commented Sep 8, 2016

@bradfitz thank you, I was a little bit worried there was another bigger refactoring happening.

@mattbaird

This comment has been minimized.

Copy link
Author

commented Feb 16, 2017

hi @quentinmit how can I help move this issue along?

@mattbaird

This comment has been minimized.

Copy link
Author

commented Aug 4, 2017

is this version of websockets being abandoned? I see references to Gorilla websockets package in the code comments - are users encouraged to migrate away from this package to the Gorilla version?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

It's not explicitly abandoned, but nobody works on it either, and its design is basically wrong. It's kept for people who still depend on it but, yes, you should probably use Gorilla.

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