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

[Colin Watson]

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.

Index: debian/changelog
===================================================================
--- debian/changelog	(revision 3748)
+++ debian/changelog	(working copy)
@@ -1,3 +1,12 @@
+shadow (1:4.1.5.1-1.1) UNRELEASED; urgency=low
+
+  * debian/patches/496_su_kill_process_group: Kill the child process group,
+    rather than just the immediate child; this is needed now that su no
+    longer starts a controlling terminal when not running an interactive
+    shell (closes: #713979).
+
+ -- Colin Watson <cjwatson at ubuntu.com>  Fri, 26 Jul 2013 16:55:52 +0100
+
 shadow (1:4.1.5.1-1) unstable; urgency=low
 
   * The "Gruyère" release.
Index: debian/patches/496_su_kill_process_group
===================================================================
--- debian/patches/496_su_kill_process_group	(revision 0)
+++ debian/patches/496_su_kill_process_group	(working copy)
@@ -0,0 +1,29 @@
+Description: su: Kill the child process group, not just the immediate child
+ This is needed now that su no longer starts a controlling terminal when not
+ running an interactive shell.
+Author: Colin Watson <cjwatson at ubuntu.com>
+Forwarded: no
+Last-Update: 2013-07-26
+
+Index: b/src/su.c
+===================================================================
+--- 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);
+ 		(void) fputs (_(" ...killed.\n"), stderr);
+ 	} else {
+ 		(void) fputs (_(" ...waiting for child to terminate.\n"),
+@@ -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);
+ 	}
+ 
+ 	ret = pam_close_session (pamh, 0);
Index: debian/patches/series
===================================================================
--- debian/patches/series	(revision 3748)
+++ debian/patches/series	(working copy)
@@ -16,3 +16,4 @@
 523_su_arguments_are_no_more_concatenated_by_default
 508_nologin_in_usr_sbin
 505_useradd_recommend_adduser
+496_su_kill_process_group

Thanks,

-- 
Colin Watson                                       [cjwatson at ubuntu.com]