eidheim / Simple-WebSocket-Server

A very simple, fast, multithreaded, platform independent WebSocket (WS) and WebSocket Secure (WSS) server and client library implemented using C++11, Boost.Asio and OpenSSL. Created to be an easy way to make WebSocket endpoints in C++.
MIT License
800 stars 277 forks source link

Messages split among frames aren't reasembled #90

Closed moonshadow565 closed 6 years ago

moonshadow565 commented 6 years ago

While receiving message that is split among multiple frames, websocket client threats them as individual messages. Other websocket libraries reasamble the messages correctly so i know it isn't something wrong with the server binary i'm connecting to. Possible relavant place in code: https://github.com/eidheim/Simple-WebSocket-Server/blob/master/client_ws.hpp#L565 It doesn't check either if FIN bit or 0x00 opcode(continue message?) It would be nice if the library reassembled messages or at least provided us a feedback that it isn't a complete message at least, since AFAIK there can't be other messages incoming until the first one arrives.

Example sent from server:

{
  "Key": "data"
}

Example received in Simple-Websocket-Client: message 1:

{
  "K

message 2:

ey":"data"

message 3:

}

EDIT: This has no relation with message size, the server binary that runs websocket server just likes fragmented messages...

eidheim commented 6 years ago

Yes, this is a missing feature. Until now I have never heard of anyone using fragmented messages, but I'll implement it soon when I have time to spare.

moonshadow565 commented 6 years ago

To me it looks more like a part of how websockets work than a feature. Sry if sounded rude :) but it might explain why other wierd bugs ocur... I also just noticed that f_rsv_opcode is exposed when you handle message so i will try manualy concating messages for time being. Also you should look into struct bitfields they look like they would represent that value better than manual byteshifting.

Ty for your time

eidheim commented 6 years ago

I added client support for fragmented messages in the above commit, although untested as of now. Will do the same for the server when I have confirmation that it works. Please let me know if the changes work on your side.

moonshadow565 commented 6 years ago

Looking at your code for some time now. You don't seem to check for 0x1, 0x02 and 0x00 opcodes explicitly :) Only 0x01 and 0x02 support fragmentation and 0x00 opcode should be used to check if frame is continuation not a nullptr. This is example from mozillas website:

Client: FIN=1, opcode=0x1, msg="hello"
Server: (process complete message immediately) Hi.
Client: FIN=0, opcode=0x1, msg="and a"
Server: (listening, new message containing text started)
Client: FIN=0, opcode=0x0, msg="happy new"
Server: (listening, payload concatenated to previous message)
Client: FIN=1, opcode=0x0, msg="year!"
Server: (process complete message) Happy new year to you too!

https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers

It "works" for now but i suspect there might be another issue related to this going on(possibly how you shift bits?) . I know this because as soon as i set "Sec-Websocket-Protocl" header to "wamp" the server switches to a really small buffer or something and starts to send a lot of small fragmented messages (again?). AFAIK wamp only specs what websocket messages should contain and nothing else(no extra bits or opcodes?). So i switched it to check for opcodes and parse messages and i get a lot of unkown opcodes, trying to find what causes it now...

EDIT: Gave up, have no idea what causes it :) I did found out that it never recieves first message btw.

eidheim commented 6 years ago

I'm correct in assuming that the above commit works, and the fragmented messages are correctly resulting in a combined message? That is without the wamp extension?

moonshadow565 commented 6 years ago

Yeah it works but its not following websocket specification so i'm afraid it will stop working sometimes and cause someone a headache :) What i'm refering to is that opcode 0x0 is used to indicate continued message and not the fin bit. Fin bit only marks start/end of fragmented message. Its weird because other websockets implementations (beast, uWS) handle it just fine. AFAIK wamp isn't extension its secondary protocol so it should work with wamp protocol specified too. There is really no reason not to... I think there might be another issue because i noticed that i don't recieve the first message immediately as it should be the case. I only recieve it after i start sending messages from client?

eidheim commented 6 years ago

Great, thank you for testing it.

If the FIN bit is 0 it means that this message is separated into several fragments. See https://tools.ietf.org/html/rfc6455#section-5.2

After a quick look into wamp, it seems like you need to subscribe to message topics on the server? I'll see if I can find a low-lever explanation of this.

You should receive messages to the client without first sending a message from the client.

eidheim commented 6 years ago

Or rather, FIN bit of 0 means that this is part of a fragmented message and this is not the final fragment.

moonshadow565 commented 6 years ago

I know i need to subscribe :) but i first need to receive welcome message (which doesn't happen when its supposed to). The welcome message doesn't arrive until i send at least one message which brakes wamp protocol. FIN bit is set to 0 to all messages broken into fragments except the last one but there is also opcode 0. Example:

fragment 0: FIN: 0 OP: 1 
fragment 1: FIN: 0 OP: 0
fragment 2: FIN: 0 OP: 0
fragment 3: FIN: 1 OP: 0

Fragmentation can not occur on control frames so you must check for opcode being less than 8 and you use else which handles all opcodes. I don't think this is according to spec :)

      Defines the interpretation of the "Payload data".  If an unknown
      opcode is received, the receiving endpoint MUST _Fail the
      WebSocket Connection_.  The following values are defined.

      *  %x0 denotes a continuation frame

      *  %x1 denotes a text frame

      *  %x2 denotes a binary frame

      *  %x3-7 are reserved for further non-control frames

      *  %x8 denotes a connection close

      *  %x9 denotes a ping

      *  %xA denotes a pong

You don't fail connection on unknown opcode

moonshadow565 commented 6 years ago

This is what i get when connecting with wamp :) zshwtv If i ignore the fact i don't recieve wellcome message and just subscribe to endpoint The welcome message comes then but its bit wierd... 0x000D and 0x000A coresponds to \r\n

eidheim commented 6 years ago

What is the fin_rsv_opcode of the welcome message when using wamp?

eidheim commented 6 years ago

And I agree that my websocket implementation could be improved with better error handling for instance, but the main focus has been to implement a websocket client/server on top of Asio supporting the most common use cases of the websocket protocol. Fragmented messages should of course be included here as is apparent from your use case.

moonshadow565 commented 6 years ago

Now even after i send message before welcome message i need to wait for ~20sec ... This line here is reseting the pointer to fragmented message: https://github.com/eidheim/Simple-WebSocket-Server/blob/fragmented_message/client_ws.hpp#L600 image Maybe your locks are wrong somewhere probably? Also idk if this matters but first message could be broadcasted together with handshake response(is that handled?). Reading asio code has been much pain for me :)

I suggset that you simply change your code like this:

else if(opcode == 0) {
  if(fin) {
  //finish writing fragmented message and broadcast to client
  } else {
  //continue writing fragmented messae
  }
} else if(opcode < 8) { //or just else?
 if(fin) {
  //broadcast whole message
 } else {
  //start writing fragmented message
 }
}
moonshadow565 commented 6 years ago

I'll try on master branch and see what i get there :) EDIT: Hex dump on master: https://hastebin.com/uhogoqevos.go

moonshadow565 commented 6 years ago

EDITED: Captured the trafic on another websocket and fiddler. The first message length is 0x42 just as it says but your client implementation captures it wrongly :/

moonshadow565 commented 6 years ago

You aren't locking the stream properly somewhere and multiple messages get written in there :D for async_read

This operation is implemented in terms of zero or more calls to the stream's async_read_some function, and is known as a composed operation. The program must ensure that the stream performs no other read operations (such as async_read, the stream's async_read_some function, or any other composed operations that perform reads) until this operation completes.

http://www.boost.org/doc/libs/1_61_0/doc/html/boost_asio/reference/async_read/overload4.html EDIT: It might be worth noteing that first message is sent together with HTTP upgrade response

eidheim commented 6 years ago

I did a quick test on the fragmented messages, and it works on my side.

Aha, the message is HTTP request content and is sent together with the HTTP upgrade response? That explains it since request content is not expected, and thus not read, in the handshake. Could you send me the HTTP handshake request with content?

eidheim commented 6 years ago

I mean HTTP response above, not request (your client is doing the HTTP request)

moonshadow565 commented 6 years ago
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Accept: tE2J/81FnOOkezlH7tVQjZtz1WQ=
Sec-WebSocket-Protocol: wamp

B[
    0,
    "8b396dae",
    1,
    "WAMP-1.0-RiotRemoting"
]

In case you want to check yourself the websocket is hosted by launcher of game called League of Legends

eidheim commented 6 years ago

That is a websocket message, and not response content. So no solution so far.

eidheim commented 6 years ago

I think I know the issue. asio::async_read_until will sometimes read more bytes than necessary and put these in the buffer. One would have to check if such additional_bytes are read before performing asio::async_read after. This issue arises because the server is sending a websocket message right after its handshake response.I'll fix it Tomorrow.

moonshadow565 commented 6 years ago

The bytes transfered size is equal to message size, so i doubt it :) So i said it might be that stream isn't locked for reading and another message gets read into the stream in the meantime ? At least thats how it looks to me... Did you consider using synchronous reads and do the dispatch part asynch? EDIT: The documentation says that async_read will use multiple read_some, so i'm not so sure that it will put it more than needed.

eidheim commented 6 years ago

~The issue you have with wamp should be fixed in the above commit~

To me it seems that this is the server's fault. It should wait for the handshake to finish, that is the client should have received the handshake, before sending a websocket message.

eidheim commented 6 years ago

There was an error in the previous commit, the last commit above should work.

moonshadow565 commented 6 years ago

Works with wamp now tnx! I'm pretty sure that server can send message "embeded" in first http response :) Its not servers fault, i tried it with other websockets libraries such as uWS, beast and websocketsharp.