From d22dedf3d55a307fded2da1749ba92240bb0ffea Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Fri, 17 Jan 2025 11:12:15 -0800 Subject: [PATCH] Check all 3 stdio FDs to determine if terminal is connected to a pty. Previously we were only looking at stdin, which could be a pty but other stdio fds might be redirected. In that case, we can incorrectly end up using the stdin fd as *the* console fd, and sending all stdout/stderr to that FD, ignoring the redirect. Note that the behavior was actually flaky because the mechanism for choosing which stdio fd to treat as *the* pty fd is non-deterministic (due to the map iteration in fdimport/fdimport.go:Import) and so sometimes we would choose the correct one. This CL also cleans up `argsFromProcess` and `argsFromCLI`, which were setting their `FilePayload` unnecessarily, since it is always set in `Execute`. Fixes #11350 Fixes #11349 PiperOrigin-RevId: 716733446 --- pkg/sentry/fdimport/fdimport.go | 7 +++++- runsc/cmd/do.go | 2 +- runsc/cmd/exec.go | 12 +-------- runsc/cmd/exec_test.go | 44 +++++++++------------------------ runsc/console/pty_linux.go | 18 ++++++++++---- 5 files changed, 33 insertions(+), 50 deletions(-) diff --git a/pkg/sentry/fdimport/fdimport.go b/pkg/sentry/fdimport/fdimport.go index 33118a333..ba4a22df5 100644 --- a/pkg/sentry/fdimport/fdimport.go +++ b/pkg/sentry/fdimport/fdimport.go @@ -17,6 +17,8 @@ package fdimport import ( "fmt" + "maps" + "slices" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/context" @@ -50,8 +52,11 @@ func Import(ctx context.Context, fdTable *kernel.FDTable, console bool, uid auth } } + // We iterate through the FDs in sorted order for better determinancy + // in startup, but it shouldn't matter. var ttyFile *vfs.FileDescription - for appFD, hostFD := range fds { + for _, appFD := range slices.Sorted(maps.Keys(fds)) { + hostFD := fds[appFD] fdOpts := host.NewFDOptions{ Savable: true, } diff --git a/runsc/cmd/do.go b/runsc/cmd/do.go index dc3a9c04d..8d639cf48 100644 --- a/runsc/cmd/do.go +++ b/runsc/cmd/do.go @@ -177,7 +177,7 @@ func (c *Do) Execute(_ context.Context, f *flag.FlagSet, args ...any) subcommand Args: f.Args(), Env: os.Environ(), Capabilities: specutils.AllCapabilities(), - Terminal: console.IsPty(os.Stdin.Fd()), + Terminal: console.StdioIsPty(), }, Hostname: hostname, } diff --git a/runsc/cmd/exec.go b/runsc/cmd/exec.go index 48af12a79..1b094c8ab 100644 --- a/runsc/cmd/exec.go +++ b/runsc/cmd/exec.go @@ -375,12 +375,7 @@ func (ex *Exec) argsFromCLI(p *specs.Process, argv []string, enableRaw bool) (*c KGID: kgid, ExtraKGIDs: extraKGIDs, Capabilities: caps, - StdioIsPty: ex.consoleSocket != "" || console.IsPty(os.Stdin.Fd()), - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), + StdioIsPty: ex.consoleSocket != "" || console.StdioIsPty(), }, nil } @@ -447,11 +442,6 @@ func argsFromProcess(specProc *specs.Process, p *specs.Process, enableRaw bool) ExtraKGIDs: extraKGIDs, Capabilities: caps, StdioIsPty: p.Terminal, - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), }, nil } diff --git a/runsc/cmd/exec_test.go b/runsc/cmd/exec_test.go index d0361920e..eda54b662 100644 --- a/runsc/cmd/exec_test.go +++ b/runsc/cmd/exec_test.go @@ -79,14 +79,9 @@ func TestCLIArgs(t *testing.T) { Argv: []string{"ls", "/"}, Envv: []string{"FOO=bar"}, WorkingDirectory: "/foo/bar", - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), - KUID: 2, - KGID: 2, - ExtraKGIDs: []auth.KGID{1, 2, 3}, + KUID: 2, + KGID: 2, + ExtraKGIDs: []auth.KGID{1, 2, 3}, Capabilities: &auth.TaskCapabilities{ BoundingCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), InheritableCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), @@ -113,14 +108,9 @@ func TestCLIArgs(t *testing.T) { Argv: []string{"ls", "/"}, Envv: []string{"FOO=bar", "BAZ=new"}, WorkingDirectory: "/baz", - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), - KUID: 4, - KGID: 4, - ExtraKGIDs: []auth.KGID{1, 2, 3, 4, 5, 6}, + KUID: 4, + KGID: 4, + ExtraKGIDs: []auth.KGID{1, 2, 3, 4, 5, 6}, Capabilities: &auth.TaskCapabilities{ BoundingCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_OVERRIDE, linux.CAP_DAC_READ_SEARCH}), EffectiveCaps: auth.CapabilitySetOfMany([]linux.Capability{linux.CAP_DAC_READ_SEARCH}), @@ -182,14 +172,9 @@ func TestJSONArgs(t *testing.T) { expected: control.ExecArgs{ Argv: []string{"ls", "/"}, WorkingDirectory: "/foo/bar", - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), - KUID: 0, - KGID: 0, - ExtraKGIDs: []auth.KGID{1, 2, 3}, + KUID: 0, + KGID: 0, + ExtraKGIDs: []auth.KGID{1, 2, 3}, Capabilities: &auth.TaskCapabilities{ BoundingCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), EffectiveCaps: auth.CapabilitySetOf(linux.CAP_DAC_OVERRIDE), @@ -214,14 +199,9 @@ func TestJSONArgs(t *testing.T) { expected: control.ExecArgs{ Argv: []string{"ls", "/"}, WorkingDirectory: "/foo/bar", - FilePayload: control.NewFilePayload(map[int]*os.File{ - 0: os.Stdin, - 1: os.Stdout, - 2: os.Stderr, - }, nil), - KUID: 0, - KGID: 0, - ExtraKGIDs: []auth.KGID{}, + KUID: 0, + KGID: 0, + ExtraKGIDs: []auth.KGID{}, Capabilities: &auth.TaskCapabilities{ BoundingCaps: auth.CapabilitySetOf(linux.CAP_DAC_READ_SEARCH), }, diff --git a/runsc/console/pty_linux.go b/runsc/console/pty_linux.go index 589e915d4..b47b675f1 100644 --- a/runsc/console/pty_linux.go +++ b/runsc/console/pty_linux.go @@ -17,10 +17,18 @@ package console -import "golang.org/x/sys/unix" +import ( + "os" -// IsPty returns true if FD is a PTY. -func IsPty(fd uintptr) bool { - _, err := unix.IoctlGetTermios(int(fd), unix.TCGETS) - return err == nil + "golang.org/x/sys/unix" +) + +// StdioIsPty returns true if all stdio FDs are ptys. +func StdioIsPty() bool { + for _, f := range []*os.File{os.Stdin, os.Stdout, os.Stderr} { + if _, err := unix.IoctlGetTermios(int(f.Fd()), unix.TCGETS); err != nil { + return false + } + } + return true }