GNOME Bugzilla – Bug 599103
UTMP Session registration code doesn't work with remote users
Last modified: 2015-08-14 19:24:09 UTC
Created attachment 145888 [details] [review] Patch for gdm session registration code that adds information to ut_line when logging in remotely Hi! As was already explained by Maximiliano Curia in this mail [1], the code that was added to gdm to handle session registration does not work as expected with remote users. [1] http://mail.gnome.org/archives/gdm-list/2009-October/msg00004.html This is because the first 4 characters of the hostname are used as the -supposedly unique- identifier, and since the hostname is not resolved, this first 4 characters are probably going to be "192." or if another network is used, it's very probable that they'll be the same. This means that only the last logged in user is shown, even if many others are also logged in. Maximiliano and I have prepared a patch, and we have tested in our environment (we heavily relay on remote sessions) and works correctly. This patch fixes the problem by leaving the ut_id empty, and filling the ut_line, which allows 32 characters. If the hostname is more than 32 characters long, it's truncated so that the display part (:0, :1, :0.0, etc) is always shown. The libc specifies that if no ut_id is given, then ut_id is ignored and ut_line is used. If leaving ut_id empty is not considered nice for some reason, the other possible option is to apply a hash function to the whole line and transform that into the ut_id (this is what sessreg currently does). The attached patch was tested against 2.20.7 (Debian Lenny) and 2.20.9 (Debian Sid).
I guess this is also a problem for you with 2.28? If so, would you mind updating the patch?
I'm going to close this bug out, but if it's still a problem in the latest release of GDM please reopen.
I observe this problem still, at Debian wheezy version 3.4.1-3. Cheers, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
Created attachment 226224 [details] [review] Patch for version 3.4 or 3.6 I propose the patch attached, goes cleanly into versions 3.4.1 and 3.6.0. I have not yet tested this patch, plan to do soon. Could this bug be re-opened, please? (I do not seem to be able to do.) Cheers, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
Created attachment 226431 [details] [review] Patch for version 3.4 or 3.6 I have now tested this patch, it works fine for me. Please consider accepting it. Cheers, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
Review of attachment 226431 [details] [review]: Thanks for working on this. I think this is probably okay, but it needs some minor changes to integrate with the code base better before it can be commited. What I mean is, after the patch is applied a reader shouldn't really be able to tell which parts of the code were patched. It should make the codebase better without leaving a trail. ::: daemon/gdm-session-record.c @@ +132,3 @@ { #if defined(HAVE_UT_UT_ID) +/* PSz 11 Oct 12 No need to tag the comment, people can look a the git logs to get that kind of information. @@ +157,3 @@ + * https://bugzilla.gnome.org/show_bug.cgi?id=599103 + */ + return; if we're returning here, we should just drop the entire function instead of keeping dead code under the return; @@ +182,3 @@ + * IP.of.cli.ent:0 (and not host_name and "plain" :0 separately). This mess + * with hostname is wasteful, in fact we just use x11_display_name as is. + */ This comment doesn't add much value over the comment above it. Should just drop it, or potentially clarify the comment above, if you think it's vague. @@ +228,3 @@ + * IP.of.cli.ent:0 (and not to "plain" :0). Use this as is, do not leave + * ut_line blank. (Previous "else if" clause wasteful.) + */ you could roll this into the existing else if block by getting rid of the && g_str_has_prefix bit. @@ +260,3 @@ /* Set ut_id to the $DISPLAY value */ + /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */ +/* record_set_id (&session_record, x11_display_name); */ don't comment it out, just remove it. @@ +291,3 @@ + * pututxline (&session_record); + * here. + */ If you'd like to simplify the code here, that's fine with me. We shouldn't have comments in the code to that effect though (well unless they're in FIXME: format) @@ +338,3 @@ /* Set ut_id to the $DISPLAY value */ + /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */ +/* record_set_id (&session_record, x11_display_name); */ don't comment this out, just delete it. @@ +361,3 @@ + * g_debug ("Adding or updating utmp record for logout"); + * pututxline (&session_record); + * here. Same goes here, clean ups are fine and FIXME:'s are sometimes okay, but we want the code base to be coherent, so comments should be written in the "person" of "code author", not as a 3rd party. @@ +420,3 @@ /* Set ut_id to the $DISPLAY value */ + /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */ +/* record_set_id (&session_record, x11_display_name); */ just delete this, don't comment it out.
are you planning on picking this back up Paul?
Dear Ray, I thought we were talking just about style, comments (and dead code) within my patch; and thought that others (you?) were more familiar with the style desired or required. Would you be able to make the right changes? I can give it another go, but was (am) afraid that maybe my style will still not be acceptable. Thanks, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
if you don't want to make the follow up changes, that's fine. your contribution is still very much appreciated. I or someone will update it eventually and integrate it.
Created attachment 264592 [details] [review] Patch for version 3.4 or 3.6 Dear Ray, I re-wrote my patch attempting to follow your instructions. (As yet this is un-tested: I did not yet get around to build GDM with it.) Cheers, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
I have now tested my patch id=264592 in Comment#10 (tested at Debian wheezy), and it worked perfectly. Please consider accepting it. Cheers, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
thanks, for following up with the testing, will look soonish
(In reply to Ray Strode [halfline] from comment #12) > thanks, for following up with the testing, will look soonish That was 18 months ago. Could this be fixed, please? Thanks, Paul Paul Szabo psz@maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/ School of Mathematics and Statistics University of Sydney Australia
i've updated the patch to apply against master and split it up, will post here momentarily
(sorry this fell off my radar)
Created attachment 309303 [details] [review] session-record: fix use after free hostname is getting freed before it's getting used if ut_syslen is available. This commit moves the free down a couple of lines
Created attachment 309304 [details] [review] session-record: simplify utmp updating logic The code carefully iterates over all utmp entries trying to update existing records if found. This is not necessary putuxline will do that for us.
Created attachment 309305 [details] [review] session-record: don't set ut_id It's only 4 bytes big and it's really hard to come up with a useful, unique id.
Created attachment 309306 [details] [review] session-record: write ut_line for remote logins too We currently only write out ut_line for local logins. This commit makes sure it gets written out for remote logins as well.
Review of attachment 309303 [details] [review]: ::: daemon/gdm-session-record.c @@ +165,3 @@ u->ut_syslen = MIN (strlen (hostname), sizeof (u->ut_host)); #endif + g_free (hostname); clearly right, though linux doesn't have ut_syslen. I'm a little surprised this didn't get noticed by the bsd crowd in the intervening years.
Review of attachment 309304 [details] [review]: hmm man page says this: pututline() writes the utmp structure ut into the utmp file. It uses getutid() to search for the proper place in the file to insert the new entry. If it cannot find an appropriate slot for ut, pututline() will append the new entry to the end of the file. so i guess this is right. I wonder if there are platform differences here.
Review of attachment 309305 [details] [review]: okay whatever.
Review of attachment 309306 [details] [review]: I guess this part is the core of the bug. ::: daemon/gdm-session-record.c @@ -176,3 @@ sizeof (u->ut_line)); - } else if (x11_display_name != NULL - && g_str_has_prefix (x11_display_name, ":")) { I think this g_str_has_prefix was just there on accident because the display_device condition above had a g_str_has_prefix. I really don't think it's needed so i think your patch is right.
Attachment 309303 [details] pushed as 6f8c119 - session-record: fix use after free Attachment 309304 [details] pushed as d8e1d38 - session-record: simplify utmp updating logic Attachment 309305 [details] pushed as ab058ea - session-record: don't set ut_id Attachment 309306 [details] pushed as 4c282cd - session-record: write ut_line for remote logins too