GNOME Bugzilla – Bug 759658
xprops: Protect against broken WM_CLASS property implementations
Last modified: 2016-02-19 18:03:25 UTC
Created attachment 317656 [details] [review] xprops: Protect against broken WM_CLASS property implementations According to the ICCCM the WM_CLASS property differs from a regular STRING property in that it consists of two consecutive NULL-terminated strings where both NULLs are considered part of the property data. Some broken applications (notably java/awt/swing applications) however only include the separating NULL and do not have a trailing NULL. In these cases the old code was reading past the actual property data and considered that to be part of the class string as well. This would result in broken window/app matching and in the case that the data could not be interpreted as a valid UTF-8 string in the window-switcher and hot corner not working or the dash being empty. This patch tries to protect against all possible ways an application might get this property wrong.
Hmm, not reviewing right at the moment, but two notes: 1) \0 is a NUL not a NULL 2) Are you aware (from the man page) XGetWindowProperty always allocates one extra byte in prop_return (even if the property is zero length) and sets it to zero so that simple properties consisting of characters do not have to be copied into yet another string before use. If we are reading the results of XGetWindowProperty *directly* then it wouldn't be possible to read past the end from a missing nul. But I could imagine that we're copying the data or that the async-get-property code doesnt' have the same behavior.
Reminder that we switched to xcb_get_window_property -- I thought the NUL would be on the wire, but if not, we should implement that ourselves here: https://git.gnome.org/browse/mutter/tree/src/x11/xprops.c#n210
Created attachment 318634 [details] [review] xprops: Null-terminate property reply values Thanks for pointing me in the right direction. I've now looked at this again and it seems like xcb does intentionally not use null-terminated strings: http://xcb.freedesktop.org/XcbRationale/
Review of attachment 318634 [details] [review]: ::: src/x11/xprops.c @@ +214,3 @@ + length = xcb_get_property_value_length (reply); + // Leave room for a trailing '\0' since xcb doesn't return null-terminated + // strings Use /* comments */ @@ +217,3 @@ + results->prop = g_malloc (length + 1); + memmove (results->prop, xcb_get_property_value (reply), length); + results->prop[length] = '\0'; You can simply use g_strndup for this -- sorry.
Wouldn't using g_strndup break if this wasn't a string type property or a null-separated list of strings?
Oh, I forgot that NULs are valid in property values. OK, can you fix the comment issue and push? It's ACN after that.
Created attachment 318673 [details] [review] xprops: Null-terminate property reply values Thanks for the review, here is a version with the fixed comments. I've also slightly changed the commit message since I mentioned crashes in the previous version when I was actually thinking about the window-switcher not working due to invalid UTF-8 strings. Could you push the patch for me since I don't have a git account?
Review of attachment 318673 [details] [review]: ::: src/x11/xprops.c @@ +217,3 @@ + */ + results->prop = g_malloc (length + 1); + memmove (results->prop, xcb_get_property_value (reply), length); Nit: there's no reason to use memmove instead of memcpy here since the buffers don't overlap.
Created attachment 318820 [details] [review] xprops: Null-terminate property reply values Thanks! Here is the patch with memcpy instead of memmove.
Thanks for the patch Attachment 318820 [details] pushed as a7a376a - xprops: Null-terminate property reply values
*** Bug 757689 has been marked as a duplicate of this bug. ***
*** Bug 762073 has been marked as a duplicate of this bug. ***
*** Bug 761679 has been marked as a duplicate of this bug. ***