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 767241 - wayland: Long window titles crash clients
wayland: Long window titles crash clients
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
3.21.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-06-04 14:41 UTC by Timm Bäder
Modified: 2016-06-08 13:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: Make sure window titles fit into a wl_buffer (1.43 KB, patch)
2016-06-04 14:41 UTC, Timm Bäder
none Details | Review
Make sure app-id and title fit into a wl_buffer (2.91 KB, patch)
2016-06-07 13:27 UTC, Timm Bäder
none Details | Review
wayland: Make sure window titles fit into a wl_buffer (1.76 KB, patch)
2016-06-08 05:35 UTC, Timm Bäder
accepted-commit_now Details | Review

Description Timm Bäder 2016-06-04 14:41:51 UTC
Created attachment 329123 [details] [review]
wayland: Make sure window titles fit into a wl_buffer

The max size for a wl_buffer is 4096 bytes, so setting a very long window title might exceed that.
Comment 1 Matthias Clasen 2016-06-04 16:27:02 UTC
Review of attachment 329123 [details] [review]:

Is there any documentation for the max size of wl_buffer ? I couldn't find any information about that, and it seems unlikely to me
Comment 2 Timm Bäder 2016-06-04 16:31:57 UTC
Carlos just linked https://cgit.freedesktop.org/wayland/wayland/tree/src/connection.c#n49, the rest was based on termite setting the window title to http://paste.fedoraproject.org/374369/65041958/, and I can reproduce the crash with a simple hello world example of gtk_window_set_title. Do you think this is a bug in wayland/xdg-shell/... instead?
Comment 3 Matthias Clasen 2016-06-04 16:47:02 UTC
But the buffers we use are created by wl_shm_pool_create_buffer. We use it for the entire window contents w*scale * h*scale * 4 bytes - that can easily go beyond 4096. I must say wl_buffer is very confusing in the wayland codebase. There's some headers where it is deprecated, e.g.

Clearly, more information is needed. Jonas ?
Comment 4 Matthias Clasen 2016-06-04 17:51:52 UTC
Looks like a libwayland bug to me.

I think wl_connection_write is missing a loop to deal with size > 4096 in chunks.
Comment 5 Matthias Clasen 2016-06-04 18:12:02 UTC
Alternatively, the wayland documentation should clearly state that string arguments will be limited to 4088 bytes. That probably affects not just the title, but also the app id in the xdg-shell protocol.
Comment 6 Carlos Garnacho 2016-06-05 10:45:24 UTC
(In reply to Matthias Clasen from comment #3)
> But the buffers we use are created by wl_shm_pool_create_buffer. We use it
> for the entire window contents w*scale * h*scale * 4 bytes - that can easily
> go beyond 4096. I must say wl_buffer is very confusing in the wayland
> codebase. There's some headers where it is deprecated, e.g.
> 
> Clearly, more information is needed. Jonas ?

AFAICS here wl_buffer seems to refer to lowlevel message buffers, not the high level "image buffer" object exposed in the protocol.

(In reply to Matthias Clasen from comment #4)
> Looks like a libwayland bug to me.
> 
> I think wl_connection_write is missing a loop to deal with size > 4096 in
> chunks.

If I did read the code correctly yesterday, each of those wl_buffer structs must begin with the object ID, opcode, etc. So effectively a single request can't be split in chunks.

(In reply to Matthias Clasen from comment #5)
> Alternatively, the wayland documentation should clearly state that string
> arguments will be limited to 4088 bytes. That probably affects not just the
> title, but also the app id in the xdg-shell protocol.

Agreed about the general lack of documentation, this potentially affects every request that has variable length fields (strings and arrays basically). Will bring it up on wayland-devel.
Comment 7 Matthias Clasen 2016-06-07 11:03:59 UTC
Review of attachment 329123 [details] [review]:

Ok, after talking to Jonas, lets go with this approach. Can you also apply a similar change where we call xdg_surface_set_app_id, please ?
Comment 8 Timm Bäder 2016-06-07 13:27:06 UTC
Created attachment 329273 [details] [review]
Make sure app-id and title fit into a wl_buffer

The old implementation crashed for empty titles. I didn't know if it was worth it to introduce a helper function for this (or how to call it, etc.).
Comment 9 Matthias Clasen 2016-06-07 20:30:40 UTC
I'm sorry I was misleading you here. Application ids must be valid bus names, which means they are a) ASCII only and b) limited to 255 bytes.
Comment 10 Timm Bäder 2016-06-08 05:35:45 UTC
Created attachment 329364 [details] [review]
wayland: Make sure window titles fit into a wl_buffer

I guess we don't need anything for the app-id then. This version works for both >4096/utf8 titles and empty ones for me.
Comment 11 Matthias Clasen 2016-06-08 12:08:41 UTC
Review of attachment 329364 [details] [review]:

::: gdk/wayland/gdkwindow-wayland.c
@@ +55,3 @@
    GDK_WINDOW_TYPE (window) != GDK_WINDOW_OFFSCREEN)
 
+#define MAX_WL_BUFFER_SIZE (4083) /* 4096 minus header, string argument length and NUL byte */

A little odd to exclude the NUL byte in this count, since it is an integral part of the payload. But, ok
Comment 12 Timm Bäder 2016-06-08 13:13:12 UTC
Pushed, thanks.