Old sessreg, gdm utmpwtmp and utmp->ut_id

[Maximiliano Curia-6]

Hi,

I'm running a few thinclient servers that use gdm as xdmcp. Gdm while a nice
xdmcp server fails to store utmpwtmp information for remote clients.

Back in the days when gdm Presession called sessreg, I've to patch sessreg
to store a hash of the display string in utmp->ut_id, as the id was what glibc
used a "unique identifier" in the utmp file, and without the patch all the
users in my network ended up with the same id, this caused commands like who,
to list only the last login session, thus a pain to administer. The hash used had
a very small collission rate, so, it worked quite nicelly. The patch was
accepted in sessreg, so every other admin might not have noticed the problem.

After upgrading to gdm 2.20, the code to update utmpwtmp was moved to gdm
itself, and it's coded only for local users, remote users won't set ut_line or
ut_id, so the same problem back again. Commands like who, would only report the
last logged in user. In addition, gdm won't obtain the hostname, so, only the
ip address is stored. Once again a pain to administed effectivelly.

So, once again, I'll need to fix this. I'm looking at the code of gdm 2.21,
and it seems to have some remote users utmpwmtp registering, but it uses the
first 4 char of the display, which is quite prune to generate collissions,
also, the code of utmpx in glibc uses ut_id only optionally now, so it might be
better to use ut_line as the "unique identifier", and forget about ut_id (it
must be zeroed).

As POSIX defines ut_line as a 32 bytes long string (using utmpx, which gdm
2.21 is using), it might be needed to tweak the string a bit (making sure the
display number is available, even with a long display string). I don't really
know the status of vnc remote display or X11 disconnection support nor the
display string those would generate, but when those are defined it might be
needed to tweak the ut_line once again.

Also, gdm_session_record_login and gdm_session_record_logout use getutxent to
iterate over the utmp file, which is quite inefficient, as that's the job of
getutxline.

If everything sound fine to you, I'll prepare and send the patch.

Happy hacking,
--
"The day Microsoft makes something that doesn't suck,
is probably the day Microsoft starts making vacuum cleaners."
        -- Ernst Jan Plugge
Saludos /\/\ /\ >< `/
_______________________________________________
gdm-list mailing list
gdm-list@...
http://mail.gnome.org/mailman/listinfo/gdm-list

[Brian Cameron]

Maximiliano:

> I'm running a few thinclient servers that use gdm as xdmcp. Gdm while a nice
> xdmcp server fails to store utmpwtmp information for remote clients.
>
> Back in the days when gdm Presession called sessreg, I've to patch sessreg
> to store a hash of the display string in utmp->ut_id, as the id was what glibc
> used a "unique identifier" in the utmp file, and without the patch all the
> users in my network ended up with the same id, this caused commands like who,
> to list only the last login session, thus a pain to administer. The hash used had
> a very small collission rate, so, it worked quite nicelly. The patch was
> accepted in sessreg, so every other admin might not have noticed the problem.

GDM 2.20 and later uses the display name as the ut_id identifier.  Users
should not get the same value for ut_id unless they are using the same
display.  Is this a problem?  If so, could you explain in more detail
why?

> After upgrading to gdm 2.20, the code to update utmpwtmp was moved to gdm
> itself, and it's coded only for local users, remote users won't set ut_line or
> ut_id, so the same problem back again. Commands like who, would only report the
> last logged in user. In addition, gdm won't obtain the hostname, so, only the
> ip address is stored. Once again a pain to administed effectivelly.

That is not true.  Looking at the code, GDM 2.20 and later does set
ut_id to the display value regardless of whether the display is local or
remote.

GDM will set ut_line to the device name associated with the device.  It
will only not set it if the device name is NULL.  If the display device
is NULL, then it should set the value to the $DISPLAY.

The only special handling for remote displays is that ut_host should be
set to the hostname:$DISPLAY value if the session is associated with a
remote machine.

You seem to suggest that the above logic is not working for you.  If so,
we should find out why.  Is there a bug in the code, or are the various
"HAVE_UT_*" #defines not being set properly on your platform?

> So, once again, I'll need to fix this. I'm looking at the code of gdm 2.21,
> and it seems to have some remote users utmpwmtp registering, but it uses the
> first 4 char of the display, which is quite prune to generate collissions,

Note that the latest version of GDM is 2.28.0.  I would recommend
working with the latest code rather than the older 2.21 version.  Since
all development effort has moved to the new 2.28.0 version, it probably
does not make sense to spend much effort trying to fix older versions.
It is unlikely that there will be another release of the "old GDM"
(version 2.20) unless there is a very compelling reason (e.g. a security
issue).

The ut_id value is indeed defined in /usr/include/ut_id to be a 4
character long string.  Is the problem you have that the display
values are getting truncated so that you do not have unique values?

> also, the code of utmpx in glibc uses ut_id only optionally now, so it might be
> better to use ut_line as the "unique identifier", and forget about ut_id (it
> must be zeroed).

Note that GDM support for utmpx needs to work on a wide variety of
platforms.  I have concerns about changing this in ways that might cause
problems for downstream distros.

What do other display managers, and other programs which set utmpx
records do?  It probably makes sense for GDM to be consistent with other
programs.

Perhaps if there are certain use-cases where it makes sense to set utmpx
differently than normal, it might make sense to make it possible to
configure GDM to allow this.

> As POSIX defines ut_line as a 32 bytes long string (using utmpx, which gdm
> 2.21 is using), it might be needed to tweak the string a bit (making sure the
> display number is available, even with a long display string). I don't really
> know the status of vnc remote display or X11 disconnection support nor the
> display string those would generate, but when those are defined it might be
> needed to tweak the ut_line once again.
>
> Also, gdm_session_record_login and gdm_session_record_logout use getutxent to
> iterate over the utmp file, which is quite inefficient, as that's the job of
> getutxline.

Any changes that you want to propose to make the code run more
efficiently would likely be accepted upstream with no difficulty,
assuming that they work well across different distributions.

> If everything sound fine to you, I'll prepare and send the patch.

It is not yet clear to me exactly what problems you are trying to solve.
Some of your analysis above does not seem correct to me.  For example,
you seem to suggest that ut_line and ut_id are not set for remote users,
which does not seem to match what the code does.

I also think we need to be careful to make an effort to not break how
utmpx is handled on different platforms.  So, any proposed changed
should be tested on a variety of platforms before being going upstream,
I'd think.

So, I think we need to discuss further.  A proposed patch would likely
help the discussion, and make it possible to do such cross-platform
testing.

Brian
_______________________________________________
gdm-list mailing list
gdm-list@...
http://mail.gnome.org/mailman/listinfo/gdm-list

[Maximiliano Curia-2]

Hola Brian Cameron!

El 19/10/2009 a las 17:44 escribiste:
> GDM 2.20 and later uses the display name as the ut_id identifier.  Users
> should not get the same value for ut_id unless they are using the same
> display.  Is this a problem?  If so, could you explain in more detail
> why?

The problem is that ut_id is defined to be 4 bytes long, so using a remote
display, you get the byte sequence: '1','9','2' and '.'

The usage of ut_id is alse defined as optional, so if its fill with zeroes it
not used, and ut_line is used instead.

> That is not true.  Looking at the code, GDM 2.20 and later does set
> ut_id to the display value regardless of whether the display is local or
> remote.

> GDM will set ut_line to the device name associated with the device.  It
> will only not set it if the device name is NULL.  If the display device
> is NULL, then it should set the value to the $DISPLAY.

There is no device asociated with a xdmcp conection, therefore ut_line is not
being set for remote connections in gdm 2.20. In gdm git, it would set ut_line
to the $DISPLAY only if $DISPLAY starts with ':', so it's also not being set.

> The only special handling for remote displays is that ut_host should be
> set to the hostname:$DISPLAY value if the session is associated with a
> remote machine.

I don't know in which case that is being set, as $DISPLAY is already of the
form host:display_number. I suspect its being used with display migration so I
didn't change it, but that hostname composition might be wrong.

> You seem to suggest that the above logic is not working for you.  If so,
> we should find out why.  Is there a bug in the code, or are the various
> "HAVE_UT_*" #defines not being set properly on your platform?

Are you suggesting that it works for you? To test it you'll need at least two
clients connecting via xdmcp.

> Note that GDM support for utmpx needs to work on a wide variety of
> platforms.  I have concerns about changing this in ways that might cause
> problems for downstream distros.

> What do other display managers, and other programs which set utmpx
> records do?  It probably makes sense for GDM to be consistent with other
> programs.

xdm calls sessreg (which, at least in debian, still has the hash implementation
mentioned before). I haven't checked the behavior of kdm, the source code is
too big.

The old sessreg behavior was to copy the last part of the ut_line (taking the
display number and part of the domain).

> So, I think we need to discuss further.  A proposed patch would likely
> help the discussion, and make it possible to do such cross-platform
> testing.

I'm attaching a patch for git, also, Margarita Manterola reported and
submitted the patch for 2.20 [1]

The changes can also be fetched from my personal git [2]

[1]: https://bugzilla.gnome.org/show_bug.cgi?id=599103
[2]: http://www.maxy.com.ar/gnome/gdm.git

--
"It is not the task of the University to offer what society asks for, but to
give what society needs." -- Edsger W. Dijkstra
Saludos /\/\ /\ >< `/

---

commit db8dfa01f888bbf4b63d52f5d9e3d3766b9ade51
Author: Maximiliano Curia <maxy@...>
Date:   Wed Oct 21 13:26:23 2009 -0300

    Replacing a one time while with a simple if

diff --git a/daemon/gdm-session-record.c b/daemon/gdm-session-record.c
index 8ca0e4f..49237fa 100644
--- a/daemon/gdm-session-record.c
+++ b/daemon/gdm-session-record.c
@@ -328,14 +328,9 @@ gdm_session_record_logout (GPid                  session_pid,
 #if defined(HAVE_GETUTXENT)
         setutxent ();
 
-        while ((u = getutxline (&session_record)) != NULL) {
+        if ((u = getutxline (&session_record)) != NULL) {
 
                 g_debug ("Removing utmp record");
-                if (u->ut_pid == session_pid &&
-                    u->ut_type == DEAD_PROCESS) {
-                        /* Already done */
-                        break;
-                }
 
                 u->ut_type = DEAD_PROCESS;
 #if defined(HAVE_UT_UT_TV)

commit 1121c86629798f294c56d131fd8e33f937df5db8
Author: Maximiliano Curia <maxy@...>
Date:   Wed Oct 21 13:14:32 2009 -0300

    Removing unneeded call to getutxent

diff --git a/daemon/gdm-session-record.c b/daemon/gdm-session-record.c
index e65c79e..8ca0e4f 100644
--- a/daemon/gdm-session-record.c
+++ b/daemon/gdm-session-record.c
@@ -328,8 +328,7 @@ gdm_session_record_logout (GPid                  session_pid,
 #if defined(HAVE_GETUTXENT)
         setutxent ();
 
-        while ((u = getutxent ()) != NULL &&
-               (u = getutxline (&session_record)) != NULL) {
+        while ((u = getutxline (&session_record)) != NULL) {
 
                 g_debug ("Removing utmp record");
                 if (u->ut_pid == session_pid &&

commit 1725094cadcf2896a8fed59313494f92d3f0d8f9
Author: Maximiliano Curia <maxy@...>
Date:   Wed Oct 21 12:35:51 2009 -0300

    Remote display utmp fix

diff --git a/daemon/gdm-session-record.c b/daemon/gdm-session-record.c
index 4ed60a1..e65c79e 100644
--- a/daemon/gdm-session-record.c
+++ b/daemon/gdm-session-record.c
@@ -136,36 +136,38 @@ record_set_id (UTMP       *u,
 #endif
 }
 
-static void
-record_set_host (UTMP       *u,
-                 const char *x11_display_name,
-                 const char *host_name)
+static char *get_hostname(const char* x11_display_name, const char* host_name)
 {
-        char *hostname;
-
-#if defined(HAVE_UT_UT_HOST)
-        hostname = NULL;
-
         /*
-         * Set ut_host to hostname:$DISPLAY if remote, otherwise set
+         * Returns hostname:$DISPLAY if remote, otherwise set
          * to $DISPLAY
          */
         if (host_name != NULL
             && x11_display_name != NULL
             && g_str_has_prefix (x11_display_name, ":")) {
-                hostname = g_strdup_printf ("%s%s", host_name, x11_display_name);
-        } else {
-                hostname = g_strdup (x11_display_name);
+                return g_strdup_printf ("%s%s", host_name, x11_display_name);
         }
+ return g_strdup (x11_display_name);
+}
+
+static void
+record_set_host (UTMP       *u,
+                 const char *x11_display_name,
+                 const char *host_name)
+{
+        char *hostname;
+
+#if defined(HAVE_UT_UT_HOST)
+        hostname = get_hostname(x11_display_name, host_name);
 
         if (hostname != NULL) {
                 strncpy (u->ut_host, hostname, sizeof (u->ut_host));
                 g_debug ("using ut_host %.*s", (int) sizeof (u->ut_host), u->ut_host);
-                g_free (hostname);
 
 #ifdef HAVE_UT_UT_SYSLEN
                 u->ut_syslen = MIN (strlen (hostname), sizeof (u->ut_host));
 #endif
+                g_free (hostname);
         }
 #endif
 }
@@ -173,7 +175,8 @@ record_set_host (UTMP       *u,
 static void
 record_set_line (UTMP       *u,
                  const char *display_device,
-                 const char *x11_display_name)
+                 const char *x11_display_name,
+ const char *host_name)
 {
         /*
          * Set ut_line to the device name associated with this display
@@ -185,13 +188,29 @@ record_set_line (UTMP       *u,
                 strncpy (u->ut_line,
                          display_device + strlen ("/dev/"),
                          sizeof (u->ut_line));
-        } else if (x11_display_name != NULL
-                   && g_str_has_prefix (x11_display_name, ":")) {
-                strncpy (u->ut_line,
-                         x11_display_name,
-                         sizeof (u->ut_line));
-        }
-
+        } else {
+ char *hostname = get_hostname(x11_display_name, host_name);
+ if ( hostname != NULL ) {
+            strncpy (u->ut_line, hostname, sizeof (u->ut_line));
+
+ /* If the hostname was too long put the display at the end */
+ if ( strlen(hostname)+1 > sizeof(u->ut_line) ) {
+ size_t len_display = 0;
+ char *sep = strrchr(hostname,':');
+ len_display = strlen(sep);
+
+ if ( sep && ( len_display+1 < sizeof (u->ut_line) ) )
+ {
+ strncpy(u->ut_line + sizeof (u->ut_line) -
+ (len_display+1), sep, len_display+1 );
+ } else {
+ /* if no display was found (or display was too
+ * long), make sure it's a string */
+ u->ut_line[sizeof (u->ut_line) - 1] = '\0';
+ }
+ }
+ }
+ }
         g_debug ("using ut_line %.*s", (int) sizeof (u->ut_line), u->ut_line);
 }
 
@@ -218,9 +237,11 @@ gdm_session_record_login (GPid                  session_pid,
         record_set_pid (&session_record, session_pid);
 
         /* Set ut_id to the $DISPLAY value */
-        record_set_id (&session_record, x11_display_name);
+ /* Don't, leave it as all \0s */
+        /* record_set_id (&session_record, x11_display_name); */
         record_set_host (&session_record, x11_display_name, host_name);
-        record_set_line (&session_record, display_device, x11_display_name);
+        record_set_line (&session_record, display_device, x11_display_name,
+ host_name);
 
         /* Handle wtmp */
         g_debug ("Writing wtmp session record to " GDM_NEW_SESSION_RECORDS_FILE);
@@ -286,9 +307,11 @@ gdm_session_record_logout (GPid                  session_pid,
         record_set_timestamp (&session_record);
         record_set_pid (&session_record, session_pid);
         /* Set ut_id to the $DISPLAY value */
-        record_set_id (&session_record, x11_display_name);
+ /* Don't, leave it as all \0s */
+        /* record_set_id (&session_record, x11_display_name); */
         record_set_host (&session_record, x11_display_name, host_name);
-        record_set_line (&session_record, display_device, x11_display_name);
+        record_set_line (&session_record, display_device, x11_display_name,
+ host_name);
 
 
         /* Handle wtmp */
@@ -306,7 +329,7 @@ gdm_session_record_logout (GPid                  session_pid,
         setutxent ();
 
         while ((u = getutxent ()) != NULL &&
-               (u = getutxid (&session_record)) != NULL) {
+               (u = getutxline (&session_record)) != NULL) {
 
                 g_debug ("Removing utmp record");
                 if (u->ut_pid == session_pid &&

---

_______________________________________________
gdm-list mailing list
gdm-list@...
http://mail.gnome.org/mailman/listinfo/gdm-list