[Pkg-shadow-devel] Bug#713979: su: kill child process group on signal, not just immediate child

[Nicolas François]

Hello Colin,

I'm struggling a bit with this issue.

On Fri, Jul 26, 2013 at 05:37:37PM +0100, cjwatson at ubuntu.com wrote:
> Control: tag -1 patch
> 
> On Mon, Jun 24, 2013 at 03:30:22PM +0100, Colin Watson wrote:
> > For some time I've noticed that, when an Ubuntu build times out (150
> > minutes with no output), sbuild tries to terminate it, and I see a
> > "Session terminated, terminating shell... ...terminated." message in the
> > log (which is from su), but the build does not actually terminate
> > properly.  Now, in both Debian and Ubuntu, sbuild invokes builds using
> > something like this simplified command:
> > 
> >   sudo chroot $chroot su $username -s sh -c "cd $dir && exec dpkg-buildpackage"
> > 
> > When su receives a signal, it passes it on to its child process (it has
> > to go to unusual lengths here because it starts new sessions).  However,
> > it only kills its immediate child, not the associated process group.
> [...]
> > Could su please kill the process group associated with its immediate
> > child process instead?  This should just be a matter of negating the pid
> > passed to kill.  If it did that, then I think it would do a much better
> > job of cleaning up after itself.
> 
> I think that this started to become a problem in 4.1.5 when su stopped
> letting its child process have a controlling terminal, so the whole
> child process group doesn't automatically get the signal.  My proposed
> approach still seems reasonable.  Here's a suggested patch.

I would like to document it in the code, but I don't understand how this
is related to having a controlling terminal.

When there is a controlling terminal, I'm not sure how this really works. It
seems to me that the terminal, after receiving a Ctrl-C Ctrl-Z, Ctrl-\
sends the corresponding signal to the foreground process (?).
So in the example you gave, sleep receives the signal, not sh.
I don't think I can mimic this behavior.

But I also expect that in your use case, sbuild sends some SIGTERM.
I don't think there were changes regarding those signals.

>  $ pgrep sleep
>  $ su cjwatson -c 'sh -c "sleep 1h"'
>  Password:
>  [wait a few seconds]
>  ^C
>  Session terminated, terminating shell...Sessions still open, not unmounting
>   ...killed.
>  $ pgrep sleep
>  32421

When there is only one command, bash seems to exec the command instead
of forking, so su receives a SIGINT, forwards it to bash, which has became
sleep in the mean time. So sleep terminates.

With dash, su receives a SIGINT, forwards it to dash, which does nothing with
it. After 2 seconds, su kills dash, and the sleep gets detached.

With bash, if 2 commands are executed (sh -c "sleep 1h;echo"), the same
behavior as with dash is achieved.

If instead of a Ctrl-C, you send a SIGTERM, the behavior is a bit different,
because dash / bash do not ignore the SIGTERM. So they terminate
themselves, but sleep remains untouched (execpt in the first case).

Shells do not forward such signals. This might be a bug, or it just mean
that shell users have different expectation than su users.


> +--- a/src/su.c
> ++++ b/src/su.c
> +@@ -194,7 +194,7 @@
> + static RETSIGTYPE kill_child (int unused(s))
> + {
> + 	if (0 != pid_child) {
> +-		(void) kill (pid_child, SIGKILL);
> ++		(void) kill (-pid_child, SIGKILL);

That part could be OK for me.
I.e. we kill more, but that should not be a problem. When su is
terminated, I can understand that users expect that nothing remains 
executed by the switched to user in this session. Killing the process
group goes in this direction.
Killing even more (the session) might be better, but I don't know how to do
this easily.

Note that this will not work
 * For processes detached by the child (child has to do the cleanup after
   receiving the previous (SIGTERM/SIGINT/SIGQUIT) signal)
 * If the child created new process groups
 * If SIGKILL sent to su (signal will not be catched)


Also, kill_child is executed due to this alarm:
	if (0 != caught) {
		(void) signal (SIGALRM, kill_child);
		(void) alarm (2);

		(void) wait (&status);
		(void) fputs (_(" ...terminated.\n"), stderr);
	}

	exit ((0 != WIFEXITED (status)) ? WEXITSTATUS (status)
	                                : WTERMSIG (status) + 128);

It could occur that the child gets terminated early, which would lead to a
termination of su before the alarm is triggered (hence, no termination of the process group).

su should waitpid (0, &status, options) until it returns ECHILD.
(not tested)

> +@@ -383,7 +383,7 @@
> + 		(void) fputs ("\n", stderr);
> + 		(void) fputs (_("Session terminated, terminating shell..."),
> + 		              stderr);
> +-		(void) kill (pid_child, caught);
> ++		(void) kill (-pid_child, caught);

This block forwards the signal received by su to its child.
This intends to give a last chance for the child to do some cleanups.

I think we should give a chance to the process to do a clean handling of
the signal (e.g. the processes it started may need to be handled in a
given order), and not bypass the child.

I would propose not to apply this change.
Would the first change be sufficient for you?
(and replacing the wait with a "while waitpid does not raise an ECHILD")
If more cleanup are needed, then it might be just a sbuild use case that
sbuild has to handle itself (e.g. walking through all processes in a
session).


I would propose also to document the signal handling in the su man page
(for the PAM enabled version of su):

SIGNAL HANDLING
	In interactive mode (no -c), SIGTERM signals are forwarded to the
	executed shell. After 2 seconds, su kills the process group of the
	shell.
	When -c is used, the SIGTERM, SIGINT, SIGQUIT and SIGTSTP signals
	received by su are similarly forwarded to the executed command.

	Depending how processes are started by the executed command or shell,
	they may still be running even after termination of su.

Best Regards,
-- 
Nekral