[buildd-tools-devel] Bug#477770: [PATCH] improve SIGINT handling

[Nathaniel W. Turner]

From: Nathaniel W. Turner <nate at houseofnate.net>

This addresses the case where a Ctrl-C is intended for a process running
in the chroot; before, schroot would always exit without cleaning up
(the most visible symptom being left-over bind-mounts in
/var/lib/schroot/mount).  Now, cleanup occurs, and an appropriate error
message is displayed ("E: Child terminated by signal ‘Interrupt’").

This patch does not cover all the cases described by bug 477770, but I
think it is a step in the right direction.

Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=477770
Signed-off-by: Nathaniel W. Turner <nate at houseofnate.net>
---
 sbuild/sbuild-session.cc |   52 +++++++++++++++++++++++++++++++++++++++++++++-
 sbuild/sbuild-session.h  |   16 ++++++++++++++
 2 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/sbuild/sbuild-session.cc b/sbuild/sbuild-session.cc
index 9d83ca1..685a0bd 100644
--- a/sbuild/sbuild-session.cc
+++ b/sbuild/sbuild-session.cc
@@ -184,6 +184,7 @@ namespace
   }
 
   volatile bool sighup_called = false;
+  volatile bool sigint_called = false;
   volatile bool sigterm_called = false;
 
   /**
@@ -199,6 +200,29 @@ namespace
   }
 
   /**
+   * Handle the SIGINT signal.
+   *
+   * @param ignore the signal number.
+   *
+   * We explicitly do nothing with SIGINT, and rely on the exit status of child
+   * processes to determine if we should care.  We want to make sure any child
+   * process (which will also have received SIGINT) has exited before we do
+   * anything, and some child processes (for example, emacs) may expect SIGINT
+   * during normal operation.  See http://www.cons.org/cracauer/sigint.html for
+   * a good discussion of SIGINT handling.
+   */
+  void
+  sigint_handler (int ignore)
+  {
+    /*
+     * Allows us to detect if an interrupted waitpid() was interrupted due to
+     * SIGINT or something else.  We may also want to use this at exit time to
+     * see if we should re-kill ourselves with SIGINT.
+     */
+    sigint_called = true;
+  }
+
+  /**
    * Handle the SIGTERM signal.
    *
    * @param ignore the signal number.
@@ -242,6 +266,7 @@ session::session (std::string const&         service,
   session_id(),
   force(false),
   saved_sighup_signal(),
+  saved_sigint_signal(),
   saved_sigterm_signal(),
   saved_termios(),
   termios_ok(false),
@@ -571,6 +596,8 @@ session::run_impl ()
     {
       sighup_called = false;
       set_sighup_handler();
+      sigint_called = false;
+      set_sigint_handler();
       sigterm_called = false;
       set_sigterm_handler();
 
@@ -695,6 +722,7 @@ session::run_impl ()
   catch (error const& e)
     {
       clear_sigterm_handler();
+      clear_sigint_handler();
       clear_sighup_handler();
 
       /* If a command was not run, but something failed, the exit
@@ -705,6 +733,7 @@ session::run_impl ()
     }
 
   clear_sigterm_handler();
+  clear_sigint_handler();
   clear_sighup_handler();
 }
 
@@ -1268,6 +1297,14 @@ session::wait_for_child (pid_t pid,
 
   while (1)
     {
+      /*
+       * If we (the parent process) gets SIGHUP or SIGTERM, pass this signal on
+       * to the child (once).  Note that we do not handle SIGINT this way,
+       * because when a user presses Ctrl-C, the SIGINT is sent to all
+       * processes attached to that TTY (so the child will already have gotten
+       * it).  In any case, once the child gets the signal, we just have to
+       * continue waiting for it to exit.
+       */
       if ((sighup_called || sigterm_called) && !child_killed)
 	{
 	  if (sighup_called)
@@ -1290,7 +1327,7 @@ session::wait_for_child (pid_t pid,
 
       if (waitpid(pid, &status, 0) == -1)
 	{
-	  if (errno == EINTR && (sighup_called || sigterm_called))
+	  if (errno == EINTR && (sighup_called || sigterm_called || sigint_called))
 	    continue; // Kill child and wait again.
 	  else
 	    throw error(CHILD_WAIT, strerror(errno));
@@ -1305,6 +1342,7 @@ session::wait_for_child (pid_t pid,
 	  sigterm_called = false;
 	  throw error(SIGNAL_CATCH, strsignal(SIGTERM));
 	}
+      /* no need to handle the SIGINT case here; it is handled correctly below */
       else
 	break;
     }
@@ -1372,6 +1410,18 @@ session::clear_sighup_handler ()
 }
 
 void
+session::set_sigint_handler ()
+{
+  set_signal_handler(SIGINT, &this->saved_sigint_signal, sigint_handler);
+}
+
+void
+session::clear_sigint_handler ()
+{
+  clear_signal_handler(SIGINT, &this->saved_sigint_signal);
+}
+
+void
 session::set_sigterm_handler ()
 {
   set_signal_handler(SIGTERM, &this->saved_sigterm_signal, sigterm_handler);
diff --git a/sbuild/sbuild-session.h b/sbuild/sbuild-session.h
index 888fbfe..9179a29 100644
--- a/sbuild/sbuild-session.h
+++ b/sbuild/sbuild-session.h
@@ -417,6 +417,20 @@ namespace sbuild
     clear_sighup_handler ();
 
     /**
+     * Set the SIGINT handler.
+     *
+     * An error will be thrown on failure.
+     */
+    void
+    set_sigint_handler ();
+
+    /**
+     * Restore the state of SIGINT prior to setting the handler.
+     */
+    void
+    clear_sigint_handler ();
+
+    /**
      * Set the SIGTERM handler.
      *
      * An error will be thrown on failure.
@@ -474,6 +488,8 @@ namespace sbuild
     bool             force;
     /// Signal saved while sighup handler is set.
     struct sigaction saved_sighup_signal;
+    /// Signal saved while sigint handler is set.
+    struct sigaction saved_sigint_signal;
     /// Signal saved while sigterm handler is set.
     struct sigaction saved_sigterm_signal;
     /// Saved terminal settings.
-- 
1.7.0.4