Fix resize race with podman exec -it

When starting a process with `podman exec -it` the terminal is resized after the process is started. To fix this allow exec start to accept the terminal height and width as parameter and let it resize right before the process is started. Fixes containers#10560 Signed-off-by: Paul Holzinger <[email protected]>
mheon · Jun 24, 2021 · 6ba9617 · 6ba9617
1 parent eb6d4b0
commit 6ba9617
Show file tree

Hide file tree

Showing 12 changed files with 96 additions and 31 deletions.
diff --git a/libpod/container_exec.go b/libpod/container_exec.go
@@ -276,9 +276,10 @@ func (c *Container) ExecStart(sessionID string) error {
 }
 
 // ExecStartAndAttach starts and attaches to an exec session in a container.
+// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
 // TODO: Should we include detach keys in the signature to allow override?
 // TODO: How do we handle AttachStdin/AttachStdout/AttachStderr?
-func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams) error {
+func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *define.TerminalSize) error {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -309,7 +310,7 @@ func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachS
 		return err
 	}
 
-	pid, attachChan, err := c.ociRuntime.ExecContainer(c, session.ID(), opts, streams)
+	pid, attachChan, err := c.ociRuntime.ExecContainer(c, session.ID(), opts, streams, newSize)
 	if err != nil {
 		return err
 	}
@@ -372,7 +373,9 @@ func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachS
 }
 
 // ExecHTTPStartAndAttach starts and performs an HTTP attach to an exec session.
-func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, hijackDone chan<- bool) error {
+// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
+func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w http.ResponseWriter,
+	streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, hijackDone chan<- bool, newSize *define.TerminalSize) error {
 	// TODO: How do we combine streams with the default streams set in the exec session?
 
 	// Ensure that we don't leak a goroutine here
@@ -430,7 +433,7 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 		close(holdConnOpen)
 	}()
 
-	pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, r, w, streams, cancel, hijackDone, holdConnOpen)
+	pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, r, w, streams, cancel, hijackDone, holdConnOpen, newSize)
 	if err != nil {
 		session.State = define.ExecStateStopped
 		session.ExitCode = define.TranslateExecErrorToExitCode(define.ExecErrorCodeGeneric, err)
@@ -733,7 +736,10 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
 	// API there.
 	// TODO: Refactor so this is closed here, before we remove the exec
 	// session.
+	var size *define.TerminalSize
 	if resize != nil {
+		s := <-resize
+		size = &s
 		go func() {
 			logrus.Debugf("Sending resize events to exec session %s", sessionID)
 			for resizeRequest := range resize {
@@ -751,7 +757,7 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
 		}()
 	}
 
-	if err := c.ExecStartAndAttach(sessionID, streams); err != nil {
+	if err := c.ExecStartAndAttach(sessionID, streams, size); err != nil {
 		return -1, err
 	}
 

diff --git a/libpod/oci.go b/libpod/oci.go
@@ -72,13 +72,16 @@ type OCIRuntime interface {
 	// has completed, as one might expect. The attach session will remain
 	// running, in a goroutine that will return via the chan error in the
 	// return signature.
-	ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error)
+	// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
+	ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error)
 	// ExecContainerHTTP executes a command in a running container and
 	// attaches its standard streams to a provided hijacked HTTP session.
 	// Maintains the same invariants as ExecContainer (returns on session
 	// start, with a goroutine running in the background to handle attach).
 	// The HTTP attach itself maintains the same invariants as HTTPAttach.
-	ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error)
+	// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
+	ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, r *http.Request, w http.ResponseWriter,
+		streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error)
 	// ExecContainerDetached executes a command in a running container, but
 	// does not attach to it. Returns the PID of the exec session and an
 	// error (if starting the exec session failed)

diff --git a/libpod/oci_attach_linux.go b/libpod/oci_attach_linux.go
@@ -94,17 +94,18 @@ func (c *Container) attach(streams *define.AttachStreams, keys string, resize <-
 //    this ensures attachToExec gets all of the output of the called process
 //  conmon will then send the exit code of the exec process, or an error in the exec session
 // startFd must be the input side of the fd.
+// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
 //   conmon will wait to start the exec session until the parent process has setup the console socket.
 //   Once attachToExec successfully attaches to the console socket, the child conmon process responsible for calling runtime exec
 //     will read from the output side of start fd, thus learning to start the child process.
 // Thus, the order goes as follow:
 // 1. conmon parent process sets up its console socket. sends on attachFd
-// 2. attachToExec attaches to the console socket after reading on attachFd
+// 2. attachToExec attaches to the console socket after reading on attachFd and resizes the tty
 // 3. child waits on startFd for attachToExec to attach to said console socket
 // 4. attachToExec sends on startFd, signalling it has attached to the socket and child is ready to go
 // 5. child receives on startFd, runs the runtime exec command
 // attachToExec is responsible for closing startFd and attachFd
-func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, sessionID string, startFd, attachFd *os.File) error {
+func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, sessionID string, startFd, attachFd *os.File, newSize *define.TerminalSize) error {
 	if !streams.AttachOutput && !streams.AttachError && !streams.AttachInput {
 		return errors.Wrapf(define.ErrInvalidArg, "must provide at least one stream to attach to")
 	}
@@ -137,6 +138,14 @@ func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, se
 		return err
 	}
 
+	// resize before we start the container process
+	if newSize != nil {
+		err = c.ociRuntime.ExecAttachResize(c, sessionID, *newSize)
+		if err != nil {
+			logrus.Warn("resize failed", err)
+		}
+	}
+
 	// 2: then attach
 	conn, err := openUnixSocket(sockPath)
 	if err != nil {

diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
@@ -25,7 +25,7 @@ import (
 )
 
 // ExecContainer executes a command in a running container
-func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error) {
+func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error) {
 	if options == nil {
 		return -1, nil, errors.Wrapf(define.ErrInvalidArg, "must provide an ExecOptions struct to ExecContainer")
 	}
@@ -68,7 +68,7 @@ func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options
 	attachChan := make(chan error)
 	go func() {
 		// attachToExec is responsible for closing pipes
-		attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, pipes.startPipe, pipes.attachPipe)
+		attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, pipes.startPipe, pipes.attachPipe, newSize)
 		close(attachChan)
 	}()
 
@@ -83,7 +83,8 @@ func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options
 
 // ExecContainerHTTP executes a new command in an existing container and
 // forwards its standard streams over an attach
-func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error) {
+func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter,
+	streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error) {
 	if streams != nil {
 		if !streams.Stdin && !streams.Stdout && !streams.Stderr {
 			return -1, nil, errors.Wrapf(define.ErrInvalidArg, "must provide at least one stream to attach to")
@@ -133,7 +134,7 @@ func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, o
 	conmonPipeDataChan := make(chan conmonPipeData)
 	go func() {
 		// attachToExec is responsible for closing pipes
-		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog)
+		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog, newSize)
 		close(attachChan)
 	}()
 
@@ -486,7 +487,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 }
 
 // Attach to a container over HTTP
-func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string) (deferredErr error) {
+func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string, newSize *define.TerminalSize) (deferredErr error) {
 	// NOTE: As you may notice, the attach code is quite complex.
 	// Many things happen concurrently and yet are interdependent.
 	// If you ever change this function, make sure to write to the
@@ -524,6 +525,14 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 		return err
 	}
 
+	// resize before we start the container process
+	if newSize != nil {
+		err = c.ociRuntime.ExecAttachResize(c, sessionID, *newSize)
+		if err != nil {
+			logrus.Warn("resize failed", err)
+		}
+	}
+
 	// 2: then attach
 	conn, err := openUnixSocket(sockPath)
 	if err != nil {

diff --git a/libpod/oci_missing.go b/libpod/oci_missing.go
@@ -119,12 +119,13 @@ func (r *MissingRuntime) AttachResize(ctr *Container, newSize define.TerminalSiz
 }
 
 // ExecContainer is not available as the runtime is missing
-func (r *MissingRuntime) ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error) {
+func (r *MissingRuntime) ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error) {
 	return -1, nil, r.printError()
 }
 
 // ExecContainerHTTP is not available as the runtime is missing
-func (r *MissingRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error) {
+func (r *MissingRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter,
+	streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error) {
 	return -1, nil, r.printError()
 }
 

diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
@@ -178,8 +178,16 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		logrus.Error(errors.Wrapf(e, "error attaching to container %s exec session %s", sessionCtr.ID(), sessionID))
 	}
 
+	var size *define.TerminalSize
+	if bodyParams.Tty && (bodyParams.Height > 0 || bodyParams.Width > 0) {
+		size = &define.TerminalSize{
+			Height: bodyParams.Height,
+			Width:  bodyParams.Width,
+		}
+	}
+
 	hijackChan := make(chan bool, 1)
-	err = sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan)
+	err = sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan, size)
 
 	if <-hijackChan {
 		// If connection was Hijacked, we have to signal it's being closed

diff --git a/pkg/api/handlers/compat/resize.go b/pkg/api/handlers/compat/resize.go
@@ -73,7 +73,7 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 		if err := ctnr.ExecResize(name, sz); err != nil {
-			if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning {
+			if errors.Cause(err) != define.ErrExecSessionStateInvalid || !query.IgnoreNotRunning {
 				utils.InternalServerError(w, errors.Wrapf(err, "cannot resize session"))
 				return
 			}

diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go
@@ -166,8 +166,10 @@ type ExecCreateResponse struct {
 }
 
 type ExecStartConfig struct {
-	Detach bool `json:"Detach"`
-	Tty    bool `json:"Tty"`
+	Detach bool   `json:"Detach"`
+	Tty    bool   `json:"Tty"`
+	Height uint16 `json:"h"`
+	Width  uint16 `json:"w"`
 }
 
 func ImageToImageSummary(l *libimage.Image) (*entities.ImageSummary, error) {

diff --git a/pkg/api/server/register_exec.go b/pkg/api/server/register_exec.go
@@ -269,10 +269,16 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	//      properties:
 	//        Detach:
 	//          type: boolean
-	//          description: Detach from the command. Not presently supported.
+	//          description: Detach from the command.
 	//        Tty:
 	//          type: boolean
-	//          description: Allocate a pseudo-TTY. Presently ignored.
+	//          description: Allocate a pseudo-TTY.
+	//        h:
+	//          type: integer
+	//          description: Height of the TTY session in characters. Tty must be set to true to use it.
+	//        w:
+	//          type: integer
+	//          description: Width of the TTY session in characters. Tty must be set to true to use it.
 	// produces:
 	// - application/json
 	// responses:

diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go
@@ -343,7 +343,7 @@ func attachHandleResize(ctx, winCtx context.Context, winChange chan os.Signal, i
 			resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
 		}
 		if resizeErr != nil {
-			logrus.Warnf("failed to resize TTY: %v", resizeErr)
+			logrus.Infof("failed to resize TTY: %v", resizeErr)
 		}
 	}
 
@@ -408,6 +408,17 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
 	// If we are in TTY mode, we need to set raw mode for the terminal.
 	// TODO: Share all of this with Attach() for containers.
 	needTTY := terminalFile != nil && terminal.IsTerminal(int(terminalFile.Fd())) && isTerm
+
+	body := struct {
+		Detach bool   `json:"Detach"`
+		TTY    bool   `json:"Tty"`
+		Height uint16 `json:"h"`
+		Width  uint16 `json:"w"`
+	}{
+		Detach: false,
+		TTY:    needTTY,
+	}
+
 	if needTTY {
 		state, err := setRawTerminal(terminalFile)
 		if err != nil {
@@ -419,13 +430,14 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
 			}
 			logrus.SetFormatter(&logrus.TextFormatter{})
 		}()
+		w, h, err := terminal.GetSize(int(terminalFile.Fd()))
+		if err != nil {
+			logrus.Warnf("failed to obtain TTY size: %v", err)
+		}
+		body.Width = uint16(w)
+		body.Height = uint16(h)
 	}
 
-	body := struct {
-		Detach bool `json:"Detach"`
-	}{
-		Detach: false,
-	}
 	bodyJSON, err := json.Marshal(body)
 	if err != nil {
 		return err

diff --git a/pkg/domain/infra/abi/terminal/terminal_linux.go b/pkg/domain/infra/abi/terminal/terminal_linux.go
@@ -15,12 +15,13 @@ import (
 
 // ExecAttachCtr execs and attaches to a container
 func ExecAttachCtr(ctx context.Context, ctr *libpod.Container, execConfig *libpod.ExecConfig, streams *define.AttachStreams) (int, error) {
-	resize := make(chan define.TerminalSize)
+	var resize chan define.TerminalSize
 	haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
 
 	// Check if we are attached to a terminal. If we are, generate resize
 	// events, and set the terminal to raw mode
 	if haveTerminal && execConfig.Terminal {
+		resize = make(chan define.TerminalSize)
 		cancel, oldTermState, err := handleTerminalAttach(ctx, resize)
 		if err != nil {
 			return -1, err
@@ -32,7 +33,6 @@ func ExecAttachCtr(ctx context.Context, ctr *libpod.Container, execConfig *libpo
 			}
 		}()
 	}
-
 	return ctr.Exec(execConfig, streams, resize)
 }
 

diff --git a/test/system/450-interactive.bats b/test/system/450-interactive.bats
@@ -57,7 +57,16 @@ function teardown() {
 
     # ...and make sure stty under podman reads that.
     run_podman run -it --name mystty $IMAGE stty size <$PODMAN_TEST_PTY
-    is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
+    is "$output" "$rows $cols" "stty under podman run reads the correct dimensions"
+
+    run_podman rm -f mystty
+
+    # check that the same works for podman exec
+    run_podman run -d --name mystty $IMAGE top
+    run_podman exec -it mystty stty size <$PODMAN_TEST_PTY
+    is "$output" "$rows $cols" "stty under podman exec reads the correct dimensions"
+
+    run_podman rm -f mystty
 }