Bug#690197: gdm3: fails to correctly add XDMCP user's session to utmp
Paul Szabo
paul.szabo at sydney.edu.au
Sun Oct 14 23:32:26 UTC 2012
I have now made a better patch, tested that it works for me, and
submitted it upstream at
https://bugzilla.gnome.org/show_bug.cgi?id=599103#c5
Patch attached here also.
Cheers, Paul
Paul Szabo psz at maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/
School of Mathematics and Statistics University of Sydney Australia
-------------- next part --------------
--- a/daemon/gdm-session-record.c 2012-09-07 04:33:20.000000000 +1000
+++ b/daemon/gdm-session-record.c 2012-10-15 08:37:45.000000000 +1100
@@ -124,39 +124,71 @@
}
g_debug ("using ut_pid %d", (int) u->ut_pid);
#endif
}
static void
record_set_id (UTMP *u,
const char *id)
{
#if defined(HAVE_UT_UT_ID)
+/* PSz 11 Oct 12
+ * Seems this record_set_id() is only ever called to set ID to $DISPLAY.
+ *
+ * For console logins, $DISPLAY may be ":0" and that might be a sensible ID.
+ * But for remote XDMCP clients, $DISPLAY is "IP.of.cli.ent:0" and that does
+ * not go well into a 4-byte ut_id, is likely to cause all clients to use
+ * the same ut_id of "192."
+ *
+ * It may be sensible to simply leave ut_id="" as then pututxline() and
+ * getutxid() will use ut_line to match, see sysdeps/generic/utmp-equal.h
+ * in libc sources. We do such similar matching ourselves, ignoring ut_id,
+ * when "Updating existing utmp record".
+ *
+ * Another idea might be to encode session_pid or session_id into ut_id,
+ * say somewhat similarly to Samba ut_id_encode() in source/smbd/utmp.c .
+ * But PID is not good, too large a range: each time would leave a new
+ * entry in utmp which would grow large, slowly filling with dead entries.
+ * We would need some ID that is reused; I do not know whether session_id
+ * would be suitable.
+ * Anyway I am lazy and go for the simple "leave blank".
+ *
+ * Based on previous patch for older version, see:
+ * http://bugs.debian.org/551802
+ * https://bugzilla.gnome.org/show_bug.cgi?id=599103
+ */
+ return;
+/* No need for return above, since record_set_id() is never called */
strncpy (u->ut_id, id, sizeof (u->ut_id));
g_debug ("using ut_id %.*s", (int) sizeof (u->ut_id), u->ut_id);
#endif
}
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 = NULL;
/*
* Set ut_host to hostname:$DISPLAY if remote, otherwise set
* to $DISPLAY
*/
+/* PSz 15 Oct 12
+ * Seems that for remote XDMCP logins, we get x11_display_name set to
+ * 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.
+ */
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);
}
if (hostname != NULL) {
strncpy (u->ut_host, hostname, sizeof (u->ut_host));
@@ -183,20 +215,29 @@
if (display_device != NULL
&& g_str_has_prefix (display_device, "/dev/")) {
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 if (x11_display_name != NULL) {
+/* PSz 15 Oct 12
+ * Seems that for remote XDMCP logins, we get x11_display_name set to
+ * 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.)
+ */
+ strncpy (u->ut_line,
+ x11_display_name,
+ sizeof (u->ut_line));
}
g_debug ("using ut_line %.*s", (int) sizeof (u->ut_line), u->ut_line);
}
void
gdm_session_record_login (GPid session_pid,
const char *user_name,
const char *host_name,
const char *x11_display_name,
@@ -209,23 +250,23 @@
g_debug ("Writing login record");
#if defined(HAVE_UT_UT_TYPE)
session_record.ut_type = USER_PROCESS;
g_debug ("using ut_type USER_PROCESS");
#endif
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);
+ /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */
+/* 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);
/* Handle wtmp */
g_debug ("Writing wtmp session record to " GDM_NEW_SESSION_RECORDS_FILE);
#if defined(HAVE_UPDWTMPX)
updwtmpx (GDM_NEW_SESSION_RECORDS_FILE, &session_record);
#elif defined(HAVE_UPDWTMP)
updwtmp (GDM_NEW_SESSION_RECORDS_FILE, &session_record);
#elif defined(HAVE_LOGWTMP) && defined(HAVE_UT_UT_HOST)
@@ -234,20 +275,29 @@
#elif defined(HAVE_UT_UT_NAME)
logwtmp (session_record.ut_line, session_record.ut_name, session_record.ut_host);
#endif
#endif
/*
* Handle utmp
* Update if entry already exists
*/
#if defined(HAVE_GETUTXENT)
+/* PSz 14 Oct 12
+ * Either way we do pututxline() which finds its "proper place" by itself.
+ * Do we really want to go to all this trouble, just to be be able to give
+ * different debug messages (which may be wrong anyway)?
+ * I suggest to do a simple
+ * g_debug ("Adding or updating utmp record for login");
+ * pututxline (&session_record);
+ * here.
+ */
setutxent ();
while ((u = getutxent ()) != NULL) {
if (u->ut_type == USER_PROCESS &&
(session_record.ut_line != NULL &&
(strncmp (u->ut_line, session_record.ut_line,
sizeof (u->ut_line)) == 0 ||
u->ut_pid == session_record.ut_pid))) {
g_debug ("Updating existing utmp record");
pututxline (&session_record);
@@ -279,37 +329,46 @@
g_debug ("Writing logout record");
#if defined(HAVE_UT_UT_TYPE)
session_record.ut_type = DEAD_PROCESS;
g_debug ("using ut_type DEAD_PROCESS");
#endif
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);
+ /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */
+/* 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);
/* Handle wtmp */
g_debug ("Writing wtmp logout record to " GDM_NEW_SESSION_RECORDS_FILE);
#if defined(HAVE_UPDWTMPX)
updwtmpx (GDM_NEW_SESSION_RECORDS_FILE, &session_record);
#elif defined (HAVE_UPDWTMP)
updwtmp (GDM_NEW_SESSION_RECORDS_FILE, &session_record);
#elif defined(HAVE_LOGWTMP)
logwtmp (session_record.ut_line, "", "");
#endif
/* Handle utmp */
#if defined(HAVE_GETUTXENT)
+/* PSz 14 Oct 12
+ * Do we really care whether we are about to destroy a "still alive" entry
+ * (which should be there), or maybe add an un-needed logout one?
+ * I suggest to do a simple
+ * g_debug ("Adding or updating utmp record for logout");
+ * pututxline (&session_record);
+ * here.
+ */
setutxent ();
while ((u = getutxent ()) != NULL &&
(u = getutxid (&session_record)) != NULL) {
g_debug ("Removing utmp record");
if (u->ut_pid == session_pid &&
u->ut_type == DEAD_PROCESS) {
/* Already done */
break;
@@ -352,21 +411,22 @@
g_debug ("Writing failed session attempt record");
#if defined(HAVE_UT_UT_TYPE)
session_record.ut_type = USER_PROCESS;
g_debug ("using ut_type USER_PROCESS");
#endif
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);
+ /* PSz 11 Oct 12 Do not set ut_id to $DISPLAY, but leave blank */
+/* 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);
/* Handle btmp */
g_debug ("Writing btmp failed session attempt record to "
GDM_BAD_SESSION_RECORDS_FILE);
#if defined(HAVE_UPDWTMPX)
updwtmpx (GDM_BAD_SESSION_RECORDS_FILE, &session_record);
#elif defined(HAVE_UPDWTMP)
More information about the pkg-gnome-maintainers
mailing list