remote: always send resize before the container starts

There is race condition in the remote client attach logic. Because the
resize api call was handled in an extra goroutine the container was
started before the resize call happend. To fix this we have to call
resize in the same goroutine as attach. When the first resize is done
start a goroutine to listen on SIGWINCH in the background and resize
again if the signal is received.

Fixes #9859

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

6 files changed, 40 insertions, 40 deletions

diff --git a/pkg/api/handlers/compat/resize.go b/pkg/api/handlers/compat/resize.go
index 23ed33a22..f65e313fc 100644
--- a/pkg/api/handlers/compat/resize.go
+++ b/pkg/api/handlers/compat/resize.go
@@ -46,20 +46,13 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
 			utils.ContainerNotFound(w, name, err)
 			return
 		}
-		if state, err := ctnr.State(); err != nil {
-			utils.InternalServerError(w, errors.Wrapf(err, "cannot obtain container state"))
-			return
-		} else if state != define.ContainerStateRunning && !query.IgnoreNotRunning {
-			utils.Error(w, "Container not running", http.StatusConflict,
-				fmt.Errorf("container %q in wrong state %q", name, state.String()))
-			return
-		}
-		// If container is not running, ignore since this can be a race condition, and is expected
 		if err := ctnr.AttachResize(sz); err != nil {
-			if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning {
+			if errors.Cause(err) != define.ErrCtrStateInvalid {
 				utils.InternalServerError(w, errors.Wrapf(err, "cannot resize container"))
-				return
+			} else {
+				utils.Error(w, "Container not running", http.StatusConflict, err)
 			}
+			return
 		}
 		// This is not a 204, even though we write nothing, for compatibility
 		// reasons.
diff --git a/pkg/api/server/register_containers.go b/pkg/api/server/register_containers.go
index aa999905e..88ebb4df5 100644
--- a/pkg/api/server/register_containers.go
+++ b/pkg/api/server/register_containers.go
@@ -1364,6 +1364,8 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
 	//     $ref: "#/responses/ok"
 	//   404:
 	//     $ref: "#/responses/NoSuchContainer"
+	//   409:
+	//     $ref: "#/responses/ConflictError"
 	//   500:
 	//     $ref: "#/responses/InternalError"
 	r.HandleFunc(VersionedPath("/libpod/containers/{name}/resize"), s.APIHandler(compat.ResizeTTY)).Methods(http.MethodPost)
diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go
index fd8a7011d..adef1e7c8 100644
--- a/pkg/bindings/containers/attach.go
+++ b/pkg/bindings/containers/attach.go
@@ -138,7 +138,7 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri
 		winCtx, winCancel := context.WithCancel(ctx)
 		defer winCancel()
 
-		go attachHandleResize(ctx, winCtx, winChange, false, nameOrID, file)
+		attachHandleResize(ctx, winCtx, winChange, false, nameOrID, file)
 	}
 
 	// If we are attaching around a start, we need to "signal"
@@ -327,32 +327,38 @@ func (f *rawFormatter) Format(entry *logrus.Entry) ([]byte, error) {
 	return append(buffer, '\r'), nil
 }
 
-// This is intended to be run as a goroutine, handling resizing for a container
-// or exec session.
+// This is intended to not be run as a goroutine, handling resizing for a container
+// or exec session. It will call resize once and then starts a goroutine which calls resize on winChange
 func attachHandleResize(ctx, winCtx context.Context, winChange chan os.Signal, isExec bool, id string, file *os.File) {
-	// Prime the pump, we need one reset to ensure everything is ready
-	winChange <- sig.SIGWINCH
-	for {
-		select {
-		case <-winCtx.Done():
-			return
-		case <-winChange:
-			w, h, err := terminal.GetSize(int(file.Fd()))
-			if err != nil {
-				logrus.Warnf("failed to obtain TTY size: %v", err)
-			}
+	resize := func() {
+		w, h, err := terminal.GetSize(int(file.Fd()))
+		if err != nil {
+			logrus.Warnf("failed to obtain TTY size: %v", err)
+		}
 
-			var resizeErr error
-			if isExec {
-				resizeErr = ResizeExecTTY(ctx, id, new(ResizeExecTTYOptions).WithHeight(h).WithWidth(w))
-			} else {
-				resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
-			}
-			if resizeErr != nil {
-				logrus.Warnf("failed to resize TTY: %v", resizeErr)
-			}
+		var resizeErr error
+		if isExec {
+			resizeErr = ResizeExecTTY(ctx, id, new(ResizeExecTTYOptions).WithHeight(h).WithWidth(w))
+		} else {
+			resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
+		}
+		if resizeErr != nil {
+			logrus.Warnf("failed to resize TTY: %v", resizeErr)
 		}
 	}
+
+	resize()
+
+	go func() {
+		for {
+			select {
+			case <-winCtx.Done():
+				return
+			case <-winChange:
+				resize()
+			}
+		}
+	}()
 }
 
 // Configure the given terminal for raw mode
@@ -457,7 +463,7 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
 		winCtx, winCancel := context.WithCancel(ctx)
 		defer winCancel()
 
-		go attachHandleResize(ctx, winCtx, winChange, true, sessionID, terminalFile)
+		attachHandleResize(ctx, winCtx, winChange, true, sessionID, terminalFile)
 	}
 
 	if options.GetAttachInput() {
diff --git a/test/apiv2/python/rest_api/fixtures/api_testcase.py b/test/apiv2/python/rest_api/fixtures/api_testcase.py
index 8b771774b..155e93928 100644
--- a/test/apiv2/python/rest_api/fixtures/api_testcase.py
+++ b/test/apiv2/python/rest_api/fixtures/api_testcase.py
@@ -49,7 +49,7 @@ class APITestCase(unittest.TestCase):
 
     def setUp(self):
         super().setUp()
-        APITestCase.podman.run("run", "alpine", "/bin/ls", check=True)
+        APITestCase.podman.run("run", "-d", "alpine", "top", check=True)
 
     def tearDown(self) -> None:
         APITestCase.podman.run("pod", "rm", "--all", "--force", check=True)
diff --git a/test/apiv2/python/rest_api/test_v2_0_0_container.py b/test/apiv2/python/rest_api/test_v2_0_0_container.py
index f67013117..b4b3af2df 100644
--- a/test/apiv2/python/rest_api/test_v2_0_0_container.py
+++ b/test/apiv2/python/rest_api/test_v2_0_0_container.py
@@ -12,7 +12,7 @@ class ContainerTestCase(APITestCase):
         r = requests.get(self.uri("/containers/json"), timeout=5)
         self.assertEqual(r.status_code, 200, r.text)
         obj = r.json()
-        self.assertEqual(len(obj), 0)
+        self.assertEqual(len(obj), 1)
 
     def test_list_all(self):
         r = requests.get(self.uri("/containers/json?all=true"))
@@ -36,7 +36,7 @@ class ContainerTestCase(APITestCase):
             self.assertId(r.content)
 
     def test_delete(self):
-        r = requests.delete(self.uri(self.resolve_container("/containers/{}")))
+        r = requests.delete(self.uri(self.resolve_container("/containers/{}?force=true")))
         self.assertEqual(r.status_code, 204, r.text)
 
     def test_stop(self):
diff --git a/test/system/450-interactive.bats b/test/system/450-interactive.bats
index a9bf52ee8..a2db39492 100644
--- a/test/system/450-interactive.bats
+++ b/test/system/450-interactive.bats
@@ -56,8 +56,7 @@ function teardown() {
     stty rows $rows cols $cols <$PODMAN_TEST_PTY
 
     # ...and make sure stty under podman reads that.
-    # FIXME: 'sleep 1' is needed for podman-remote; without it, there's
-    run_podman run -it --name mystty $IMAGE sh -c 'sleep 1;stty size' <$PODMAN_TEST_PTY
+    run_podman run -it --name mystty $IMAGE stty size <$PODMAN_TEST_PTY
     is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
 }