After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792113 - Improvements on websocket implementation
Improvements on websocket implementation
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
2.60.x
Other All
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2018-01-02 09:18 UTC by Italo Guerrieri
Modified: 2018-02-01 09:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix big control message payload (1.41 KB, patch)
2018-01-02 09:20 UTC, Italo Guerrieri
committed Details | Review
Fix reserved bit (1021 bytes, patch)
2018-01-02 09:23 UTC, Italo Guerrieri
committed Details | Review
Fix reserved opcode (1.25 KB, patch)
2018-01-02 09:23 UTC, Italo Guerrieri
committed Details | Review
Fix payload close (1.11 KB, patch)
2018-01-02 09:24 UTC, Italo Guerrieri
none Details | Review
Fix close problem (1.11 KB, patch)
2018-01-02 09:31 UTC, Italo Guerrieri
none Details | Review
Fix invalid UTF8 close payload (1.20 KB, patch)
2018-01-02 09:32 UTC, Italo Guerrieri
committed Details | Review
Fix invalid close code (1.03 KB, patch)
2018-01-02 09:33 UTC, Italo Guerrieri
reviewed Details | Review
Fix order control frame (2.89 KB, patch)
2018-01-02 09:34 UTC, Italo Guerrieri
none Details | Review
Fix UTF8 message (1.39 KB, patch)
2018-01-02 09:35 UTC, Italo Guerrieri
needs-work Details | Review
Fix payload close (957 bytes, patch)
2018-01-02 09:41 UTC, Italo Guerrieri
committed Details | Review
Fix invalid close code (1.61 KB, patch)
2018-01-11 12:03 UTC, Italo Guerrieri
committed Details | Review
Fix UTF8 message (1.91 KB, patch)
2018-01-11 12:05 UTC, Italo Guerrieri
committed Details | Review
Fix order control frame (3.25 KB, patch)
2018-01-23 09:46 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
Fix order control frame (3.23 KB, patch)
2018-01-23 13:54 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review

Description Italo Guerrieri 2018-01-02 09:18:11 UTC
Following up I will attach some patches that will improve the websocket implementation fixing some of the autobahn unit tests.
Comment 1 Italo Guerrieri 2018-01-02 09:20:01 UTC
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.
Comment 2 Italo Guerrieri 2018-01-02 09:23:04 UTC
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.*
Comment 3 Italo Guerrieri 2018-01-02 09:23:41 UTC
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.*.
Comment 4 Italo Guerrieri 2018-01-02 09:24:25 UTC
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 5 Italo Guerrieri 2018-01-02 09:29:33 UTC
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
Comment 6 Italo Guerrieri 2018-01-02 09:31:13 UTC
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.
Comment 7 Italo Guerrieri 2018-01-02 09:32:21 UTC
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.
Comment 8 Italo Guerrieri 2018-01-02 09:33:05 UTC
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.*.
Comment 9 Italo Guerrieri 2018-01-02 09:34:08 UTC
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.
Comment 10 Italo Guerrieri 2018-01-02 09:35:03 UTC
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.
Comment 11 Italo Guerrieri 2018-01-02 09:37:08 UTC
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
Comment 12 Italo Guerrieri 2018-01-02 09:41:32 UTC
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 13 Dan Winship 2018-01-10 18:39:18 UTC
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 14 Dan Winship 2018-01-10 18:42:06 UTC
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 15 Dan Winship 2018-01-10 18:45:16 UTC
Comment on attachment 366164 [details] [review]
Fix reserved opcode

"If an unknown opcode is received, the receiving endpoint MUST _Fail the WebSocket Connection_."
Comment 16 Dan Winship 2018-01-10 18:46:25 UTC
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 17 Dan Winship 2018-01-10 18:55:42 UTC
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 18 Dan Winship 2018-01-10 19:03:57 UTC
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 19 Dan Winship 2018-01-10 19:08:28 UTC
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...
Comment 20 Ignacio Casal Quinteiro (nacho) 2018-01-10 21:13:17 UTC
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
Comment 21 Italo Guerrieri 2018-01-11 12:03:59 UTC
Created attachment 366653 [details] [review]
Fix invalid close code
Comment 22 Italo Guerrieri 2018-01-11 12:05:19 UTC
Created attachment 366654 [details] [review]
Fix UTF8 message
Comment 23 Ignacio Casal Quinteiro (nacho) 2018-01-11 12:14:10 UTC
(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
Comment 24 Claudio Saavedra 2018-01-11 15:47:43 UTC
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.
Comment 25 Ignacio Casal Quinteiro (nacho) 2018-01-23 09:46:22 UTC
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.
Comment 26 Claudio Saavedra 2018-01-23 13:34:49 UTC
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!
Comment 27 Ignacio Casal Quinteiro (nacho) 2018-01-23 13:54:38 UTC
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.
Comment 28 Ignacio Casal Quinteiro (nacho) 2018-01-23 13:55:14 UTC
(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!
Comment 29 Claudio Saavedra 2018-01-23 14:01:47 UTC
Review of attachment 367311 [details] [review]:

Thank you! Assuming all tests are passing (ab 2.10 included), please go ahead and push!
Comment 30 Ignacio Casal Quinteiro (nacho) 2018-01-23 14:11:15 UTC
Claudio, can we get an ack on the other 2 patches as well?
Comment 31 Ignacio Casal Quinteiro (nacho) 2018-01-23 14:42:05 UTC
Comment on attachment 367311 [details] [review]
Fix order control frame

Attachment 367311 [details] pushed as d17d5ee - Fix order control frame
Comment 32 Ignacio Casal Quinteiro (nacho) 2018-02-01 09:02:08 UTC
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