runc-shim: release terminal I/O on delete

A terminal exec or init process has no processIO. The shim keeps a writer
on the stdin FIFO (openStdin) so the console's stdin copier never sees a
premature EOF, and keeps the console master in `console`. delete closed
the closers only under `if io != nil`, which is never true on the
terminal path, and never closed the console. When a client exits without
calling CloseIO, the stdin copier therefore stays blocked on the FIFO and
the FIFO handles, the console master and the goroutine stay alive until a
later garbage collection finalizes the dropped process object, and for as
long as the client holds its own writer on the FIFO, past that too.

Release the I/O on every path and through one owner. delete shuts the
console down, drains, then closes closers, console and io regardless of
io; a process deleted from the created state never ran setExited, so this
is its only shutdown. CopyConsole returns a console that owns its
teardown: shutdown and deregistration from the epoller happen once, while
the fd is still open, and Close then closes the master, wakes a writer
parked on it, and closes the stdin FIFO reader, which ends a copier
blocked reading stdin. The copier, the setup-error path, ShutdownConsole
and both deletes go through it, so a bare Close can no longer leave a
copier asleep, and a late ShutdownConsole is a no-op instead of a
deregistration by a fd number that a newer console may own. CopyConsole
also closes the received master when registering it fails and the stdout
FIFO writer when opening its reader fails.

Six regression tests cover the delete ordering, the setup-error paths,
the copier wake-up and the fd reuse; each fails before its part of this
change.

Assisted-by: Claude Code
Signed-off-by: Jacob Gabrielson <jacob.gabrielson@docker.com>

cmd/containerd-shim-runc-v2/process/exec.go

@@ -35,7 +35,6 @@ import (
 	"github.com/containerd/errdefs"
 	"github.com/containerd/fifo"
 	runc "github.com/containerd/go-runc"
-	"github.com/containerd/log"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
@@ -112,15 +111,8 @@ func (e *execProcess) Delete(ctx context.Context) error {
 }
 
 func (e *execProcess) delete(ctx context.Context) error {
-	if err := waitTimeout(ctx, &e.wg, 10*time.Second); err != nil {
-		log.G(ctx).WithError(err).Errorf("failed to drain exec process %s io", e.id)
-	}
-	if e.io != nil {
-		for _, c := range e.closers {
-			c.Close()
-		}
-		e.io.Close()
-	}
+	shutdownAndDrain(ctx, e.parent.Platform, e.console, &e.wg, "exec process "+e.id)
+	closeIO(e.closers, e.console, e.io)
 	pidfile := filepath.Join(e.path, e.id+".pid")
 	// silently ignore error
 	os.Remove(pidfile)

cmd/containerd-shim-runc-v2/process/exec_test.go

@@ -0,0 +1,112 @@
+//go:build !windows
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package process
+
+import (
+	"context"
+	"io"
+	"strings"
+	"testing"
+
+	"github.com/containerd/console"
+
+	"github.com/containerd/containerd/v2/pkg/stdio"
+)
+
+type closeCounter struct{ closed int }
+
+func (c *closeCounter) Close() error {
+	c.closed++
+	return nil
+}
+
+type closeCountingConsole struct {
+	console.Console
+	closed int
+}
+
+func (c *closeCountingConsole) Close() error {
+	c.closed++
+	return nil
+}
+
+// A terminal exec has no processIO; the shim's writer on the stdin FIFO and
+// the console master are the handles it holds, and delete must release both
+// so the console's stdin copier can end.
+func TestExecDeleteReleasesTerminalIO(t *testing.T) {
+	stdin := &closeCounter{}
+	cons := &closeCountingConsole{}
+	var events []string
+	e := &execProcess{
+		id:      "tty",
+		path:    t.TempDir(),
+		parent:  &Init{Platform: recordingPlatform{events: &events}},
+		stdin:   stdin,
+		closers: []io.Closer{stdin},
+		console: cons,
+	}
+	if err := e.delete(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if stdin.closed != 1 {
+		t.Fatalf("stdin FIFO writer closed %d times, want 1", stdin.closed)
+	}
+	if cons.closed != 1 {
+		t.Fatalf("console closed %d times, want 1", cons.closed)
+	}
+}
+
+// recordingPlatform notes the order of console operations.
+type recordingPlatform struct {
+	stdio.Platform
+	events *[]string
+}
+
+func (p recordingPlatform) ShutdownConsole(context.Context, console.Console) error {
+	*p.events = append(*p.events, "shutdown")
+	return nil
+}
+
+type orderedConsole struct {
+	console.Console
+	events *[]string
+}
+
+func (c orderedConsole) Close() error {
+	*c.events = append(*c.events, "close")
+	return nil
+}
+
+// A process deleted from the created state never ran setExited, so delete
+// must shut the console down itself, and before it closes the master.
+func TestExecDeleteShutsConsoleDownBeforeClosing(t *testing.T) {
+	var events []string
+	e := &execProcess{
+		id:      "created-tty",
+		path:    t.TempDir(),
+		parent:  &Init{Platform: recordingPlatform{events: &events}},
+		console: orderedConsole{events: &events},
+	}
+	if err := e.delete(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if got := strings.Join(events, ","); got != "shutdown,close" {
+		t.Fatalf("console operations = %q, want shutdown,close", got)
+	}
+}

cmd/containerd-shim-runc-v2/process/init.go

@@ -298,9 +298,7 @@ func (p *Init) Delete(ctx context.Context) error {
 }
 
 func (p *Init) delete(ctx context.Context) error {
-	if err := waitTimeout(ctx, &p.wg, 10*time.Second); err != nil {
-		log.G(ctx).WithError(err).Errorf("failed to drain init process %s io", p.id)
-	}
+	shutdownAndDrain(ctx, p.Platform, p.console, &p.wg, "init process "+p.id)
 	err := p.runtime.Delete(ctx, p.id, nil)
 	// ignore errors if a runtime has already deleted the process
 	// but we still hold metadata and pipes
@@ -314,12 +312,7 @@ func (p *Init) delete(ctx context.Context) error {
 			err = p.runtimeError(err, "failed to delete task")
 		}
 	}
-	if p.io != nil {
-		for _, c := range p.closers {
-			c.Close()
-		}
-		p.io.Close()
-	}
+	closeIO(p.closers, p.console, p.io)
 	if err2 := mount.UnmountRecursive(p.Rootfs, 0); err2 != nil {
 		log.G(ctx).WithError(err2).Warn("failed to cleanup rootfs mount")
 		if err == nil {

cmd/containerd-shim-runc-v2/process/utils.go

@@ -29,8 +29,11 @@ import (
 	"sync"
 	"time"
 
+	"github.com/containerd/console"
+	"github.com/containerd/containerd/v2/pkg/stdio"
 	"github.com/containerd/errdefs"
 	runc "github.com/containerd/go-runc"
+	"github.com/containerd/log"
 	"golang.org/x/sys/unix"
 )
 
@@ -151,6 +154,32 @@ func (p *pidFile) Read() (int, error) {
 	return runc.ReadPidFile(p.path)
 }
 
+// shutdownAndDrain shuts a terminal console down, so a copier parked on it
+// wakes, then waits for the process I/O to drain. A process deleted from the
+// created state never ran setExited, so this is its only shutdown.
+func shutdownAndDrain(ctx context.Context, platform stdio.Platform, cons console.Console, wg *sync.WaitGroup, what string) {
+	if cons != nil {
+		platform.ShutdownConsole(ctx, cons)
+	}
+	if err := waitTimeout(ctx, wg, 10*time.Second); err != nil {
+		log.G(ctx).WithError(err).Errorf("failed to drain %s io", what)
+	}
+}
+
+// closeIO releases a process's I/O however it was wired. A terminal process
+// has no processIO, but its stdin writer and console are still ours to close.
+func closeIO(closers []io.Closer, cons console.Console, pio *processIO) {
+	for _, c := range closers {
+		c.Close()
+	}
+	if cons != nil {
+		cons.Close()
+	}
+	if pio != nil {
+		pio.Close()
+	}
+}
+
 // waitTimeout handles waiting on a waitgroup with a specified timeout.
 // this is commonly used for waiting on IO to finish after a process has exited
 func waitTimeout(ctx context.Context, wg *sync.WaitGroup, timeout time.Duration) error {

cmd/containerd-shim-runc-v2/runc/platform.go

@@ -67,8 +67,15 @@ func (p *linuxPlatform) CopyConsole(ctx context.Context, console console.Console
 
 	epollConsole, err := p.epoller.Add(console)
 	if err != nil {
+		console.Close()
 		return nil, err
 	}
+	cc := &closingConsole{EpollConsole: epollConsole, epoller: p.epoller}
+	defer func() {
+		if retErr != nil {
+			cc.Close()
+		}
+	}()
 
 	var cwg sync.WaitGroup
 	if stdin != "" {
@@ -76,16 +83,15 @@ func (p *linuxPlatform) CopyConsole(ctx context.Context, console console.Console
 		if err != nil {
 			return nil, err
 		}
+		cc.stdin = in
 		cwg.Add(1)
 		go func() {
 			cwg.Done()
 			bp := bufPool.Get().(*[]byte)
 			defer bufPool.Put(bp)
 			io.CopyBuffer(epollConsole, in, *bp)
-			// we need to shutdown epollConsole when pipe broken
-			epollConsole.Shutdown(p.epoller.CloseConsole)
-			epollConsole.Close()
-			in.Close()
+			// stdin closed or broken: release the console
+			cc.Close()
 		}()
 	}
 
@@ -173,6 +179,7 @@ func (p *linuxPlatform) CopyConsole(ctx context.Context, console console.Console
 		}
 		outr, err := fifo.OpenFifo(ctx, stdout, syscall.O_RDONLY, 0)
 		if err != nil {
+			outw.Close()
 			return nil, err
 		}
 		wg.Add(1)
@@ -190,18 +197,62 @@ func (p *linuxPlatform) CopyConsole(ctx context.Context, console console.Console
 		cwg.Wait()
 	}
 
-	return epollConsole, nil
+	return cc, nil
 }
 
 func (p *linuxPlatform) ShutdownConsole(ctx context.Context, cons console.Console) error {
 	if p.epoller == nil {
 		return errors.New("uninitialized epoller")
 	}
-	epollConsole, ok := cons.(*console.EpollConsole)
+	cc, ok := cons.(*closingConsole)
 	if !ok {
-		return fmt.Errorf("expected EpollConsole, got %#v", cons)
+		return fmt.Errorf("expected closingConsole, got %#v", cons)
 	}
-	return epollConsole.Shutdown(p.epoller.CloseConsole)
+	return cc.shutdown()
+}
+
+// closingConsole is what CopyConsole returns: the epoll console plus the stdin
+// FIFO reader, so the teardown lives in one place.
+//
+// shutdown removes the console from the epoller, once, and only while the
+// fd is still open. After the fd is closed its number can belong to another
+// console, and removing it again would hit that one.
+//
+// Close runs shutdown, closes the master, and closes the stdin FIFO reader,
+// once. Closing the reader is what stops the stdin copier, which is blocked
+// reading it. The copier, CopyConsole's error path, ShutdownConsole and
+// delete all release through these two.
+type closingConsole struct {
+	*console.EpollConsole
+	epoller      *console.Epoller
+	stdin        io.Closer
+	shutdownOnce sync.Once
+	shutdownErr  error
+	closeOnce    sync.Once
+	closeErr     error
+}
+
+func (t *closingConsole) shutdown() error {
+	t.shutdownOnce.Do(func() {
+		t.shutdownErr = t.Shutdown(t.epoller.CloseConsole)
+	})
+	return t.shutdownErr
+}
+
+func (t *closingConsole) Close() error {
+	t.closeOnce.Do(func() {
+		t.shutdown()
+		t.closeErr = t.EpollConsole.Close()
+		// a writer parked on the master before it closed retries on this
+		// wake and sees the closed file instead of another hangup
+		t.Shutdown(func(int) error { return nil })
+		if t.stdin != nil {
+			if err := t.stdin.Close(); err != nil && t.closeErr == nil {
+				t.closeErr = err
+			}
+		}
+	})
+	return t.closeErr
 }
 
 func (p *linuxPlatform) Close() error {

cmd/containerd-shim-runc-v2/runc/platform_linux_test.go

@@ -0,0 +1,312 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package runc
+
+import (
+	"context"
+	"io"
+	"os"
+	"path/filepath"
+	"sync"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/containerd/console"
+	"github.com/containerd/fifo"
+)
+
+type closeCountingConsole struct {
+	console.Console
+	closed int
+}
+
+func (c *closeCountingConsole) Close() error {
+	c.closed++
+	return c.Console.Close()
+}
+
+// A setup error inside CopyConsole must release the console master it
+// already registered with the epoller.
+func TestCopyConsoleReleasesConsoleOnSetupError(t *testing.T) {
+	p, err := NewPlatform()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { p.Close() })
+	master, _, err := console.NewPty()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cons := &closeCountingConsole{Console: master}
+	var wg sync.WaitGroup
+	if _, err := p.CopyConsole(context.Background(), cons, "id", "", ":not-a-uri", "", &wg); err == nil {
+		t.Fatal("CopyConsole accepted an unparsable stdout uri")
+	}
+	if cons.closed != 1 {
+		t.Fatalf("console master closed %d times after a setup error, want 1", cons.closed)
+	}
+}
+
+// A FIFO path with a reader goroutine the test drains, as the daemon would.
+func drainedFifo(t *testing.T) string {
+	t.Helper()
+	path := filepath.Join(t.TempDir(), "stdout")
+	if err := syscall.Mkfifo(path, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		r, err := fifo.OpenFifo(context.Background(), path, syscall.O_RDONLY, 0)
+		if err != nil {
+			return
+		}
+		defer r.Close()
+		io.Copy(io.Discard, r)
+	}()
+	return path
+}
+
+// The shim's master arrives over the runc console socket as a plain
+// blocking file, outside Go's poller; this builds the same kind from a pty.
+func receivedMaster(t *testing.T) console.Console {
+	t.Helper()
+	pty, slave, err := console.NewPty()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { pty.Close() })
+	// Keep a slave open so reads on the master wait for data instead of
+	// failing with EIO, as they do for a running process.
+	s, err := os.OpenFile(slave, os.O_RDWR, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { s.Close() })
+	fd, err := syscall.Dup(int(pty.Fd()))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := syscall.SetNonblock(fd, false); err != nil {
+		t.Fatal(err)
+	}
+	cons, err := console.ConsoleFromFile(os.NewFile(uintptr(fd), "ptmx"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return cons
+}
+
+// Closing the console CopyConsole returned shuts it down and deregisters it,
+// so the stdout copier parked on the epoll condition wakes and finishes; a
+// bare EpollConsole.Close would leave it asleep. Once released, a late
+// ShutdownConsole is a no-op: it must not touch the epoller again.
+func TestConsoleCloseEndsTheCopier(t *testing.T) {
+	p, err := NewPlatform()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { p.Close() })
+	master := receivedMaster(t)
+	var wg sync.WaitGroup
+	cons, err := p.CopyConsole(context.Background(), master, "id", "", drainedFifo(t), "", &wg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	done := make(chan struct{})
+	go func() {
+		wg.Wait()
+		close(done)
+	}()
+	if err := cons.Close(); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("the stdout copier stayed asleep after Close")
+	}
+	if err := cons.Close(); err != nil {
+		t.Fatalf("second Close = %v, want nil", err)
+	}
+	if err := p.ShutdownConsole(context.Background(), cons); err != nil {
+		t.Fatalf("ShutdownConsole after Close = %v, want nil: the released console must not be deregistered again", err)
+	}
+}
+
+func fdsReferring(t *testing.T, path string) int {
+	t.Helper()
+	var want syscall.Stat_t
+	if err := syscall.Stat(path, &want); err != nil {
+		t.Fatal(err)
+	}
+	ents, err := os.ReadDir("/proc/self/fd")
+	if err != nil {
+		t.Fatal(err)
+	}
+	n := 0
+	for _, e := range ents {
+		var st syscall.Stat_t
+		if err := syscall.Stat(filepath.Join("/proc/self/fd", e.Name()), &st); err != nil {
+			continue // closed between ReadDir and Stat
+		}
+		if st.Dev == want.Dev && st.Ino == want.Ino {
+			n++
+		}
+	}
+	return n
+}
+
+// A setup error after the stdin copier started must release the stdin FIFO
+// reader (and the fifo package's O_PATH handle behind it), or the copier
+// stays blocked on it for as long as the caller's own writer lives.
+func TestCopyConsoleSetupErrorReleasesStdinFifo(t *testing.T) {
+	p, err := NewPlatform()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { p.Close() })
+	master, _, err := console.NewPty()
+	if err != nil {
+		t.Fatal(err)
+	}
+	stdin := filepath.Join(t.TempDir(), "stdin")
+	if err := syscall.Mkfifo(stdin, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	// Hold a writer the way the shim holds e.stdin, fully open before
+	// CopyConsole runs: a throwaway non-blocking reader lets the writer's
+	// open(2) complete now rather than in a goroutine.
+	r, err := os.OpenFile(stdin, os.O_RDONLY|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	writer, err := os.OpenFile(stdin, os.O_WRONLY|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { writer.Close() })
+	r.Close()
+	before := fdsReferring(t, stdin)
+	var wg sync.WaitGroup
+	if _, err := p.CopyConsole(context.Background(), master, "id", stdin, ":not-a-uri", "", &wg); err == nil {
+		t.Fatal("CopyConsole accepted an unparsable stdout uri")
+	}
+	if after := fdsReferring(t, stdin); after != before {
+		t.Fatalf("descriptors on the stdin FIFO went %d -> %d across the failed setup: CopyConsole leaked its reader", before, after)
+	}
+}
+
+// blockingMaster re-wraps a pty master as a blocking file outside Go's
+// poller, the kind the shim receives over the runc console socket, and
+// returns the fd number it occupies.
+func blockingMaster(t *testing.T, pty console.Console) (console.Console, int) {
+	t.Helper()
+	fd, err := syscall.Dup(int(pty.Fd()))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := syscall.SetNonblock(fd, false); err != nil {
+		t.Fatal(err)
+	}
+	cons, err := console.ConsoleFromFile(os.NewFile(uintptr(fd), "ptmx"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return cons, fd
+}
+
+// A console released by Close frees its fd number; the next console in the
+// shim takes it. A late ShutdownConsole for the old one, as setExited or
+// delete issue, must not deregister the new one.
+func TestShutdownAfterCloseLeavesAReusedFdAlone(t *testing.T) {
+	p, err := NewPlatform()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { p.Close() })
+	ctx := context.Background()
+	ptyA, slaveA, err := console.NewPty()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { ptyA.Close() })
+	sA, err := os.OpenFile(slaveA, os.O_RDWR, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { sA.Close() })
+	masterA, fdA := blockingMaster(t, ptyA)
+	var wgA sync.WaitGroup
+	consA, err := p.CopyConsole(ctx, masterA, "a", "", drainedFifo(t), "", &wgA)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ptyB, slaveB, err := console.NewPty()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { ptyB.Close() })
+	sB, err := os.OpenFile(slaveB, os.O_RDWR, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { sB.Close() })
+	if err := consA.Close(); err != nil {
+		t.Fatal(err)
+	}
+	masterB, fdB := blockingMaster(t, ptyB)
+	if fdB != fdA {
+		t.Skipf("the freed fd %d was not reused (got %d); the scenario needs the reuse", fdA, fdB)
+	}
+	out := filepath.Join(t.TempDir(), "stdout")
+	if err := syscall.Mkfifo(out, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	got := make(chan string, 1)
+	go func() {
+		r, err := fifo.OpenFifo(ctx, out, syscall.O_RDONLY, 0)
+		if err != nil {
+			return
+		}
+		defer r.Close()
+		buf := make([]byte, 64)
+		n, _ := r.Read(buf)
+		got <- string(buf[:n])
+	}()
+	var wgB sync.WaitGroup
+	consB, err := p.CopyConsole(ctx, masterB, "b", "", out, "", &wgB)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { consB.Close() })
+	// the late shutdown of the released console
+	if err := p.ShutdownConsole(ctx, consA); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := sB.Write([]byte("still here\n")); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case s := <-got:
+		// the pty's output translation turns the newline into CR LF
+		if s != "still here\r\n" {
+			t.Fatalf("B delivered %q", s)
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatal("B's copier delivered nothing: the stale shutdown deregistered the reused fd")
+	}
+}