From 46b8a2c93c4e54c02d1c143058f5dca4b8fa91b9 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 15 Oct 2026 21:36:58 +0000 Subject: [PATCH] shim: keep console open when stdin closes On a terminal process, a single PTY master file descriptor carries both stdin and stdout/stderr. The stdin FIFO can reach EOF while the process is still running, either when a client calls CloseIO to close stdin or when an empty client stdin stream closes before openStdin's background writer opens in the kernel. Shutting down and closing the PTY master when the stdin copy goroutine returns hangs up the terminal, which sends SIGHUP to the process, causes writes to the slave to fail with EIO, and causes stdout reads from the master to fail with EBADF. Close only the stdin FIFO reader when the stdin copy goroutine finishes. Closing stdin via CloseIO on a terminal process leaves the PTY master open and the process running without sending SIGHUP. Assisted-by: Antigravity Signed-off-by: Samuel Karp --- cmd/containerd-shim-runc-v2/runc/platform.go | 60 ++++---- .../runc/util_linux_test.go | 139 ++++++++++++++++++ 2 files changed, 173 insertions(+), 26 deletions(-) diff --git a/cmd/containerd-shim-runc-v2/runc/platform.go b/cmd/containerd-shim-runc-v2/runc/platform.go index 864a2376d1..4ad3c6d1ae 100644 --- a/cmd/containerd-shim-runc-v2/runc/platform.go +++ b/cmd/containerd-shim-runc-v2/runc/platform.go @@ -83,15 +83,14 @@ func (p *linuxPlatform) CopyConsole(ctx context.Context, console console.Console if err != nil { return nil, err } - cc.stdin = in + cc.stdinReader = in cwg.Add(1) go func() { cwg.Done() bp := bufPool.Get().(*[]byte) defer bufPool.Put(bp) io.CopyBuffer(epollConsole, in, *bp) - // stdin closed or broken: release the console - cc.Close() + cc.closeStdin() }() } @@ -219,40 +218,49 @@ func (p *linuxPlatform) ShutdownConsole(ctx context.Context, cons console.Consol // 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. +// once. Its closeStdin, shutdown, and Close methods are idempotent and safe +// for concurrent calls. The stdin copier, CopyConsole's error path, +// ShutdownConsole and delete all release through these methods. type closingConsole struct { *console.EpollConsole epoller *console.Epoller - stdin io.Closer + stdinReader io.Closer + stdinOnce sync.Once + stdinErr error 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 - } +func (c *closingConsole) closeStdin() error { + c.stdinOnce.Do(func() { + if c.stdinReader != nil { + c.stdinErr = c.stdinReader.Close() } }) - return t.closeErr + return c.stdinErr +} + +func (c *closingConsole) shutdown() error { + c.shutdownOnce.Do(func() { + c.shutdownErr = c.Shutdown(c.epoller.CloseConsole) + }) + return c.shutdownErr +} + +func (c *closingConsole) Close() error { + c.closeOnce.Do(func() { + c.shutdown() + c.closeErr = c.EpollConsole.Close() + // a writer parked on the master before it closed retries on this + // wake and sees the closed file instead of another hangup + c.Shutdown(func(int) error { return nil }) + if err := c.closeStdin(); err != nil && c.closeErr == nil { + c.closeErr = err + } + }) + return c.closeErr } func (p *linuxPlatform) Close() error { diff --git a/cmd/containerd-shim-runc-v2/runc/util_linux_test.go b/cmd/containerd-shim-runc-v2/runc/util_linux_test.go index 6ecfc16ff8..8deb733f11 100644 --- a/cmd/containerd-shim-runc-v2/runc/util_linux_test.go +++ b/cmd/containerd-shim-runc-v2/runc/util_linux_test.go @@ -17,12 +17,21 @@ package runc import ( + "bytes" + "context" + "errors" "os" + "os/exec" "path/filepath" "reflect" + "strings" + "sync" + "syscall" "testing" "time" + "github.com/containerd/console" + "github.com/containerd/fifo" runcC "github.com/containerd/go-runc" ) @@ -92,3 +101,133 @@ func TestExitStatusFile(t *testing.T) { }) } } + +func TestCopyConsoleWithEmptyStdin(t *testing.T) { + p, err := NewPlatform() + if err != nil { + t.Fatal(err) + } + defer p.Close() + + tmpDir := t.TempDir() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + stdinPath := filepath.Join(tmpDir, "stdin") + stdoutPath := filepath.Join(tmpDir, "stdout") + + stdinW, err := fifo.OpenFifo(ctx, stdinPath, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0600) + if err != nil { + t.Fatal(err) + } + defer stdinW.Close() + + stdoutR, err := fifo.OpenFifo(ctx, stdoutPath, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0600) + if err != nil { + t.Fatal(err) + } + defer stdoutR.Close() + + var stdout bytes.Buffer + stdoutDone := make(chan error, 1) + go func() { + _, err := stdout.ReadFrom(stdoutR) + _ = stdoutR.Close() + stdoutDone <- err + }() + + cons, slavePath, err := console.NewPty() + if err != nil { + t.Fatal(err) + } + + slave, err := os.OpenFile(slavePath, os.O_RDWR, 0) + if err != nil { + cons.Close() + t.Fatal(err) + } + + gateR, gateW, err := os.Pipe() + if err != nil { + slave.Close() + cons.Close() + t.Fatal(err) + } + defer gateW.Close() + + cmd := exec.CommandContext(ctx, "sh", "-c", "read _ <&3; exec seq -s ' ' 100000") + cmd.ExtraFiles = []*os.File{gateR} + cmd.Stdin = slave + cmd.Stdout = slave + cmd.Stderr = slave + cmd.SysProcAttr = &syscall.SysProcAttr{ + Setsid: true, + Setctty: true, + Ctty: 0, + } + + var wg sync.WaitGroup + epollCons, err := p.CopyConsole(ctx, cons, "exec", stdinPath, stdoutPath, "", &wg) + if err != nil { + slave.Close() + cons.Close() + gateR.Close() + t.Fatal(err) + } + defer epollCons.Close() + + if err := cmd.Start(); err != nil { + slave.Close() + gateR.Close() + t.Fatal(err) + } + slave.Close() + gateR.Close() + + if _, err := stdinW.Write(nil); err != nil { + t.Fatal(err) + } + if err := stdinW.Close(); err != nil { + t.Fatal(err) + } + + for { + fd, err := syscall.Open(stdinPath, syscall.O_WRONLY|syscall.O_NONBLOCK, 0) + if err == nil { + syscall.Close(fd) + } else if errors.Is(err, syscall.ENXIO) { + break + } else { + t.Fatalf("unexpected error polling stdin FIFO: %v", err) + } + select { + case <-ctx.Done(): + t.Fatal("timed out waiting for stdin reader to close") + case <-time.After(10 * time.Millisecond): + } + } + + if err := gateW.Close(); err != nil { + t.Fatal(err) + } + + waitErr := cmd.Wait() + if err := p.ShutdownConsole(ctx, epollCons); err != nil { + t.Fatal(err) + } + wg.Wait() + <-stdoutDone + + const expectedSuffix = "99999 100000" + outStr := stdout.String() + suffix := outStr + if len(suffix) > 20 { + suffix = suffix[len(suffix)-20:] + } + if waitErr != nil { + t.Fatalf("process failed: %v (waitStatus=%v), last 20 chars: %q", waitErr, cmd.ProcessState.Sys(), suffix) + } + if !strings.Contains(outStr, expectedSuffix) { + t.Fatalf("output does not contain %q, last 20 chars: %q", expectedSuffix, suffix) + } +}