[gdm-list] Re: PostSession script support fades out from 2.4 to 2.6?

[Brian Cameron <Brian Cameron Sun COM>]

Jerry/George:

Thanks for looking into this and for proposing a solution to the problem.
It seems that this problem was introduced when George fixed bug
#126071 by adding the call to whack clients in session stop, causing a
race condition.  Bug #126071 is regarding an issue that GDM was not always
unmounting the user's home directory because programs like xscreensaver
retain open file descripters within $HOME when GDM calls pam_close_session.
The bug reporter proposed fixing the problem by moving the call to
pam_close_session until after the Xserver is reset so all user programs
will have closed any open file descriptors by the time pam_close_session
is called.

Doesn't it seem better to backout the fix to bug 126071 and fix the problem
more simply by moving the pam_close_session call rather than having this
complicated race condition?  Furthermore, George highlighted that this
could/should perhaps better be fixed in the PAM module code by doing
an async unmount.  It may be better to fix this in PAM rather than hacking
up GDM with nasty logic to deal with race conditions.  The person who
submitted bug 126071 suggested a patch that looks like a much cleaner
solution to the problem.  I'm not sure why George rejected it.  Perhaps
he might remember and could tell us.  I'd appreciate some other eyes
looking at the suggested patch and give feedback if doing something like
this would be a better way to fix 126071.

Jerry, could you check to see if you backout George's fix for bug
#126071 and see if that also fixes the problem.  Not running PostSession
on logout seems much worse than sometimes not unmounting home directories.

Brian

> > Spent the past few days dusting off my last experience with setjmp (about 2
> > decades ago), adding gdm_debug calls in various places, and tweaking code.
> > Found the following.  These are based upon the RHEL4 source, which is a
> > patched up version of 2.6.0.5.
> >
> > 1) There's a 'session_started = FALSE' at the beginning of
> > gdm_slave_session_stop.  When I started adding debug code it became
> > apparent that this was the line that was turning off session_started for
> > the longjmp target code.  I'm not sure it should even be there.
> >
> > 2) You never get to the postsession script because:  The call to
> > gdm_server_whack_clients in gdm_slave_session_stop will trigger an
> > xioerror.
> >
> > The user has crashed X with ctl-alt-backspace,  and whack_clients tries to
> > access the X server.  The resulting xioerror event then vectors you via the
> > xioerror handler to the quick exit code.  That is, more than one crash
> > handler gets called and the second one terminates the correct execution of
> > the original exit handler.  The problem code has comments that say "fix
> > #126071".
> >
> > I was able to get back to the "old, correct" behavior by commenting the
> > code line in (1) and, later, after I understood the handler behavior,
> > adding another setjmp that caused the xioerror to be ignored during the
> > call to gdm_server_whack_clients.
> >
> > I still need to check whether the added setjmp code means that item 1 is
> > not really an issue, and to create a patch that doesn't have the fourteen
> > or so additional gdm_debug and printf patches I created.  I'll post the
> > patch for further discussion when I've done that, probably tomorrow.
> >
> > J
>
>
> Item 1 turned out to not matter once I implemented the fix to item 2.  
> Attached for discussion is a patch against 2.6.0.8 source as modified by 
> FC4's rpm.
>
> J
>
>
> ------------------------------------------------------------------------
>
> --- gdm-2.6.0.8.orig/daemon/slave.c	2005-08-19 13:32:00.000000000 -0600
> +++ gdm-2.6.0.8/daemon/slave.c	2005-08-19 13:48:43.000000000 -0600
> @@ -195,6 +195,7 @@
>  /* Local prototypes */
>  static gint     gdm_slave_xerror_handler (Display *disp, XErrorEvent *evt);
>  static gint     gdm_slave_xioerror_handler (Display *disp);
> +static gint     gdm_slave_ignore_xioerror_handler (Display *disp);
>  static void	gdm_slave_run (GdmDisplay *display);
>  static void	gdm_slave_wait_for_login (void);
>  static void     gdm_slave_greeter (void);
> @@ -231,6 +232,8 @@
>  
>  /* for signals that want to exit */
>  static Jmp_buf slave_start_jmp;
> +/* for handling xioerror during ctl-alt-bs */
> +static Jmp_buf ignore_xioerror_jmp;
>  static gboolean return_to_slave_start_jmp = FALSE;
>  static gboolean already_in_slave_start_jmp = FALSE;
>  static char *slave_start_jmp_error_to_print = NULL;
> @@ -4388,8 +4391,27 @@
>         This is to fix #126071, that is kill processes that may still hold open
>         fd's in the home directory to allow a clean unmount.  However note of course
>         that this is a race. */
> +    /* FIX HACK: We will get an xioerror out of whack clients if the user crashed X with ctl-alt-backspace.
> +       So, we need to ignore xioerror while we do this. Otherwise, we end up in a longjmp to quick_exit
> +       which will prevent things like PostSession scripts from processing */
> +    switch (Setjmp (ignore_xioerror_jmp)) {
> +    case 0:
> +       /* Having just set the jump point, activate the error handler that returns us to the next case */
> +       XSetIOErrorHandler (gdm_slave_ignore_xioerror_handler);
> +       break;
> +    default:
> +       /* Here means we saw an xioerror and ignored it. */
> +       /* xioerror will cause this to drop back into whack_clients, but I think that's OK
> +          because I haven't seen it do so more than once */
> +       gdm_debug("gdm_slave_session_stop: back here from xioerror");
> +       break;
> +    };
> +
>      gdm_server_whack_clients (d);
>  
> +    /* Now we should care about xioerror once again??? */
> +    XSetIOErrorHandler (gdm_slave_xioerror_handler);
> +
>  #if defined(_POSIX_PRIORITY_SCHEDULING) && defined(HAVE_SCHED_YIELD)
>      /* let the other processes die perhaps or whatnot */
>      sched_yield ();
> @@ -4830,7 +4852,15 @@
>      return (0);
>  }
>  
> -/* We respond to fatal errors by restarting the display */
> +/* Ignore fatal X errors when user did ctl-alt-backspace */
> +static gint
> +gdm_slave_ignore_xioerror_handler (Display *disp)
> +{
> +    gdm_debug ("Fatal X error detected.  Ignoring same during session shutdown.");
> +    Longjmp(ignore_xioerror_jmp, 1);
> +}
> +
> +/* We usually respond to fatal errors by restarting the display */
>  static gint
>  gdm_slave_xioerror_handler (Display *disp)
>  {