diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 62c89102d1b..2db7975d6db 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -507,7 +507,7 @@ func (p *setnsProcess) start() (retErr error) { } var seenProcReady bool - ierr := parseSync(p.comm.syncSockParent, func(sync *syncT) error { + ierr := parseSync(p.comm.syncSockParent, p.pid(), func(sync *syncT) error { switch sync.Type { case procReady: seenProcReady = true @@ -904,7 +904,7 @@ func (p *initProcess) start() (retErr error) { } var seenProcReady bool - ierr := parseSync(p.comm.syncSockParent, func(sync *syncT) error { + ierr := parseSync(p.comm.syncSockParent, p.pid(), func(sync *syncT) error { switch sync.Type { case procMountPlease: if mountRequest == nil { diff --git a/libcontainer/sync.go b/libcontainer/sync.go index 07befdbbc6a..a7f3a70041e 100644 --- a/libcontainer/sync.go +++ b/libcontainer/sync.go @@ -186,8 +186,26 @@ func readSync(pipe *syncSocket, expected syncType) error { // parseSync runs the given callback function on each syncT received from the // child. It will return once io.EOF is returned from the given pipe. -func parseSync(pipe *syncSocket, fn func(*syncT) error) error { +// +// pid is the pid of the child process on the other end of pipe. Before each +// read, parseSync waits until the pipe is actually ready or the child has +// exited (see waitForSyncReady), so a child that dies mid-handshake (e.g. +// killed by seccomp) without cleanly closing its socket can't leave us +// blocked forever. +func parseSync(pipe *syncSocket, pid int, fn func(*syncT) error) error { for { + ready, err := waitForSyncReady(pipe.File(), pid) + if err != nil { + return fmt.Errorf("waiting for sync socket: %w", err) + } + if !ready { + // A successful run always ends via the child voluntarily + // closing its own end of the socket (a real io.EOF below), so + // this always means the child died mid-protocol -- regardless + // of which sync stages we'd already seen -- and must be + // reported as an error rather than treated as a clean exit. + return fmt.Errorf("sync socket: process %d exited before completing sync handshake", pid) + } sync, err := doReadSync(pipe) if err != nil { if errors.Is(err, io.EOF) { diff --git a/libcontainer/sync_linux.go b/libcontainer/sync_linux.go new file mode 100644 index 00000000000..5a6278d301f --- /dev/null +++ b/libcontainer/sync_linux.go @@ -0,0 +1,42 @@ +package libcontainer + +import ( + "errors" + "fmt" + "os" + "time" + + "golang.org/x/sys/unix" + + "github.com/opencontainers/runc/libcontainer/system" +) + +// waitForSyncReady blocks until either the sync socket is ready to read +// (data available, or cleanly closed) or the given process has exited. +// ready=false means the process is gone without its socket becoming ready; +// the caller should treat that the same as io.EOF rather than attempting a +// read, since a stray reference to the child's end of the socket elsewhere +// can otherwise leave a plain read blocked indefinitely. +func waitForSyncReady(f *os.File, pid int) (ready bool, err 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) { + // Avoid a tight spin if signals are arriving rapidly (e.g. Go's + // runtime async-preemption SIGURG). + time.Sleep(time.Millisecond) + continue + } + if err != nil { + return false, fmt.Errorf("poll sync socket: %w", err) + } + if n > 0 && pfd[0].Revents&(unix.POLLIN|unix.POLLHUP) != 0 { + return true, nil + } + stat, err := system.Stat(pid) + if err != nil || stat.State == system.Zombie { + return false, nil + } + } +} diff --git a/libcontainer/sync_linux_test.go b/libcontainer/sync_linux_test.go new file mode 100644 index 00000000000..50e59db7d00 --- /dev/null +++ b/libcontainer/sync_linux_test.go @@ -0,0 +1,71 @@ +package libcontainer + +import ( + "os" + "os/exec" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// TestParseSyncDeadChildDoesNotHang reproduces opencontainers/runc#5087: +// parseSync must not block forever if the child dies mid-handshake (e.g. +// killed by seccomp) while its end of the socket is still held open +// elsewhere. +func TestParseSyncDeadChildDoesNotHang(t *testing.T) { + // Real SOCK_SEQPACKET pair, same as runc's actual sync socket. + fds, err := unix.Socketpair(unix.AF_LOCAL, unix.SOCK_SEQPACKET|unix.SOCK_CLOEXEC, 0) + if err != nil { + t.Fatalf("socketpair: %v", err) + } + // Keep a duplicate of the child's end open for the test's lifetime, so + // the kernel never delivers EOF on fds[0] no matter what happens to + // the child process below (simulating a leaked reference elsewhere). + leaked, err := unix.Dup(fds[1]) + if err != nil { + t.Fatalf("dup: %v", err) + } + defer unix.Close(leaked) + if err := unix.Close(fds[1]); err != nil { + t.Fatalf("close: %v", err) + } + + parentFile := os.NewFile(uintptr(fds[0]), "sync-p") + defer parentFile.Close() + pipe := newSyncSocket(parentFile) + + // A real child process we can kill, standing in for the container init + // that gets killed by seccomp mid-handshake. + cmd := exec.Command("sleep", "100") + if err := cmd.Start(); err != nil { + t.Fatalf("start child: %v", err) + } + defer func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }() + + go func() { + time.Sleep(300 * time.Millisecond) + _ = cmd.Process.Kill() + _ = cmd.Wait() + }() + + done := make(chan error, 1) + go func() { + done <- parseSync(pipe, cmd.Process.Pid, func(sync *syncT) error { + return nil + }) + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected parseSync to return an error when the child dies mid-handshake, got nil") + } + t.Logf("parseSync correctly reported the dead child: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("parseSync hung indefinitely after child died with a leaked socket reference (runc#5087)") + } +}