Merge pull request #5431 from kolyshkin/fix-5087-init-hang

libct: do not hang on a half-dead "runc init"

libcontainer/container_linux.go

@@ -223,17 +223,43 @@ func (c *Container) Exec() error {
 func (c *Container) exec() error {
 	path := filepath.Join(c.stateDir, execFifoFilename)
 	if err := handleFifo(path, c.initProcess.pid()); err != nil {
+		if errors.Is(err, errInitDead) {
+			c.killDeadInit()
+		}
 		return err
 	}
 
 	return c.postStart()
 }
 
-// handleFifo listens for either a byte written to the FIFO file or
-// the init process exiting. On kernels supporting pidfd_open(2)
-// (>= 5.3), it uses a single poll(2) for efficiency. On older kernels,
-// it falls back to a polling loop that periodically checks the init
-// process's liveness. This function is blocking.
+// killDeadInit disposes of an init process whose thread group leader is gone
+// while its other threads are still running (see [initDead]). Those threads
+// hold the container's file descriptors open, so whoever inherited them would
+// wait for an EOF forever, and they keep its cgroup populated, so destroying
+// the container would fail.
+//
+// Note the container has to have a private PID namespace for the kernel to
+// kill the rest of its processes once init is gone; destroy() takes care of
+// the shared namespace case.
+func (c *Container) killDeadInit() {
+	// This can not go through c.signal, as hasInit (and so signalInit) treats
+	// a zombie init as not running. Compare the start time here instead, to
+	// make sure the pid was not reused.
+	stat, err := system.Stat(c.initProcess.pid())
+	if err != nil || stat.StartTime != c.initProcessStartTime {
+		return
+	}
+	// For "runc start", init is not our child, so it is only killed, and
+	// reaped by someone else later; its cgroup may thus briefly remain
+	// populated after we return.
+	if err := ignoreTerminateErrors(c.initProcess.terminate()); err != nil {
+		logrus.WithError(err).Warn("unable to kill dead container init")
+	}
+}
+
+// handleFifo waits for either a byte written to the FIFO file by the
+// container's init process (meaning it is about to execve the container
+// process), or for that init process to die. This function is blocking.
 func handleFifo(path string, pid int) error {
 	f, err := os.OpenFile(path, os.O_RDONLY|unix.O_NONBLOCK, 0)
 	if err != nil {
@@ -241,8 +267,8 @@ func handleFifo(path string, pid int) error {
 	}
 	defer f.Close()
 
-	if err := waitForFifoReady(f, pid); err != nil {
-		return err
+	if err := waitForInitWrite(int(f.Fd()), pid); err != nil {
+		return fmt.Errorf("wait for exec fifo: %w", err)
 	}
 
 	if err := readFromExecFifo(f); err != nil {
@@ -255,67 +281,6 @@ func handleFifo(path string, pid int) error {
 	return nil
 }
 
-// waitForFifoReady blocks until either the FIFO has data available to read,
-// or the init process has exited. It does not consume the data — it only
-// returns once a subsequent read on f will not block indefinitely.
-func waitForFifoReady(f *os.File, pid int) error {
-	// TODO: switch to os.Process.WithHandle once go < 1.26 is no longer supported.
-	pidFd, err := unix.PidfdOpen(pid, 0)
-	if err == nil {
-		defer unix.Close(pidFd)
-		return waitForFifoReadyPidfd(f, pidFd)
-	}
-	// fall through to the polling path.
-	return waitForFifoReadyPolling(f, pid)
-}
-
-// fast path: a single poll(2) on both the
-// FIFO and a pidfd, blocking with no timeout.
-func waitForFifoReadyPidfd(f *os.File, pidFd int) error {
-	pfds := []unix.PollFd{
-		{Fd: int32(f.Fd()), Events: unix.POLLIN},
-		{Fd: int32(pidFd), Events: unix.POLLIN},
-	}
-	for {
-		_, err := unix.Poll(pfds, -1)
-		if errors.Is(err, unix.EINTR) {
-			continue
-		}
-		if err != nil {
-			return fmt.Errorf("poll exec fifo: %w", err)
-		}
-		// We don't care which FD woke us up. In all cases the next step
-		// is to read the FIFO: if init wrote the byte (alive or dead),
-		// it's there to be read; if init died without writing, the read
-		// returns EOF and readFromExecFifo reports the dead-process error.
-		return nil
-	}
-}
-
-// the fallback for kernels without pidfd_open.
-func waitForFifoReadyPolling(f *os.File, pid int) error {
-	pfd := []unix.PollFd{{Fd: int32(f.Fd()), Events: unix.POLLIN}}
-	const pollIntervalMs = 100
-	for {
-		n, err := unix.Poll(pfd, pollIntervalMs)
-		if errors.Is(err, unix.EINTR) {
-			continue
-		}
-		if err != nil {
-			return fmt.Errorf("poll exec fifo: %w", err)
-		}
-		if n > 0 && pfd[0].Revents&(unix.POLLIN|unix.POLLHUP) != 0 {
-			return nil
-		}
-		// If init is dead, fallthrough and let readFromExecFifo distinguish
-		// "wrote before dying" from "died before writing".
-		stat, err := system.Stat(pid)
-		if err != nil || stat.State == system.Zombie {
-			return nil
-		}
-	}
-}
-
 func readFromExecFifo(execFifo io.Reader) error {
 	var buf [1]byte
 	n, err := execFifo.Read(buf[:])

libcontainer/init_wait_linux.go

@@ -0,0 +1,85 @@
+package libcontainer
+
+import (
+	"errors"
+	"fmt"
+
+	"golang.org/x/sys/unix"
+
+	"github.com/opencontainers/runc/libcontainer/system"
+)
+
+// errInitDead is returned when "runc init" died before saying what we were
+// waiting for it to say.
+var errInitDead = errors.New("container init process died unexpectedly")
+
+// initPollTimeoutMs is how often waitForInitWrite rechecks that "runc init" is
+// still alive.
+const initPollTimeoutMs = 100
+
+// initDead tells whether the "runc init" process with the given pid is dead,
+// meaning it will never write anything to us again.
+//
+// A zombie counts as dead even though its thread group may still be alive.
+// Seccomp's SCMP_ACT_KILL (SECCOMP_RET_KILL_THREAD) only kills the thread that
+// performed the syscall, and "runc init" runs on the thread group leader (see
+// [Init]). Once that thread is killed, the remaining Go runtime threads live
+// on forever, keeping the shared descriptor table -- and thus their ends of
+// the sync socket and the exec fifo -- open, so we never see an EOF. Neither
+// wait(2) nor a pidfd report a zombie leader with live threads either, which
+// leaves /proc/[pid]/stat as the only usable signal.
+func initDead(pid int) bool {
+	stat, err := system.Stat(pid)
+	// Either the process is gone, or its /proc entry is unreadable;
+	// either way, there is nothing left to wait for.
+	return err != nil || stat.State == system.Zombie || stat.State == system.Dead
+}
+
+// checkInitDead is initDead, replaceable by tests.
+var checkInitDead = initDead
+
+// pollIn polls fd for input, retrying on EINTR, and tells whether the fd is
+// ready to be read from.
+func pollIn(fd, timeoutMs int) (bool, error) {
+	pfds := []unix.PollFd{{Fd: int32(fd), Events: unix.POLLIN}}
+	for {
+		n, err := unix.Poll(pfds, timeoutMs)
+		if errors.Is(err, unix.EINTR) {
+			continue
+		}
+		if err != nil {
+			return false, fmt.Errorf("poll init: %w", err)
+		}
+		return n > 0, nil
+	}
+}
+
+// waitForInitWrite blocks until the given fd, which "runc init" with the given
+// pid is expected to write to, has something to read (or its writing end was
+// closed). It returns errInitDead if init dies before that happens.
+//
+// Should the pid get recycled while we are polling, init is gone entirely,
+// and so are its ends of the fd, which makes the fd readable (with EOF).
+func waitForInitWrite(fd, pid int) error {
+	for {
+		// Do not poll indefinitely -- a dead init is not always
+		// detectable via the fd alone (see [initDead]).
+		//
+		// Any revents (including POLLHUP) means the caller can now read
+		// without blocking, so let it -- data that init managed to write
+		// before dying still has to be processed.
+		if ready, err := pollIn(fd, initPollTimeoutMs); ready || err != nil {
+			return err
+		}
+		if !checkInitDead(pid) {
+			continue
+		}
+		// Init might have written something (and then died, or execve'd a
+		// short-lived container process) after the above poll has timed
+		// out. Whatever it wrote before dying is there now, so check again.
+		if ready, err := pollIn(fd, 0); ready || err != nil {
+			return err
+		}
+		return errInitDead
+	}
+}

libcontainer/init_wait_linux_test.go

@@ -0,0 +1,130 @@
+package libcontainer
+
+import (
+	"errors"
+	"os"
+	"os/exec"
+	"runtime"
+	"testing"
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+const zombieLeaderEnv = "RUNC_TEST_ZOMBIE_LEADER"
+
+// init turns this process into the helper for TestWaitForInitWriteZombieLeader
+// when asked to: it kills its own thread group leader while the rest of its
+// threads (and thus all of its file descriptors) stay alive, the way seccomp's
+// SCMP_ACT_KILL does to "runc init".
+//
+// This has to happen from init, because that is the only place where the Go
+// runtime guarantees we are running on the main thread (i.e. on the thread
+// group leader).
+func init() {
+	if os.Getenv(zombieLeaderEnv) != "1" {
+		return
+	}
+
+	// Keep a non-leader thread around forever. It has to block in a syscall
+	// rather than on a channel, or the Go runtime declares a deadlock and
+	// terminates the whole process.
+	started := make(chan struct{})
+	go func() {
+		runtime.LockOSThread()
+		close(started)
+		for {
+			_ = unix.Pause()
+		}
+	}()
+	<-started
+
+	if unix.Gettid() != unix.Getpid() {
+		panic("zombie leader helper is not running on the thread group leader")
+	}
+	// Terminate this thread, and only this thread.
+	_, _, _ = unix.RawSyscall(unix.SYS_EXIT, 0, 0, 0)
+}
+
+func TestWaitForInitWriteZombieLeader(t *testing.T) {
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+
+	cmd := exec.Command(os.Args[0], "-test.run=^$")
+	cmd.Env = append(os.Environ(), zombieLeaderEnv+"=1")
+	cmd.ExtraFiles = []*os.File{w}
+	if err := cmd.Start(); err != nil {
+		t.Fatal(err)
+	}
+	// Only the helper is supposed to hold the writing end now.
+	w.Close()
+	defer func() {
+		// SIGKILL is group-wide, so it does what the helper's own
+		// exit(2) could not.
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+	}()
+
+	done := make(chan error, 1)
+	go func() {
+		done <- waitForInitWrite(int(r.Fd()), cmd.Process.Pid)
+	}()
+
+	select {
+	case err := <-done:
+		if !errors.Is(err, errInitDead) {
+			t.Fatalf("waitForInitWrite: want %v, got %v", errInitDead, err)
+		}
+	case <-time.After(30 * time.Second):
+		t.Fatal("waitForInitWrite did not notice the dead init")
+	}
+}
+
+func TestWaitForInitWriteReadable(t *testing.T) {
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+	defer w.Close()
+
+	if _, err := w.Write([]byte("0")); err != nil {
+		t.Fatal(err)
+	}
+	// A readable fd has to win over the liveness check.
+	checkInitDead = func(int) bool {
+		t.Error("liveness checked while the fd is readable")
+		return true
+	}
+	defer func() { checkInitDead = initDead }()
+
+	if err := waitForInitWrite(int(r.Fd()), 1); err != nil {
+		t.Fatalf("waitForInitWrite: %v", err)
+	}
+}
+
+// Init may write something and die right after poll(2) has timed out, but
+// before its death is noticed. What it wrote must not be lost.
+func TestWaitForInitWriteThenDie(t *testing.T) {
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+	defer w.Close()
+
+	checkInitDead = func(int) bool {
+		if _, err := w.Write([]byte("0")); err != nil {
+			t.Error(err)
+		}
+		return true
+	}
+	defer func() { checkInitDead = initDead }()
+
+	if err := waitForInitWrite(int(r.Fd()), 1); err != nil {
+		t.Fatalf("waitForInitWrite: %v", err)
+	}
+}

libcontainer/process_linux.go

@@ -42,7 +42,7 @@ type parentProcess interface {
 	// start starts the process execution.
 	start() error
 
-	// send a SIGKILL to the process and wait for the exit.
+	// send a SIGKILL to the process and, if it is our child, wait for the exit.
 	terminate() error
 
 	// wait waits on the process returning the process state.
@@ -130,6 +130,20 @@ func (p *containerProcess) pid() int {
 	return p.cmd.Process.Pid
 }
 
+// setFinalPid makes p refer to the final "runc init" process, which nsexec
+// started and reported the pid of, rather than to the one we have started.
+func (p *containerProcess) setFinalPid(pid int) error {
+	process, err := os.FindProcess(pid)
+	if err != nil {
+		return err
+	}
+	p.cmd.Process = process
+	// Now that the final pid is known, let the sync socket use it to detect an
+	// init that dies without closing its end of the socket.
+	p.comm.syncSockParent.peerPid = pid
+	return nil
+}
+
 func (p *containerProcess) startTime() (uint64, error) {
 	stat, err := system.Stat(p.pid())
 	return stat.StartTime, err
@@ -164,7 +178,7 @@ func (p *containerProcess) terminate() error {
 	return err
 }
 
-func (p *containerProcess) wait() (*os.ProcessState, error) { //nolint:unparam
+func (p *containerProcess) wait() (*os.ProcessState, error) {
 	err := p.cmd.Wait()
 
 	// Return actual ProcessState even on Wait error
@@ -584,9 +598,10 @@ func (p *setnsProcess) start() (retErr error) {
 	if !seenProcReady && ierr == nil {
 		ierr = errors.New("procReady not received")
 	}
-	// Must be done after Shutdown so the child will exit and we can wait for it.
+	// Do not wait for the child here: it may be unable to exit on its own
+	// (for example, if only one of its threads was killed), and the deferred
+	// error handler above terminates and reaps it anyway.
 	if ierr != nil {
-		_, _ = p.wait()
 		return ierr
 	}
 	return nil
@@ -619,11 +634,9 @@ func (p *setnsProcess) execSetns() error {
 	// Ignore the error in case the child has already been reaped for any reason
 	_, _ = firstChildProcess.Wait()
 
-	process, err := os.FindProcess(pid.Pid)
-	if err != nil {
+	if err := p.setFinalPid(pid.Pid); err != nil {
 		return err
 	}
-	p.cmd.Process = process
 	p.process.ops = p
 	return nil
 }
@@ -662,11 +675,9 @@ func (p *initProcess) waitForChildExit(childPid int) error {
 		return &exec.ExitError{ProcessState: status}
 	}
 
-	process, err := os.FindProcess(childPid)
-	if err != nil {
+	if err := p.setFinalPid(childPid); err != nil {
 		return err
 	}
-	p.cmd.Process = process
 	p.process.ops = p
 	return nil
 }

libcontainer/restored_process.go

@@ -5,6 +5,8 @@ import (
 	"os"
 	"os/exec"
 
+	"golang.org/x/sys/unix"
+
 	"github.com/opencontainers/runc/libcontainer/system"
 )
 
@@ -94,8 +96,10 @@ func (p *nonChildProcess) pid() int {
 	return p.processPid
 }
 
+// terminate only kills the process: as we are not its parent, it is someone
+// else's to reap.
 func (p *nonChildProcess) terminate() error {
-	return errors.New("restored process cannot be terminated")
+	return p.signal(unix.SIGKILL)
 }
 
 func (p *nonChildProcess) wait() (*os.ProcessState, error) {

libcontainer/sync_unix.go

@@ -18,6 +18,12 @@ import (
 type syncSocket struct {
 	f      *os.File
 	closed atomic.Bool
+	// peerPid is the pid of the process on the other end of the socket, if
+	// known. When set, ReadPacket does not block forever if that process
+	// dies without closing its end of the socket. Unlike closed, it needs
+	// no synchronization: a parent socket is only ever used by the
+	// goroutine that starts the process, and a child one has no peer pid.
+	peerPid int
 }
 
 func newSyncSocket(f *os.File) *syncSocket {
@@ -43,6 +49,13 @@ func (s *syncSocket) WritePacket(b []byte) (int, error) {
 }
 
 func (s *syncSocket) ReadPacket() ([]byte, error) {
+	if s.peerPid > 0 {
+		// A dead peer does not always result in a readable socket, so
+		// recvfrom(2) alone is not enough to avoid blocking forever.
+		if err := waitForInitWrite(int(s.f.Fd()), s.peerPid); err != nil {
+			return nil, err
+		}
+	}
 	size, _, err := linux.Recvfrom(int(s.f.Fd()), nil, unix.MSG_TRUNC|unix.MSG_PEEK)
 	if err != nil {
 		return nil, fmt.Errorf("fetch packet length from socket: %w", err)

tests/integration/run.bats

@@ -218,3 +218,31 @@ EOF
 	run -1 runc run test_hello
 	assert_output --partial "invalid rootfs: / is not allowed"
 }
+
+# https://github.com/opencontainers/runc/issues/5087
+@test "runc run [seccomp SCMP_ACT_KILL kills init]" {
+	# SCMP_ACT_KILL only kills the offending thread, leaving runc init with a
+	# zombie thread group leader and live threads holding its ends of the sync
+	# socket and the exec fifo open. Make sure runc notices that init is gone,
+	# rather than waiting for an EOF that can never arrive.
+	update_config '   .process.args = ["/bin/true"]
+			| .process.terminal = false
+			| .process.rlimits = [{"type": "RLIMIT_CORE", "soft": 0, "hard": 0}]
+			| .linux.seccomp = {
+				"defaultAction": "SCMP_ACT_ALLOW",
+				"syscalls": [{"names": ["write"], "action": "SCMP_ACT_KILL"}]
+			}'
+
+	# Give runc its own stdio. Should this regress, the leftover threads keep
+	# it open, and bats would hang on it instead of failing the test.
+	local log="$BATS_TEST_TMPDIR/run-5087.log" ret=0
+	timeout -k 5 30 runc run test_busybox </dev/null >"$log" 2>&1 || ret=$?
+	run -0 cat "$log"
+
+	# Not 124 or 137, which mean the timeout fired, i.e. runc has hung.
+	[ "$ret" -eq 1 ]
+	assert_output --partial "container init process died unexpectedly"
+	# The leftover threads have to be gone as well, or the container's cgroup
+	# can not be removed, and the next test to use this name would fail.
+	refute_output --partial "unable to destroy container"
+}