GNOME Bugzilla – Bug 767241
wayland: Long window titles crash clients
Last modified: 2016-06-08 13:13:12 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.
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
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?
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 ?
Looks like a libwayland bug to me. I think wl_connection_write is missing a loop to deal with size > 4096 in chunks.
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.
(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.
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 ?
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.).
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.
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.
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
Pushed, thanks.