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
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mattbaird
Copy link

mattbaird 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
Copy link
Author

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

@bradfitz
Copy link
Contributor

bradfitz commented Sep 8, 2016

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

@mattbaird
Copy link
Author

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

@mattbaird
Copy link
Author

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

@mattbaird
Copy link
Author

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
Copy link
Contributor

bradfitz 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.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants