HTTP Attach: Wait until both STDIN and STDOUT finish

In the old code, there was a chance that we could return when
only one of STDIN or STDOUT had finished - this could lead to us
dropping either input to the container, or output from it, in the
case that one stream terminated early.

To resolve this, use separate channels to return STDOUT and STDIN
errors, and track which ones have returned cleanly to ensure that
we need bith in order to return from the HTTP attach function and
pass control back to the HTTP handler (which would assume we
exited cleanly and close the client's attach connection).

Signed-off-by: Matthew Heon <mheon@redhat.com>

2 files changed, 32 insertions, 22 deletions

diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index c18da68fe..8651c1dc5 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -537,9 +537,6 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 		}
 	}()
 
-	// Make a channel to pass errors back
-	errChan := make(chan error)
-
 	attachStdout := true
 	attachStderr := true
 	attachStdin := true
@@ -580,13 +577,16 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 		hijackWriteErrorAndClose(deferredErr, c.ID(), isTerminal, httpCon, httpBuf)
 	}()
 
+	stdoutChan := make(chan error)
+	stdinChan := make(chan error)
+
 	// Next, STDIN. Avoid entirely if attachStdin unset.
 	if attachStdin {
 		go func() {
 			logrus.Debugf("Beginning STDIN copy")
 			_, err := utils.CopyDetachable(conn, httpBuf, detachKeys)
 			logrus.Debugf("STDIN copy completed")
-			errChan <- err
+			stdinChan <- err
 		}()
 	}
 
@@ -613,19 +613,24 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 			logrus.Debugf("Performing non-terminal HTTP attach for container %s", c.ID())
 			err = httpAttachNonTerminalCopy(conn, httpBuf, c.ID(), attachStdin, attachStdout, attachStderr)
 		}
-		errChan <- err
+		stdoutChan <- err
 		logrus.Debugf("STDOUT/ERR copy completed")
 	}()
 
-	if cancel != nil {
+	for {
 		select {
-		case err := <-errChan:
-			return err
+		case err := <-stdoutChan:
+			if err != nil {
+				return err
+			}
+
+			return nil
+		case err := <-stdinChan:
+			if err != nil {
+				return err
+			}
 		case <-cancel:
 			return nil
 		}
-	} else {
-		var connErr error = <-errChan
-		return connErr
 	}
 }
diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go
index e3f2d6403..1d4f33794 100644
--- a/libpod/oci_conmon_linux.go
+++ b/libpod/oci_conmon_linux.go
@@ -555,9 +555,6 @@ func (r *ConmonOCIRuntime) HTTPAttach(ctr *Container, req *http.Request, w http.
 		return err
 	}
 
-	// Make a channel to pass errors back
-	errChan := make(chan error)
-
 	attachStdout := true
 	attachStderr := true
 	attachStdin := true
@@ -672,6 +669,9 @@ func (r *ConmonOCIRuntime) HTTPAttach(ctr *Container, req *http.Request, w http.
 
 	logrus.Debugf("Forwarding attach output for container %s", ctr.ID())
 
+	stdoutChan := make(chan error)
+	stdinChan := make(chan error)
+
 	// Handle STDOUT/STDERR
 	go func() {
 		var err error
@@ -690,7 +690,7 @@ func (r *ConmonOCIRuntime) HTTPAttach(ctr *Container, req *http.Request, w http.
 			logrus.Debugf("Performing non-terminal HTTP attach for container %s", ctr.ID())
 			err = httpAttachNonTerminalCopy(conn, httpBuf, ctr.ID(), attachStdin, attachStdout, attachStderr)
 		}
-		errChan <- err
+		stdoutChan <- err
 		logrus.Debugf("STDOUT/ERR copy completed")
 	}()
 	// Next, STDIN. Avoid entirely if attachStdin unset.
@@ -698,20 +698,25 @@ func (r *ConmonOCIRuntime) HTTPAttach(ctr *Container, req *http.Request, w http.
 		go func() {
 			_, err := utils.CopyDetachable(conn, httpBuf, detach)
 			logrus.Debugf("STDIN copy completed")
-			errChan <- err
+			stdinChan <- err
 		}()
 	}
 
-	if cancel != nil {
+	for {
 		select {
-		case err := <-errChan:
-			return err
+		case err := <-stdoutChan:
+			if err != nil {
+				return err
+			}
+
+			return nil
+		case err := <-stdinChan:
+			if err != nil {
+				return err
+			}
 		case <-cancel:
 			return nil
 		}
-	} else {
-		var connErr error = <-errChan
-		return connErr
 	}
 }