GNOME Bugzilla – Bug 594270
simple-greeter crashes on bad UTF-8 in passwd file
Last modified: 2009-10-22 06:38:32 UTC
Created attachment 142564 [details] [review] fix (patch) simple-greeter crashes on bad UTF-8 in passwd file. The problem was a missing NULL check after calling g_locale_to_utf8 (it seems I had an old user in my system with his name encoded in ISO-8859-1 rather than UTF-8).
I could not find g_locale_to_utf8() in gdm-user.c: http://svn.gnome.org/viewvc/gdm/trunk/gui/simple-greeter/gdm-user.c?view=markup
But I do find it here: http://git.gnome.org/cgit/gdm/tree/gui/simple-greeter/gdm-user.c
You are right. I haven't tried git. I'd like to remove g_locale_to_utf8() and /etc/passwd should be UTF-8.
I would do what I did =). You need to check the name for valid UTF-8 anyway, so why using that function?
Created attachment 142678 [details] [review] Patch for gdm-user.c I added g_utf8_validate() and a warning message. This can prevent GDM from showing the invalid chars.
>+ g_warning ("User %s has invalid UTF-8 in GECOS field. " >+ "It's better to check /etc/passwd.", >+ pwent->pw_name ? pwent->pw_name : ""); Sounds weird... I guess it would be better this: "User %s has invalid UTF-8 in GECOS field. " "It would be a good thing to check /etc/passwd.", (disclaimer: IANANS - I'm not a native speaker)
Created attachment 142757 [details] [review] Patch for gdm-user.c OK, I updated the warning message.
Comment on attachment 142757 [details] [review] Patch for gdm-user.c Well, there is no rule that says /etc/passwd has to be in UTF-8. It's encoding isn't defined anywhere as far as I know. Now the question is what should we do in the face of it not being utf-8? Just showng username instead makes sense. I think that's what this patch does? It probably wouldn't hurt to attempt g_locale_to_utf8 if validate fails just in case it works. Otherwise, patch seems fine to me.
> It probably wouldn't hurt to attempt g_locale_to_utf8 if validate fails just in > case it works. Better yet: just try to convert it without validating, like my patch did. The conversion will fail if the encoding is neither UTF-8 nor in the current locale.
> Better yet: just try to convert it without validating I would think the just showing means to show the string without iconv. GtkLabel and XFT accepts UTF-8 so it would make sense to have the validation. If the validation is failed, it would be an option to use the convertion. Since there is no specification of GECOS encoding, it would possible one user write UTF-8, a user writes ISO8859-1, a user writes EUC-JP. It would be nice, saved encoding is UTF-8 and the viewer application can shows the string on the current encoding. It's the same situation with syslog, cups config. The suggestion would work with other applications likes gnome-panel, gdm-user-switch-applet.
The point is /etc/passwd is the system file instead of the user file. a system can be logged into with multiple users, one is UTF-8 user, another is non-UTF-8 user. Also NIS environment could provide the GECOS field from multiple hosts. That's why I would think the saving file is UTF-8 and the viewer application can shows the string on the current locale.
I thknk that the best option would be: Try UTF-8, then locale, and then just show the username. The good thing is that UTF-8 is very particular, and something encoded with another locale will never validate as UTF-8. And if it's not UTF-8, many times the system locale is the user's locale. Multi-locale machines are the uncommon case. Before UTF-8, the way of having a Linux system in your locale was just make sure that LANG was set for everybody (so everybody had the user locale according to the country). There wasn't the idea of a system locale really. That was the case with my machine, which had still ISO-8859-1 names in /etc/passwd from before the UTF-8 "switch".
Created attachment 144926 [details] [review] Patch for gdm/daemon/gdm-welcome-session.c, gdm/daemon/gdm.in, gdm/gui/simple-greeter/gdm-user.c Since UTF-8 and non-UTF-8 has the duplicated code points, if we run g_locale_to_utf8() after g_utf8_validate(), some non-UTF-8 encodings could show the broken chars. If you need the option to show non-UTF-8 GECOS, I think a configuration is needed to choose either UTF-8 or the current encoding. Now I think the update patch likes G_BROKEN_FILENAME. If the line is uncommented in /usr/sbin/gdm, gdm thinks the system config file is encoded by the gdm encoding.
Hi, > Created an attachment (id=144926) [details] > Patch for gdm/daemon/gdm-welcome-session.c, gdm/daemon/gdm.in, > gdm/gui/simple-greeter/gdm-user.c > > Since UTF-8 and non-UTF-8 has the duplicated code points, if we run > g_locale_to_utf8() after g_utf8_validate(), some non-UTF-8 encodings could show > the broken chars. You know what, you've convinced me. Let's avoid the heuristics. > If you need the option to show non-UTF-8 GECOS, I think a configuration is > needed to choose either UTF-8 or the current encoding. I don't think we really need it. > Now I think the update patch likes G_BROKEN_FILENAME. > If the line is uncommented in /usr/sbin/gdm, gdm thinks the system config file > is encoded by the gdm encoding. Let's avoid this and commit your first patch.
Thanks for the review. I'll commit the attachment 142757 [details] [review] after gnome-2-28 branch is created.
Committed the patch. Could you close the bug since I'm not the bug submitter? commit 8b8521dd83637d4e83d67adddefba4f30c99715c Author: Takao Fujiwara <tfujiwar@redhat.com> Date: 2009-10-22 15:20:49 +0900 Fix real names not to crash greeter. Bug #594270 Reviewed by Ray Strode M gui/simple-greeter/gdm-user.c