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: Receive returns ErrNotImplemented for unsolicited pong frames #6377

Closed
gopherbot opened this issue Sep 12, 2013 · 16 comments
Closed

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 12, 2013

by aicommander:

What steps will reproduce the problem?
0. Open a WebSocket which will be connected to Internet Explorer 11
1. Call WebSocket.Receive() without providing any data to receive
2. Wait 30 seconds or so for the Receive() call to fail

What is the expected output?
WebSocket.Receive() should continue to wait after receiving a pong.

What do you see instead?
WebSocket.Receive() returns ErrNotImplemented when it gets an unsolicited pong from IE

Which operating system are you using?
Windows 8.1 with IE 11

Which version are you using?  (run 'go version')
Go 1.1.2 Windows/AMD64

Please provide any additional information below.

This behavior does not adhere to RFC 6455 Section 5.5.3 which explicitly mentions that
browsers are allowed to send unsolicited pongs.

"A Pong frame MAY be sent unsolicited.  This serves as a unidirectional heartbeat. 
A response to an unsolicited Pong frame is not expected."
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 12, 2013

Comment 1 by aicommander:

Here is a potential patch for this issue. I've changed the code handling PongFrame in
Hybi's HandleFrame() function to return nil data and nil error (similar to how the
PingFrame is handled). Returning nil nil here makes WebSocket.Receive() wait for another
frame which is the intended behavior.
diff -r bc411e2ac33f websocket/hybi.go
--- a/websocket/hybi.go Thu Aug 15 13:52:04 2013 +1000
+++ b/websocket/hybi.go Thu Sep 12 18:20:12 2013 -0400
@@ -301,7 +301,7 @@
        }
        return nil, nil
    case PongFrame:
-       return nil, ErrNotImplemented
+       return nil, nil
    }
    return frame, nil
 }
@rsc
Copy link
Contributor

@rsc rsc commented Oct 18, 2013

Comment 2:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 3:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 4:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 5:

Labels changed: added repo-net.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 25, 2014

Comment 6 by ledangster:

I noticed that while it is returning ErrNotImplemented, can it be further made useful by
discarding the entire frame itself, similar to what the PingFrame handler does? The
connection becomes useless after receiving a Pong frame because of the frame not being
fully junked away.
I did a test by sending a PingFrame from the server and just waited until a Pong was
received from the browser:
  conn.PayloadTyhpe = websocket.PingFrame
  _, err = conn.Write([]byte("ping")
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 26, 2014

Comment 7 by ledangster:

aicomman...@gmail.com ... returning nil, nil will not be sufficient as there's seems to
remain the frame payload which hasn't been skipped, so per my previous message, the
PongFrame needs to be discarded the similar to PingFrame:
diff -r 93f5aef8cb25 websocket/hybi.go
--- a/websocket/hybi.go Wed Jan 29 10:23:52 2014 +0900
+++ b/websocket/hybi.go Wed Feb 26 10:00:05 2014 -0800
@@ -288,20 +288,21 @@
        handler.payloadType = frame.PayloadType()
    case CloseFrame:
        return nil, io.EOF
-   case PingFrame:
+   case PingFrame, PongFrame:
        pingMsg := make([]byte, maxControlFramePayloadLength)
        n, err := io.ReadFull(frame, pingMsg)
        if err != nil && err != io.ErrUnexpectedEOF {
            return nil, err
        }
        io.Copy(ioutil.Discard, frame)
+       if frame.PayloadType() == PongFrame {
+           return nil, ErrNotImplemented
+       }
        n, err = handler.WritePong(pingMsg[:n])
        if err != nil {
            return nil, err
        }
        return nil, nil
-   case PongFrame:
-       return nil, ErrNotImplemented
    }
    return frame, nil
 }
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 4, 2014

Comment 8 by matusis:

ledangs...@gmail.com : your patch seems to return EOF err for an unexpected Pong frame,
which an application cannot distinguish from a normal client disconnect. I think
PongFrame needs to be handled separately from PingFrame:
--- hybi.go     2014-09-04 11:03:07.000000000 -0700
+++ hybi.go.patched     2014-09-04 11:02:58.000000000 -0700
@@ -301,7 +301,14 @@
                }
                return nil, nil
        case PongFrame:
-               return nil, ErrNotImplemented
+               //discard unexpected Pong frame
+               pongMsg := make([]byte, maxControlFramePayloadLength)
+               _, err := io.ReadFull(frame, pongMsg)
+               if err != nil && err != io.EOF {
+                       return nil, err
+               }
+               io.Copy(ioutil.Discard, frame)
+               return nil,  nil
        }
        return frame, nil
 }
It would be nice if the code was actually fixed in the repo, since it causes real world
disconnects with IE11.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 4, 2014

Comment 9 by ledangster:

matu...@gmail.com: that's a weird misbehavior of io.EOF (golang spec says/recommends the
use of EOF to indicate an end of stream - due to connection being disconnected). If
io.ReadFull() is getting an EOF, then there's a bug somewhere in the hybi frame Read
handler.
btw, have you tested this against all the other browsers? I don't have ie11 on hand to
play with.
Like you, I don't have control of the repo.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 4, 2014

Comment 10 by matusis:

ledangs...@gmail.com : I tested in Firefox, IE11, Chrome and Safari on iOS, and it does
not disconnect.
@mikioh mikioh changed the title go.net/websocket: Receive returns ErrNotImplemented for unsolicited pong frames x/net/websocket: Receive returns ErrNotImplemented for unsolicited pong frames Dec 23, 2014
@mikioh mikioh added repo-net and removed repo-net labels Dec 23, 2014
@mikioh mikioh changed the title x/net/websocket: Receive returns ErrNotImplemented for unsolicited pong frames websocket: Receive returns ErrNotImplemented for unsolicited pong frames Jan 4, 2015
@mattbaird
Copy link

@mattbaird mattbaird commented Mar 20, 2015

i have tested this patch with IE 11, Chrome, Safari and firefox and it fixes the issue (websockets unusable on ie, and does not effect other browsers). Any chance this gets integrated soon? It's very hard/impossible to patch this through forking when releasing a product.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title websocket: Receive returns ErrNotImplemented for unsolicited pong frames x/net/websocket: Receive returns ErrNotImplemented for unsolicited pong frames Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-net label Apr 14, 2015
@fredlebel
Copy link

@fredlebel fredlebel commented Jul 16, 2015

This is also an issue with the WebSocket implementation in WinJS. Is there any chance we can get an estimate on when this issue will be resolved?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 16, 2015

It looks like people have an idea as to what to fix. Can somebody send in a CL?

@mikioh mikioh added the help wanted label Jul 30, 2015
@fredlebel
Copy link

@fredlebel fredlebel commented Jul 31, 2015

We're going to create a PR for this. We'll take the patch from comment 8 and test it with the AppRTC server and WinJS which is where we're having issues.
Is there anything else we should know before starting this effort?
Thanks.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 31, 2015

If by PR you mean pull request, please be aware that Go does not accept Github pull requests. In to contribute, please see https://golang.org/doc/contribute.html . Thanks.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Aug 3, 2015

CL https://golang.org/cl/13054 mentions this issue.

@golang golang locked and limited conversation to collaborators Aug 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.