GNOME Bugzilla – Bug 792113
Improvements on websocket implementation
Last modified: 2018-02-01 09:02:18 UTC
Following up I will attach some patches that will improve the websocket implementation fixing some of the autobahn unit tests.
Created attachment 366161 [details] [review] Fix big control message payload Close connection with protocol error if the payload of a control message is bigger than 125 octets. Fix Authobahn test case 2.5.
Created attachment 366163 [details] [review] Fix reserved bit Close connection if reserved bits are different than zero. These bits should always be 000 because libsoup does not support extentions that need to use these reserved bits. Fix Autobahn test cases 3.*
Created attachment 366164 [details] [review] Fix reserved opcode Some opcode are reserved for future implementations of the WebSocket protocol. If these reserved code are send, the connection needs to be closed. Fix Autobahn test cases 4.*.
Created attachment 366165 [details] [review] Fix payload close When the client send two consecutive closing frames, send back only one closing frame. Fix Autobahn test case 7.1.2.
Comment on attachment 366165 [details] [review] Fix payload close From 79f8329d140d054de5555dde49ec6b0802b67885 Mon Sep 17 00:00:00 2001 From: Italo Guerrieri <guerital@amazon.it> Date: Thu, 28 Dec 2017 12:12:20 +0100 Subject: [PATCH 4/8] Fix payload close Pass the close payload to the function send_message in order to check if the payload if bigger than 125 octect Fix Autobahn test case 7.3.6. --- libsoup/soup-websocket-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsoup/soup-websocket-connection.c b/libsoup/soup-websocket-connection.c index f3812563..682a69f5 100644 --- a/libsoup/soup-websocket-connection.c +++ b/libsoup/soup-websocket-connection.c @@ -623,7 +623,7 @@ receive_close (SoupWebsocketConnection *self, if (pv->connection_type == SOUP_WEBSOCKET_CONNECTION_SERVER) close_io_stream (self); } else { - close_connection (self, pv->peer_close_code, NULL); + close_connection (self, pv->peer_close_code, pv->peer_close_data); } } -- 2.15.0.windows.1
Created attachment 366166 [details] [review] Fix close problem Pass the close payload to the function send_message in order to check if the payload if bigger than 125 octet Fix Autobahn test case 7.3.6.
Created attachment 366167 [details] [review] Fix invalid UTF8 close payload Close the connection with a protocol error if the close payload is an invalid utf8. Fix Autobahn test case 7.5.1.
Created attachment 366168 [details] [review] Fix invalid close code Close the connection with a protocol error, if a close control uses a code reserved for future implementations of the WebSocket protocol. Fix Autobahn test cases 7.9.*.
Created attachment 366169 [details] [review] Fix order control frame Ensure that the control frames received are sent back in the order in which they arrived. Fix Autobahn test case 2.10.
Created attachment 366170 [details] [review] Fix UTF8 message Instead of checking if a string is valid frame by frame, it check if it is a valid utf8 only when the message is totaly reassembled. Fix Autobahn test cases: 6.2.3, 6.2.4 and 6.4.2.
Created attachment 366171 [details] [review] Fix payload close From 79f8329d140d054de5555dde49ec6b0802b67885 Mon Sep 17 00:00:00 2001 From: Italo Guerrieri <guerital@amazon.it> Date: Thu, 28 Dec 2017 12:12:20 +0100 Subject: [PATCH 4/8] Fix payload close Pass the close payload to the function send_message in order to check if the payload if bigger than 125 octect Fix Autobahn test case 7.3.6. --- libsoup/soup-websocket-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsoup/soup-websocket-connection.c b/libsoup/soup-websocket-connection.c index f3812563..682a69f5 100644 --- a/libsoup/soup-websocket-connection.c +++ b/libsoup/soup-websocket-connection.c @@ -623,7 +623,7 @@ receive_close (SoupWebsocketConnection *self, if (pv->connection_type == SOUP_WEBSOCKET_CONNECTION_SERVER) close_io_stream (self); } else { - close_connection (self, pv->peer_close_code, NULL); + close_connection (self, pv->peer_close_code, pv->peer_close_data); } } -- 2.15.0.windows.1
Comment on attachment 366161 [details] [review] Fix big control message payload spec says "control frames MUST have a payload length of 125 bytes or less" so this seems right
Comment on attachment 366163 [details] [review] Fix reserved bit Yup. "If a nonzero value is received and none of the negotiated extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocket Connection_."
Comment on attachment 366164 [details] [review] Fix reserved opcode "If an unknown opcode is received, the receiving endpoint MUST _Fail the WebSocket Connection_."
Comment on attachment 366167 [details] [review] Fix invalid UTF8 close payload "When an endpoint is to interpret a byte stream as UTF-8 but finds that the byte stream is not, in fact, a valid UTF-8 stream, that endpoint MUST _Fail the WebSocket Connection_."
Comment on attachment 366168 [details] [review] Fix invalid close code >+ if ((code > 0 && code < 3000) || code > 4999) { >+ g_debug ("Wrong closing code %d received", code); >+ protocol_error_and_close (self); 1) This allows code to be 0, which is not allowed. 2) The spec doesn't say that the code can't be > 4999. It just says those codes aren't "reserved". So I think this should be "if (code < 3000) { ..."?
Comment on attachment 366170 [details] [review] Fix UTF8 message >+ if (fin && !g_utf8_validate ((char *) pv->message_data->data, >+ pv->message_data->len, >+ NULL)) { indentation is wrong here. ("pv->message_data->len" should be lined up with "(char *)") Also, maybe reorganize the whole section? You could just have a single g_byte_array_append() call, and then move the utf8 validation inside the existing "if (fin)" section below? Maybe this would end up being messier than it is now...
Comment on attachment 366169 [details] [review] Fix order control frame hm... ok, well, i guess if this doesn't break our tests and doesn't break your tests, then it's probably right...
Attachment 366161 [details] pushed as 14c3397 - Fix big control message payload Attachment 366163 [details] pushed as 3eadca3 - Fix reserved bit Attachment 366164 [details] pushed as 5dec678 - Fix reserved opcode Attachment 366167 [details] pushed as 03c6006 - Fix invalid UTF8 close payload Attachment 366171 [details] pushed as 910d67c - Fix payload close
Created attachment 366653 [details] [review] Fix invalid close code
Created attachment 366654 [details] [review] Fix UTF8 message
(In reply to Dan Winship from comment #19) > Comment on attachment 366169 [details] [review] [review] > Fix order control frame > > hm... ok, well, i guess if this doesn't break our tests and doesn't break > your tests, then it's probably right... All the tests pass properly: ./websocket-test /websocket/soup/handshake: OK /websocket/soup/send-client-to-server: OK /websocket/soup/send-server-to-client: OK /websocket/soup/send-big-packets: OK /websocket/soup/send-bad-data: OK /websocket/soup/close-clean-client: OK /websocket/soup/close-clean-server: OK /websocket/soup/message-after-closing: OK /websocket/soup/protocol-negotiate: OK /websocket/soup/protocol-mismatch: OK /websocket/soup/protocol-server-any: OK /websocket/soup/protocol-client-any: OK /websocket/soup/client-context: OK /websocket/direct/send-client-to-server: OK /websocket/direct/send-server-to-client: OK /websocket/direct/send-big-packets: OK /websocket/direct/send-bad-data: OK /websocket/direct/close-clean-client: OK /websocket/direct/close-clean-server: OK /websocket/direct/message-after-closing: OK /websocket/direct/protocol-negotiate: OK /websocket/direct/protocol-mismatch: OK /websocket/direct/protocol-server-any: OK /websocket/direct/protocol-client-any: OK /websocket/direct/receive-fragmented: OK
Review of attachment 366169 [details] [review]: The spec doesn't mention anything about strict ordering of the control frames, but I see that autobahn ensures this anyway. It wouldn't hurt to do this, I guess. Just a comment below. ::: libsoup/soup-websocket-connection.c @@ +1043,3 @@ + + while (prev != NULL && (prev->flags & SOUP_WEBSOCKET_QUEUE_URGENT)) + prev = g_queue_peek_nth (&pv->outgoing, ++i); Using g_queue_peek_nth() in a loop is not very efficient, as each call will iterate to the nth position from either the head or tail of the queue. It should be possible to do the same with a GList pointer to the head instead, for example.
Created attachment 367285 [details] [review] Fix order control frame Ensure that the control frames received are sent back in the order in which they arrived. Fix Autobahn test case 2.10.
Review of attachment 367285 [details] [review]: This looks better, thanks. I have a rather minor comment below. ::: libsoup/soup-websocket-connection.c @@ +1066,3 @@ } + + g_queue_push_nth (&pv->outgoing, frame, i); A further micro-optimization is possible here. You already have the pointer to the element where you want to insert the frame. You can get rid of the counter and use g_queue_insert_before() (I believe) to insert it there. That way you can avoid the g_queue_push_nth() call. Sorry for splitting hairs here and thanks for fixing this!
Created attachment 367311 [details] [review] Fix order control frame Ensure that the control frames received are sent back in the order in which they arrived. Fix Autobahn test case 2.10.
(In reply to Claudio Saavedra from comment #26) > Review of attachment 367285 [details] [review] [review]: > > This looks better, thanks. I have a rather minor comment below. > > ::: libsoup/soup-websocket-connection.c > @@ +1066,3 @@ > } > + > + g_queue_push_nth (&pv->outgoing, frame, i); > > A further micro-optimization is possible here. You already have the pointer > to the element where you want to insert the frame. You can get rid of the > counter and use g_queue_insert_before() (I believe) to insert it there. That > way you can avoid the g_queue_push_nth() call. > > Sorry for splitting hairs here and thanks for fixing this! Did not know about that method, it seems new in 2.44. Thanks for pointing it out!
Review of attachment 367311 [details] [review]: Thank you! Assuming all tests are passing (ab 2.10 included), please go ahead and push!
Claudio, can we get an ack on the other 2 patches as well?
Comment on attachment 367311 [details] [review] Fix order control frame Attachment 367311 [details] pushed as d17d5ee - Fix order control frame
I am going ahead and pushing the patches. They were initially reviewed by Dan and Italo made the changes he requested and pbor and I made a review internally of them. If there are any other changes that you would like changed we are more than open to make changes. Attachment 366653 [details] pushed as 0366756 - Fix invalid close code Attachment 366654 [details] pushed as f8adddd - Fix UTF8 message