Merge pull request #21 from krallin/fix-19-pass-tty

Pass tty to child process group and release 0.8.2

.travis.yml

@@ -34,8 +34,8 @@ deploy:
   file:
     - "./dist/tini"
     - "./dist/tini-static"
-    - "./dist/tini_0.8.0.deb"
-    - "./dist/tini_0.8.0.rpm"
+    - "./dist/tini_0.8.2.deb"
+    - "./dist/tini_0.8.2.rpm"
   on:
     repo: krallin/tini
     tags: true

CMakeLists.txt

@@ -4,7 +4,7 @@ project (tini C)
 # Config
 set (tini_VERSION_MAJOR 0)
 set (tini_VERSION_MINOR 8)
-set (tini_VERSION_PATCH 0)
+set (tini_VERSION_PATCH 2)
 
 # Extract git version and dirty-ness
 execute_process (
@@ -31,7 +31,7 @@ endif()
 
 # Flags
 add_definitions (-D_FORTIFY_SOURCE=2)
-set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=gnu99 -Wextra -Wall -pedantic -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security")
+set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=gnu99 -Werror -Wextra -Wall -pedantic-errors -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat")
 set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-s")
 
 # Build

Dockerfile

@@ -5,4 +5,4 @@ RUN apt-get update \
  && rm -rf /var/lib/apt/lists/*
 
 # Pre-install those here for faster local builds.
-RUN CFLAGS="-DPR_SET_CHILD_SUBREAPER=36 -DPR_GET_CHILD_SUBREAPER=37" pip install psutil python-prctl
+RUN CFLAGS="-DPR_SET_CHILD_SUBREAPER=36 -DPR_GET_CHILD_SUBREAPER=37" pip install psutil python-prctl bitmap

README.md

@@ -37,7 +37,7 @@ In Docker, you will want to use an entrypoint so you don't have to remember
 to manually invoke Tini:
 
     # Add Tini
-    ENV TINI_VERSION v0.8.0
+    ENV TINI_VERSION v0.8.2
     ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
     RUN chmod +x /tini
     ENTRYPOINT ["/tini", "--"]

ci/run_build.sh

@@ -3,11 +3,15 @@
 set -o errexit
 set -o nounset
 
+# Default compiler
+: ${CC:="gcc"}
+
 # Paths
 : ${SOURCE_DIR:="."}
 : ${DIST_DIR:="${SOURCE_DIR}/dist"}
 : ${BUILD_DIR:="/tmp/build"}
 
+
 # Make those paths absolute, and export them for the Python tests to consume.
 export SOURCE_DIR="$(readlink -f "${SOURCE_DIR}")"
 export DIST_DIR="$(readlink -f "${DIST_DIR}")"
@@ -56,6 +60,14 @@ for tini in "${BUILD_DIR}/tini" "${BUILD_DIR}/tini-static"; do
     exit 1
   fi
 
+  # Test stdin / stdout are handed over to child
+  echo "Testing pipe"
+  echo "exit 0" | $tini -vvv sh
+  if [[ ! "$?" -eq "0" ]]; then
+    echo "Pipe test failed"
+    exit 1
+  fi
+
   # Move files to the dist dir for testing
   mkdir -p "${DIST_DIR}"
   cp "${BUILD_DIR}"/tini{,-static,*.rpm,*deb} "${DIST_DIR}"
@@ -72,6 +84,9 @@ for tini in "${BUILD_DIR}/tini" "${BUILD_DIR}/tini-static"; do
   fi
 done
 
+# Compile test code
+"${CC}" -o "${BUILD_DIR}/sigconf-test" "${SOURCE_DIR}/test/sigconf/sigconf-test.c"
+
 # Create virtual environment to run tests.
 # Accept system site packages for faster local builds.
 VENV="${BUILD_DIR}/venv"
@@ -82,7 +97,7 @@ export PATH="${VENV}/bin:${PATH}"
 export CFLAGS  # We need them to build our test suite, regardless of FORCE_SUBREAPER
 
 # Install test dependencies
-pip install psutil python-prctl
+pip install psutil python-prctl bitmap
 
 # Run tests
 python "${SOURCE_DIR}/test/run_inner_tests.py"

ddist.sh

@@ -17,7 +17,7 @@ rm -f "${HERE}/dist"/*
 docker build -t "${IMG}" .
 
 # Run test without subreaper support, don't copy build files here
-docker run --rm \
+docker run -it --rm \
   --volume="${HERE}:${SRC}" \
   -e BUILD_DIR=/tmp/tini-build \
   -e SOURCE_DIR="${SRC}" \

src/tini.c

@@ -16,14 +16,20 @@
 
 #include "tiniConfig.h"
 
-#define PRINT_FATAL(...)    fprintf(stderr, "[FATAL] "); fprintf(stderr, __VA_ARGS__); fprintf(stderr, "\n");
-#define PRINT_WARNING(...)  if (verbosity > 0) { fprintf(stderr, "[WARN ] "); fprintf(stderr, __VA_ARGS__); fprintf(stderr, "\n"); }
-#define PRINT_INFO(...)     if (verbosity > 1) { fprintf(stdout, "[INFO ] "); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
-#define PRINT_DEBUG(...)    if (verbosity > 2) { fprintf(stdout, "[DEBUG] "); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
-#define PRINT_TRACE(...)    if (verbosity > 3) { fprintf(stdout, "[TRACE] "); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
+#define PRINT_FATAL(...)                         fprintf(stderr, "[FATAL tini (%i)] ", getpid()); fprintf(stderr, __VA_ARGS__); fprintf(stderr, "\n");
+#define PRINT_WARNING(...)  if (verbosity > 0) { fprintf(stderr, "[WARN  tini (%i)] ", getpid()); fprintf(stderr, __VA_ARGS__); fprintf(stderr, "\n"); }
+#define PRINT_INFO(...)     if (verbosity > 1) { fprintf(stdout, "[INFO  tini (%i)] ", getpid()); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
+#define PRINT_DEBUG(...)    if (verbosity > 2) { fprintf(stdout, "[DEBUG tini (%i)] ", getpid()); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
+#define PRINT_TRACE(...)    if (verbosity > 3) { fprintf(stdout, "[TRACE tini (%i)] ", getpid()); fprintf(stdout, __VA_ARGS__); fprintf(stdout, "\n"); }
 
 #define ARRAY_LEN(x)  (sizeof(x) / sizeof(x[0]))
 
+typedef struct {
+   sigset_t* const sigmask_ptr;
+   struct sigaction* const sigttin_action_ptr;
+   struct sigaction* const sigttou_action_ptr;
+} signal_configuration_t;
+
 
 #ifdef PR_SET_CHILD_SUBREAPER
 #define HAS_SUBREAPER 1
@@ -55,24 +61,71 @@ static const char reaper_warning[] = "Tini is not running as PID 1 "
 #endif
         "run Tini as PID 1.";
 
+int restore_signals(const signal_configuration_t* const sigconf_ptr) {
+	if (sigprocmask(SIG_SETMASK, sigconf_ptr->sigmask_ptr, NULL)) {
+		PRINT_FATAL("Restoring child signal mask failed: '%s'", strerror(errno));
+		return 1;
+	}
+
+	if (sigaction(SIGTTIN, sigconf_ptr->sigttin_action_ptr, NULL)) {
+		PRINT_FATAL("Restoring SIGTTIN handler failed: '%s'", strerror((errno)));
+		return 1;
+	}
+
+	if (sigaction(SIGTTOU, sigconf_ptr->sigttou_action_ptr, NULL)) {
+		PRINT_FATAL("Restoring SIGTTOU handler failed: '%s'", strerror((errno)));
+		return 1;
+	}
 
-int spawn(const sigset_t* const child_sigset_ptr, char* const argv[], int* const child_pid_ptr) {
+	return 0;
+}
+
+int isolate_child() {
+	// Put the child into a new process group.
+	if (setpgid(0, 0) < 0) {
+		PRINT_FATAL("setpgid failed: '%s'", strerror(errno));
+		return 1;
+	}
+
+	// If there is a tty, allocate it to this new process group. We
+	// can do this in the child process because we're blocking
+	// SIGTTIN / SIGTTOU.
+
+	// Doing it in the child process avoids a race condition scenario
+	// if Tini is calling Tini (in which case the grandparent may make the
+	// parent the foreground process group, and the actual child ends up...
+	// in the background!)
+	if (tcsetpgrp(STDIN_FILENO, getpgrp())) {
+		if (errno == ENOTTY) {
+			PRINT_DEBUG("tcsetpgrp failed: no tty (ok to proceed)")
+		} else {
+			PRINT_FATAL("tcsetpgrp failed: '%s'", strerror(errno));
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+
+int spawn(const signal_configuration_t* const sigconf_ptr, char* const argv[], int* const child_pid_ptr) {
 	pid_t pid;
 
+	// TODO: check if tini was a foreground process to begin with (it's not OK to "steal" the foreground!")
+
 	pid = fork();
 	if (pid < 0) {
 		PRINT_FATAL("Fork failed: '%s'", strerror(errno));
 		return 1;
 	} else if (pid == 0) {
-		// Child
-		if (sigprocmask(SIG_SETMASK, child_sigset_ptr, NULL)) {
-			PRINT_FATAL("Setting child signal mask failed: '%s'", strerror(errno));
+
+		// Put the child in a process group and make it the foreground process if there is a tty.
+		if (isolate_child()) {
 			return 1;
 		}
 
-		// Put the child into a new process group
-		if (setpgid(0, 0) < 0) {
-			PRINT_FATAL("setpgid failed: '%s'", strerror(errno));
+		// Restore all signal handlers to the way they were before we touched them.
+		if (restore_signals(sigconf_ptr)) {
 			return 1;
 		}
 
@@ -207,27 +260,48 @@ void reaper_check () {
 }
 
 
-int prepare_sigmask(sigset_t* const parent_sigset_ptr, sigset_t* const child_sigset_ptr) {
-	/* Prepare signals to block; make sure we don't block program error signals. */
+int configure_signals(sigset_t* const parent_sigset_ptr, const signal_configuration_t* const sigconf_ptr) {
+	/* Block all signals that are meant to be collected by the main loop */
 	if (sigfillset(parent_sigset_ptr)) {
 		PRINT_FATAL("sigfillset failed: '%s'", strerror(errno));
 		return 1;
 	}
 
+	// These ones shouldn't be collected by the main loop
 	uint i;
-	int ignore_signals[] = {SIGFPE, SIGILL, SIGSEGV, SIGBUS, SIGABRT, SIGTRAP, SIGSYS} ;
-	for (i = 0; i < ARRAY_LEN(ignore_signals); i++) {
-		if (sigdelset(parent_sigset_ptr, ignore_signals[i])) {
-			PRINT_FATAL("sigdelset failed: '%i'", ignore_signals[i]);
+	int signals_for_tini[] = {SIGFPE, SIGILL, SIGSEGV, SIGBUS, SIGABRT, SIGTRAP, SIGSYS, SIGTTIN, SIGTTOU};
+	for (i = 0; i < ARRAY_LEN(signals_for_tini); i++) {
+		if (sigdelset(parent_sigset_ptr, signals_for_tini[i])) {
+			PRINT_FATAL("sigdelset failed: '%i'", signals_for_tini[i]);
 			return 1;
 		}
 	}
 
-	if (sigprocmask(SIG_SETMASK, parent_sigset_ptr, child_sigset_ptr)) {
+	if (sigprocmask(SIG_SETMASK, parent_sigset_ptr, sigconf_ptr->sigmask_ptr)) {
 		PRINT_FATAL("sigprocmask failed: '%s'", strerror(errno));
 		return 1;
 	}
 
+	// Handle SIGTTIN and SIGTTOU separately. Since Tini makes the child process group
+	// the foreground process group, there's a chance Tini can end up not controlling the tty.
+	// If TOSTOP is set on the tty, this could block Tini on writing debug messages. We don't
+	// want that. Ignore those signals.
+	struct sigaction ign_action;
+	memset(&ign_action, 0, sizeof ign_action);
+
+	ign_action.sa_handler = SIG_IGN;
+	sigemptyset(&ign_action.sa_mask);
+
+	if (sigaction(SIGTTIN, &ign_action, sigconf_ptr->sigttin_action_ptr)) {
+		PRINT_FATAL("Failed to ignore SIGTTIN");
+		return 1;
+	}
+
+	if (sigaction(SIGTTOU, &ign_action, sigconf_ptr->sigttou_action_ptr)) {
+		PRINT_FATAL("Failed to ignore SIGTTOU");
+		return 1;
+	}
+
 	return 0;
 }
 
@@ -345,10 +419,19 @@ int main(int argc, char *argv[]) {
 		return 1;
 	}
 
-	/* Prepare sigmask */
-	sigset_t parent_sigset;
-	sigset_t child_sigset;
-	if (prepare_sigmask(&parent_sigset, &child_sigset)) {
+	/* Configure signals */
+	sigset_t parent_sigset, child_sigset;
+	struct sigaction sigttin_action, sigttou_action;
+	memset(&sigttin_action, 0, sizeof sigttin_action);
+	memset(&sigttou_action, 0, sizeof sigttou_action);
+
+	signal_configuration_t child_sigconf = {
+		.sigmask_ptr = &child_sigset,
+		.sigttin_action_ptr = &sigttin_action,
+		.sigttou_action_ptr = &sigttou_action,
+	};
+
+	if (configure_signals(&parent_sigset, &child_sigconf)) {
 		return 1;
 	}
 
@@ -363,7 +446,7 @@ int main(int argc, char *argv[]) {
 	reaper_check();
 
 	/* Go on */
-	if (spawn(&child_sigset, *child_args_ptr, &child_pid)) {
+	if (spawn(&child_sigconf, *child_args_ptr, &child_pid)) {
 		return 1;
 	}
 	free(child_args_ptr);

test/run_inner_tests.py

@@ -6,6 +6,11 @@ import signal
 import subprocess
 import time
 import psutil
+import bitmap
+import re
+
+
+SIGNUM_TO_SIGNAME = dict((v, k) for k,v in signal.__dict__.items() if re.match("^SIG[A-Z]+$", k))
 
 
 def busy_wait(condition_callable, timeout):
@@ -55,32 +60,58 @@ def main():
 
 
         # Run the signals test
-        for signame in "SIGINT", "SIGTERM":
-            print "running signal test for: {0} ({1} with env {2})".format(signame, " ".join(target), env)
+        for signum in [signal.SIGINT, signal.SIGTERM]:
+            print "running signal test for: {0} ({1} with env {2})".format(SIGNUM_TO_SIGNAME[signum], " ".join(target), env)
             p = subprocess.Popen(target + [os.path.join(src, "test", "signals", "test.py")], env=dict(os.environ, **env))
-            sig = getattr(signal, signame)
-            p.send_signal(sig)
+            p.send_signal(signum)
             ret = p.wait()
-            assert ret == -sig, "Signals test failed!"
+            assert ret == -signum, "Signals test failed (ret was {0}, expected {1})".format(ret, -signum)
+
 
     # Run the process group test
     # This test has Tini spawn a process that ignores SIGUSR1 and spawns a child that doesn't (and waits on the child)
     # We send SIGUSR1 to Tini, and expect the grand-child to terminate, then the child, and then Tini.
     print "Running process group test"
-    p = subprocess.Popen([tini, '-g', '--', os.path.join(src, "test", "pgroup", "stage_1.py")], env=dict(os.environ, **env))
+    p = subprocess.Popen([tini, '-g', '--', os.path.join(src, "test", "pgroup", "stage_1.py")], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
     busy_wait(lambda: len(psutil.Process(p.pid).children(recursive=True)) == 2, 10)
     p.send_signal(signal.SIGUSR1)
     busy_wait(lambda: p.poll() is not None, 10)
 
+
     # Run failing test
-    print "Running failing test"
+    print "Running zombie reaping failure test (Tini should warn)"
     p = subprocess.Popen([tini, "--", os.path.join(src, "test", "reaping", "stage_1.py")], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     out, err = p.communicate()
     assert "zombie reaping won't work" in err, "No warning message was output!"
     ret = p.wait()
     assert ret == 1, "Reaping test succeeded (it should have failed)!"
 
+
+    # Test that the signals are properly in place here.
+    print "running signal configuration test"
+
+    p = subprocess.Popen([os.path.join(build, "sigconf-test"), tini, '-g', '--', "cat", "/proc/self/status"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    out, err = p.communicate()
+
+    # Extract the signal properties, and add a zero at the end.
+    props = [line.split(":") for line in out.splitlines()]
+    props = [(k.strip(), v.strip()) for (k, v) in props]
+    props = [(k, bitmap.BitMap.fromstring(bin(int(v, 16))[2:].zfill(32))) for (k, v) in props if k in ["SigBlk", "SigIgn", "SigCgt"]]
+    props = dict(props)
+
+    # Print actual handling configuration
+    for k, bmp in props.items():
+        print "{0}: {1}".format(k, ", ".join(["{0} ({1})".format(SIGNUM_TO_SIGNAME[n+1], n+1) for n in bmp.nonzero()]))
+
+    for signal_set_name, signals_to_test_for in [
+        ("SigIgn", [signal.SIGTTOU, signal.SIGSEGV, signal.SIGINT,]),
+        ("SigBlk", [signal.SIGTTIN, signal.SIGILL,  signal.SIGTERM,]),
+    ]:
+        for signum in signals_to_test_for:
+            # Use signum - 1 because the bitmap is 0-indexed but represents signals strting at 1
+            assert (signum - 1) in props[signal_set_name].nonzero(), "{0} ({1}) is missing in {2}!".format(SIGNUM_TO_SIGNAME[signum], signum, signal_set_name)
+
     print "---------------------------"
     print "All done, tests as expected"
     print "---------------------------"

test/run_outer_tests.py

@@ -5,6 +5,13 @@ import time
 import pipes
 import subprocess
 import threading
+import pexpect
+import signal
+
+
+class ReturnContainer():
+    def __init__(self):
+        self.value = None
 
 
 class Command(object):
@@ -27,6 +34,8 @@ class Command(object):
             self.stdout, self.stderr = self.proc.communicate()
 
         thread = threading.Thread(target=target)
+        thread.daemon = True
+
         thread.start()
 
         if self.post_cmd is not None:
@@ -55,7 +64,52 @@ class Command(object):
             print "OK"
 
 
-if __name__ == "__main__":
+def attach_and_type_exit_0(name):
+    p = pexpect.spawn("docker attach {0}".format(name))
+    p.sendline('')
+    p.sendline('exit 0')
+
+
+def attach_and_issue_ctrl_c(name):
+    p = pexpect.spawn("docker attach {0}".format(name))
+    p.expect_exact('#')
+    p.sendintr()
+
+
+def test_tty_handling(img, name, base_cmd, fail_cmd, container_command, exit_function, expect_exit_code):
+    print "Testing TTY handling (using container command '{0}' and exit function '{1}')".format(container_command, exit_function.__name__)
+    rc = ReturnContainer()
+
+    shell_ready_event = threading.Event()
+
+    def spawn():
+        p = pexpect.spawn(" ".join(base_cmd + ["--tty", "--interactive", img, "/tini/dist/tini", "-vvv", "--", container_command]))
+        p.expect_exact("#")
+        shell_ready_event.set()
+        rc.value = p.wait()
+
+    thread = threading.Thread(target=spawn)
+    thread.daemon = True
+
+    thread.start()
+
+    if not shell_ready_event.wait(2):
+        raise Exception("Timeout waiting for shell to spawn")
+
+    exit_function(name)
+
+    thread.join(timeout=2)
+
+    if thread.is_alive():
+        subprocess.check_call(fail_cmd)
+        raise Exception("Timeout waiting for container to exit!")
+
+    if rc.value != expect_exit_code:
+        raise Exception("Return code is: {0} (expected {1})".format(rc.value, expect_exit_code))
+
+
+
+def main():
     img = sys.argv[1]
     name = "{0}-test".format(img)
 
@@ -69,8 +123,7 @@ if __name__ == "__main__":
         "--name={0}".format(name),
     ]
 
-    fail_cmd = ["docker", "kill", name]
-
+    fail_cmd = ["docker", "kill", "-s", "KILL", name]
 
     # Funtional tests
     for entrypoint in ["/tini/dist/tini", "/tini/dist/tini-static"]:
@@ -109,3 +162,12 @@ if __name__ == "__main__":
             ["centos:7", "rpm", "rpm"],
     ]:
         Command(base_cmd + [image, "sh", "-c", "{0} -i /tini/dist/*.{1} && /usr/bin/tini true".format(pkg_manager, extension)], fail_cmd).run()
+
+
+    # Test tty handling
+    test_tty_handling(img, name, base_cmd, fail_cmd, "dash", attach_and_type_exit_0, 0)
+    test_tty_handling(img, name, base_cmd, fail_cmd, "dash -c 'while true; do echo \#; sleep 0.1; done'", attach_and_issue_ctrl_c, 128 + signal.SIGINT)
+
+
+if __name__ == "__main__":
+    main()

test/sigconf/sigconf-test.c

@@ -0,0 +1,29 @@
+/*
+Test program to:
++ Ignore a few signals
++ Block a few signals
++ Exec whatever the test runner asked for
+*/
+
+#include <stdlib.h>
+#include <signal.h>
+#include <unistd.h>
+
+
+int main(int argc, char *argv[]) {
+	// Signals to ignore
+	signal(SIGTTOU, SIG_IGN);  // This one should still be in SigIgn (Tini touches it to ignore it, and should restore it)
+	signal(SIGSEGV, SIG_IGN);  // This one should still be in SigIgn (Tini shouldn't touch it)
+	signal(SIGINT,  SIG_IGN);  // This one should still be in SigIgn (Tini should block it to forward it, and restore it)
+
+	// Signals to block
+	sigset_t set;
+	sigemptyset(&set);
+	sigaddset(&set, SIGTTIN);  // This one should still be in SigIgn (Tini touches it to ignore it, and should restore it)
+	sigaddset(&set, SIGILL);   // This one should still be in SigIgn (Tini shouldn't touch it)
+	sigaddset(&set, SIGTERM);  // This one should still be in SigIgn (Tini should block it to forward it, and restore it)
+	sigprocmask(SIG_BLOCK, &set, NULL);
+
+	// Run whatever we were asked to run
+	execvp(argv[1], argv+1);
+}