Merge pull request #85 from chriskuehl/allow-rewrite-tty-signals

Use new signal rewriting for TTY signal special cases

README.md

@@ -116,11 +116,20 @@ different stop signal in order to do graceful cleanup.
 For example, to rewrite the signal SIGTERM (number 15) to SIGQUIT (number 3),
 just add `--rewrite 15:3` on the command line.
 
-One caveat with this feature: for job control signals (SIGTSTP, SIGTTIN,
-SIGTTOU), dumb-init will always suspend itself after receiving the signal, even
-if you rewrite it to something else. Additionally, if in setsid mode, dumb-init
-will always forward SIGSTOP instead, since the original signals have no effect
-unless the child has handlers for them.
+
+#### Signal rewriting special case
+
+When running in setsid mode, it is not sufficient to forward
+`SIGTSTP`/`SIGTTIN`/`SIGTTOU` in most cases, since if the process has not added
+a custom signal handler for these signals, then the kernel will not apply
+default signal handling behavior (which would be suspending the process) since
+it is a member of an orphaned process group. For this reason, we set default
+rewrites to `SIGSTOP` from those three signals. You can opt out of this
+behavior by rewriting the signals back to their original values, if desired.
+
+One caveat with this feature: for job control signals (`SIGTSTP`, `SIGTTIN`,
+`SIGTTOU`), dumb-init will always suspend itself after receiving the signal,
+even if you rewrite it to something else.
 
 
 ## Installing inside Docker containers

dumb-init.c

@@ -75,19 +75,6 @@ void forward_signal(int signum) {
  * The libc manual is useful:
  * https://www.gnu.org/software/libc/manual/html_node/Job-Control-Signals.html
  *
- * When running in setsid mode, however, it is not sufficient to forward
- * SIGTSTP/SIGTTIN/SIGTTOU in most cases. If the process has not added a custom
- * signal handler for these signals, then the kernel will not apply default
- * signal handling behavior (which would be suspending the process) since it is
- * a member of an orphaned process group.
- *
- * Sadly this doesn't appear to be well documented except in the kernel itself:
- * https://github.com/torvalds/linux/blob/v4.2/kernel/signal.c#L2296-L2299
- *
- * Forwarding SIGSTOP instead is effective, though not ideal; unlike SIGTSTP,
- * SIGSTOP cannot be caught, and so it doesn't allow processes a change to
- * clean up before suspending. In non-setsid mode, we proxy the original signal
- * instead of SIGSTOP for this reason.
 */
 void handle_signal(int signum) {
     DEBUG("Received signal %d.\n", signum);
@@ -110,23 +97,12 @@ void handle_signal(int signum) {
                 exit(exit_status);
             }
         }
-    } else if (
-        signum == SIGTSTP || // tty: background yourself
-        signum == SIGTTIN || // tty: stop reading
-        signum == SIGTTOU    // tty: stop writing
-    ) {
-        if (use_setsid) {
-            DEBUG("Running in setsid mode, so forwarding SIGSTOP instead.\n");
-            forward_signal(SIGSTOP);
-        } else {
-            DEBUG("Not running in setsid mode, so forwarding the original signal (%d).\n", signum);
-            forward_signal(signum);
-        }
-
-        DEBUG("Suspending self due to TTY signal.\n");
-        kill(getpid(), SIGSTOP);
     } else {
         forward_signal(signum);
+        if (signum == SIGTSTP || signum == SIGTTOU || signum == SIGTTIN) {
+            DEBUG("Suspending self due to TTY signal.\n");
+            kill(getpid(), SIGSTOP);
+        }
     }
 }
 
@@ -178,6 +154,11 @@ void parse_rewrite_signum(char *arg) {
     }
 }
 
+void set_rewrite_to_sigstop_if_not_defined(int signum) {
+    if (signal_rewrite[signum] == 0)
+        signal_rewrite[signum] = SIGSTOP;
+}
+
 char **parse_command(int argc, char *argv[]) {
     int opt;
     struct option long_options[] = {
@@ -231,6 +212,12 @@ char **parse_command(int argc, char *argv[]) {
         DEBUG("Not running in setsid mode.\n");
     }
 
+    if (use_setsid) {
+        set_rewrite_to_sigstop_if_not_defined(SIGTSTP);
+        set_rewrite_to_sigstop_if_not_defined(SIGTTOU);
+        set_rewrite_to_sigstop_if_not_defined(SIGTTIN);
+    }
+
     return &argv[optind];
 }
 

tests/proxies_signals_test.py

@@ -11,6 +11,7 @@
 
 from tests.lib.testing import NORMAL_SIGNALS
 from tests.lib.testing import pid_tree
+from tests.lib.testing import process_state
 
 
 @contextmanager
@@ -70,6 +71,7 @@ def _rewrite_map_to_args(rewrite_map):
         [signal.SIGINT, signal.SIGQUIT, signal.SIGCONT, signal.SIGTERM],
     ),
 
+    # Lowest possible and highest possible signals.
     (
         {1: 31, 31: 1},
         [1, 31],
@@ -83,3 +85,27 @@ def test_proxies_signals_with_rewrite(rewrite_map, sequence, expected):
         for send, expect_receive in zip(sequence, expected):
             proc.send_signal(send)
             assert proc.stdout.readline() == '{0}\n'.format(expect_receive).encode('ascii')
+
+
+@pytest.mark.usefixtures('both_debug_modes', 'setsid_enabled')
+def test_default_rewrites_can_be_overriden_with_setsid_enabled():
+    """In setsid mode, dumb-init should allow overwriting the default
+    rewrites (but still suspend itself).
+    """
+    rewrite_map = {
+        signal.SIGTTIN: signal.SIGTERM,
+        signal.SIGTTOU: signal.SIGINT,
+        signal.SIGTSTP: signal.SIGHUP,
+    }
+    with _print_signals(_rewrite_map_to_args(rewrite_map)) as proc:
+        for send, expect_receive in rewrite_map.items():
+            assert process_state(proc.pid) in ['running', 'sleeping']
+            proc.send_signal(send)
+
+            assert proc.stdout.readline() == '{0}\n'.format(expect_receive).encode('ascii')
+            os.waitpid(proc.pid, os.WUNTRACED)
+            assert process_state(proc.pid) == 'stopped'
+
+            proc.send_signal(signal.SIGCONT)
+            assert proc.stdout.readline() == '{0}\n'.format(signal.SIGCONT).encode('ascii')
+            assert process_state(proc.pid) in ['running', 'sleeping']