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 770022 - Too low payload safety value on websockets
Too low payload safety value on websockets
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: HTTP Transport
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2016-08-17 06:49 UTC by Ignacio Casal Quinteiro (nacho)
Modified: 2016-08-22 17:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
websocket-test: add unit test for bigger packets than supported (1000 bytes, patch)
2016-08-17 07:19 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
patch (4.56 KB, patch)
2016-08-19 19:49 UTC, Paolo Borelli
reviewed Details | Review
Add max-payload-size property to the websocket connection. (7.18 KB, patch)
2016-08-22 13:46 UTC, Ignacio Casal Quinteiro (nacho)
accepted-commit_now Details | Review
Add max-incoming-payload-size property to the websocket connection. (7.55 KB, patch)
2016-08-22 17:05 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review

Description Ignacio Casal Quinteiro (nacho) 2016-08-17 06:49:52 UTC
libsoup sets a value of 128 * 1024 for the websocket. This is too low if you are i.e sending frames on the transport. Can we avoid this?
Comment 1 Ignacio Casal Quinteiro (nacho) 2016-08-17 07:00:59 UTC
Actually I wonder if the problem is the server that in some case it sends more bytes than it should (this is with both server and client being libsoup). This is the message that I am getting:

(niceviewer:17519): libsoup-DEBUG: client is trying to frame of size 131155 or greater, but max supported size is 128KiB


131155 (128.08) looks suspicious.
Comment 2 Ignacio Casal Quinteiro (nacho) 2016-08-17 07:07:28 UTC
Actually I do not see anywhere where sending binary data libsoup ensures that the frames is of a max 128KiB. If sending binary data is supported we should definitely remove this check I guess.
Comment 3 Ignacio Casal Quinteiro (nacho) 2016-08-17 07:19:13 UTC
Created attachment 333450 [details] [review]
websocket-test: add unit test for bigger packets than supported
Comment 4 Ignacio Casal Quinteiro (nacho) 2016-08-17 07:19:56 UTC
You can run the unit test above to reproduce the issue:
G_MESSAGES_DEBUG=all ./websocket-test
Comment 5 Paolo Borelli 2016-08-19 19:49:00 UTC
Created attachment 333694 [details] [review]
patch

(posting on behalf of Nacho)

This patch adds a property to set the max (default value is the same as before).
The patch adds a unit test as well
Comment 6 Dan Winship 2016-08-22 13:22:27 UTC
Comment on attachment 333694 [details] [review]
patch

1. add soup_websocket_connection_get/set_max_payload_size() please (with Since tags and SOUP_AVAILABLE_IN_...)

2. rename MAX_PAYLOAD to MAX_PAYLOAD_SIZE_DEFAULT

3. clarify in the docs that this is for incoming packets only and the code doesn't limit outgoing packet size
Comment 7 Ignacio Casal Quinteiro (nacho) 2016-08-22 13:46:36 UTC
Created attachment 333905 [details] [review]
Add max-payload-size property to the websocket connection.

This allows to change the limit for the payload of websocket packets.
Also add the corresponding unit test.
Comment 8 Dan Winship 2016-08-22 16:17:44 UTC
Comment on attachment 333905 [details] [review]
Add max-payload-size property to the websocket connection.

looks good. if you want to rename it as per IRC, I think "max-incoming-payload-size" is probably better than "max-incoming-packet-size", since the payload may be made up of many tcp packets
Comment 9 Ignacio Casal Quinteiro (nacho) 2016-08-22 17:05:15 UTC
Created attachment 333930 [details] [review]
Add max-incoming-payload-size property to the websocket connection.

This allows to change the limit for the payload of websocket packets.
Also add the corresponding unit test.
Comment 10 Ignacio Casal Quinteiro (nacho) 2016-08-22 17:06:15 UTC
Attachment 333930 [details] pushed as d4c6819 - Add max-incoming-payload-size property to the websocket connection.