Merge pull request #14502 from samuelkarp/shim-tty-stdin-close

shim: keep console open when stdin closes

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