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

Seccomp's SCMP_ACT_KILL is SECCOMP_RET_KILL_THREAD: it kills the thread
that performed the syscall, not the whole thread group. As "runc init"
locks its main goroutine to the thread group leader, killing that thread
leaves a zombie leader whose remaining Go runtime threads are alive,
keeping the shared descriptor table -- and thus the child ends of the
sync socket and of the exec fifo -- open. The parent therefore never sees
an EOF, and since the thread group is not dead, neither wait(2) nor a
pidfd report anything either, so the parent waits forever.

Add waitForInitWrite, which polls the descriptor with a timeout and, when
nothing arrives, consults /proc/[pid]/stat -- the only thing that reports
a zombie thread group leader. Use it both for reading the sync socket and
for waiting on the exec fifo; the latter replaces the pidfd fast path,
which can not detect this case at all.

Kill and reap the leftover threads once a dead init is detected while
waiting for the exec fifo. They keep the container's stdio descriptors
open, so whoever inherited those would wait for an EOF just as long, and
they keep its cgroup populated, so destroying the container would fail.

Make nonChildProcess.terminate send SIGKILL rather than fail, so that
killDeadInit can use terminate for any init, be it our child ("runc
run") or not ("runc start"). In the latter case, init is only killed,
as reaping it is up to its parent.

Finally, do not wait(2) for the child in the setnsProcess.start error
path: a half-dead child is never reaped, and the deferred error handler
terminates and reaps it anyway.

Fixes #5087

Assisted-by: Claude Opus 5
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

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)