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 <samuelkarp@google.com>

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 {

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)
+	}
+}