Re: [gdm-list] Old sessreg, gdm utmpwtmp and utmp->ut_id

[Maximiliano Curia <maxy gnuservers com ar>]

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 gnuservers com ar>
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 gnuservers com ar>
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 gnuservers com ar>
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 &&