GNOME Bugzilla – Bug 315070
metacity crashes when managing windows that have long titles
Last modified: 2005-10-04 15:45:44 UTC
Metacity will try to draw window titles longer than X will allow. This means user's desktops can become unusable if they go to a website that sets a very long <title> for instance.
Created attachment 51679 [details] [review] truncate title to 512 characters The above patch will truncate the title of a window to 512 characters.
Note this may only happen for cairo based pango / gtk+. It doesn't seem to crash on Fedora Core 4.
In that case sounds like a bug in cairo-based pango/gtk+
I'm the Ubuntu firefox maintainer. We have this problem in Ubuntu and IMO we have to fix it for our next release. I consider the ability of a website to break the user's desktop a security problem. I've looked at the patch (51679) and it seems clear that it doesn't make the situation worse. But before I just apply it to our tree and close our bug (http://bugzilla.ubuntu.com/show_bug.cgi?id=15995) I would like to be sure that the patch is actually correct and complete. So I was wondering if anyone more familiar with metacity than me would like to comment on these questions: * Why 512 and not some other number ? * Is that the only place where the title is set ? I grepped for \btitle\b and got other stuff which I don't currently understand but seems like it might have similar problems. * Are there other strings analagous to the title, which should also be checked to ensure that metacity handles them correctly in all cases ? Thanks.
bug #317364 has discussions about the issue
Hehe, #314364 should have been filed against pango, while this bug probably should be in Metacity. So, I'm going to reassign both. :-) Anyway, to answer Ian's questions (which Ray most likely could have done too): * The WM is allowed to change the title that the app requested. Thus, picking any sane limit seems fine. Personally, I was thinking 256 while reading over bug 317364 (same bug)--which is also probably longer than would ever fit into the title anyway--but 512 is fine. * I believe frames.c is the central place that it all goes through (though I didn't check that thoroughly), so this should be fine as far as fixing the bug. However, the EWMH says that if the WM changes the title from what the app requested it ought to set _NET_WM_VISIBLE_NAME. I'm not sure how much that matters (since I can't imagine a titlebar being big enough to fit over 512 characters anyway), but it'd probably be a little cleaner to do this change in window-props.c and cover _NET_WM_VISIBLE_NAME too (besides, there might be a libwnck crasher too...). * Yes, there's also WM_ICON_NAME and _NET_WM_ICON_NAME (with a corresponding _NET_WM_VISIBLE_ICON_NAME) which correspond to WM_NAME and _NET_WM_NAME; there may be a similar bug there, though Metacity never displays the icon name and so it'd only possibly be a crasher for libwnck.
I just tried out Ray's patch along with the testcase in bug 317364 for triggering this issue. As I suspected, wnck-applet does in fact crash (and re-crashes every time it's reloaded) while Metacity is just fine. Having Metacity update to _NET_WM_VISIBLE_NAME and _NET_WM_VISIBILE_ICON_NAME should probably fix that. I'm going to ping Havoc, though, and see if he can see any other problems that might be related or have any other suggestions...
Comment on attachment 51679 [details] [review] truncate title to 512 characters tabpopup.c already has the utf8_strndup thing, should probably factor it out into util.h Looks fine to me otherwise. I can't think of any breakage cases you guys haven't covered. I think a shorter max title length might be OK, e.g. a huge monitor is 1600 pixels, if we say 5 pixels per char for a tiny font, that's 320 chars. To display 512 chars in said tiny font you would need a 2560 pixel wide monitor ... Another way to think about it is how huge a font do you need to trigger the bug, i.e. with max coordinate of 32k and max chars of 512, if the font is over 64 pixels wide you could break. This is probably not worth a lot of time stressing over the exact number though.
Created attachment 52991 [details] [review] Updated patch This patch: - factors meta_g_utf8_strndup() out into util.c as Havoc requested - factors meta_prop_set_utf8_string_hint out into xprops.c to make it easier to update _NET_WM_VISIBLE_NAME and _NET_WM_VISIBLE_ICON_NAME - adds the net_wm_visible_name and net_wm_visible_icon_name atoms to the list in display.c - truncates titles in window-props.c set_*_title() functions, while updating _NET_WM_VISIBLE_NAME and _NET_WM_VISIBLE_ICON_NAME as appropriate. The effect of this is that not only does Metacity stop crashing, but it prevents libwnck from crashing (yeah, we could do that in libwnck but...) and makes sure that if it is ever possible to display a title longer than 512 characters then Metacity and the pager display the same title. :)
2005-10-03 Elijah Newren <newren@gmail.com> Truncate ridiculously long titles to avoid crashing or letting the pager crash. Based on patch from Ray, incorporating suggestions from Havoc and some further additions. Fixes #315070. * src/display.c (set_utf8_string_hint, meta_display_open): * src/xprops.[ch] (meta_prop_set_utf8_string_hint): Move set_utf8_string_hint() to props.[ch], namespace it ("meta_prop_"), and make it public * src/tabpopup.c (utf8_strndup, meta_ui_tab_popup_new): * src/util.[ch] (meta_g_utf8_strndup): Move utf8_strndup() to util.[ch], namespace it ("meta_g_"), and make it public * src/display.c (meta_display_open): * src/display.h (struct _MetaDisplay): add net_wm_visible_name and net_wm_visible_icon_name atoms to the list of atoms we work with * src/window-props.c (set_window_title, set_icon_title): If title length is greater than 512, truncate it and set _NET_WM_VISIBLE_NAME or _NET_WM_VISIBLE_ICON_NAME accordingly
Thanks for those patches, which we are including in Ubuntu. However, I will also be applying the patch which I have just attached to my report to Mozilla upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=311052
Ian: Well, if you're going to point out additional patches that may be relevant, you could at least give us a link we're allowed to view. ;-) (I get a "You are not authorized to access bug #311052..." error message)
Oh, how annoying. Sorry. I marked the bug there as `security' because I want them to add title truncation to the next security update but of course that prevents you from seeing it. --- ../orig/firefox-1.0.7/widget/src/gtk2/nsWindow.cpp 2005-09-28 16:29:31.000000000 +0100 +++ widget/src/gtk2/nsWindow.cpp 2005-10-04 12:14:47.000000000 +0100 @@ -935,11 +935,16 @@ NS_IMETHODIMP nsWindow::SetTitle(const nsString& aTitle) { + const PRUint32 aTitleLenMax = 256; + if (!mShell) return NS_OK; // convert the string into utf8 and set the title. - NS_ConvertUCS2toUTF8 utf8title(aTitle); + PRUint32 aTitleLen = aTitle.Length(); + if (aTitleLen > aTitleLenMax) aTitleLen = aTitleLenMax; + const nsSubstring& aTitleTruncated = Substring(aTitle, 0, aTitleLen); + NS_ConvertUCS2toUTF8 utf8title(aTitleTruncated); gtk_window_set_title(GTK_WINDOW(mShell), (const char *)utf8title.get()); return NS_OK;
Wasn't that firefox/mozilla bug already fixed at https://bugzilla.mozilla.org/show_bug.cgi?id=167315?