libpod: fix race when closing STDIN

There is a race where `conn.Close()` was called before `conn.CloseWrite()`.
In this case `CloseWrite` will fail and an useless error is printed. To
fix this we move the the `CloseWrite()` call to the same goroutine to
remove the race. This ensures that `CloseWrite()` is called before
`Close()` and never afterwards.
Also fixed podman-remote run where the STDIN was never was closed.
This is causing flakes in CI testing.

[NO TESTS NEEDED]

Fixes containers#11856

Signed-off-by: Paul Holzinger <[email protected]>

<MH: Fixed cherry-pick conflicts>

Signed-off-by: Matthew Heon <[email protected]>

libpod/oci_attach_linux.go

@@ -84,7 +84,7 @@ func (c *Container) attach(streams *define.AttachStreams, keys string, resize <-
 	if attachRdy != nil {
 		attachRdy <- true
 	}
-	return readStdio(streams, receiveStdoutError, stdinDone)
+	return readStdio(conn, streams, receiveStdoutError, stdinDone)
 }
 
 // Attach to the given container's exec session
@@ -165,7 +165,7 @@ func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, se
 		return err
 	}
 
-	return readStdio(streams, receiveStdoutError, stdinDone)
+	return readStdio(conn, streams, receiveStdoutError, stdinDone)
 }
 
 func processDetachKeys(keys string) ([]byte, error) {
@@ -208,11 +208,6 @@ func setupStdioChannels(streams *define.AttachStreams, conn *net.UnixConn, detac
 		var err error
 		if streams.AttachInput {
 			_, err = utils.CopyDetachable(conn, streams.InputStream, detachKeys)
-			if err == nil {
-				if connErr := conn.CloseWrite(); connErr != nil {
-					logrus.Errorf("unable to close conn: %q", connErr)
-				}
-			}
 		}
 		stdinDone <- err
 	}()
@@ -265,7 +260,7 @@ func redirectResponseToOutputStreams(outputStream, errorStream io.Writer, writeO
 	return err
 }
 
-func readStdio(streams *define.AttachStreams, receiveStdoutError, stdinDone chan error) error {
+func readStdio(conn *net.UnixConn, streams *define.AttachStreams, receiveStdoutError, stdinDone chan error) error {
 	var err error
 	select {
 	case err = <-receiveStdoutError:
@@ -274,6 +269,12 @@ func readStdio(streams *define.AttachStreams, receiveStdoutError, stdinDone chan
 		if err == define.ErrDetach {
 			return err
 		}
+		if err == nil {
+			// copy stdin is done, close it
+			if connErr := conn.CloseWrite(); connErr != nil {
+				logrus.Errorf("Unable to close conn: %v", connErr)
+			}
+		}
 		if streams.AttachOutput || streams.AttachError {
 			return <-receiveStdoutError
 		}

libpod/oci_conmon_exec_linux.go

@@ -609,9 +609,6 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 			_, err := utils.CopyDetachable(conn, httpBuf, detachKeys)
 			logrus.Debugf("STDIN copy completed")
 			stdinChan <- err
-			if connErr := conn.CloseWrite(); connErr != nil {
-				logrus.Errorf("Unable to close conn: %v", connErr)
-			}
 		}()
 	}
 
@@ -654,6 +651,10 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 			if err != nil {
 				return err
 			}
+			// copy stdin is done, close it
+			if connErr := conn.CloseWrite(); connErr != nil {
+				logrus.Errorf("Unable to close conn: %v", connErr)
+			}
 		case <-cancel:
 			return nil
 		}

libpod/oci_conmon_linux.go

@@ -699,6 +699,10 @@ func (r *ConmonOCIRuntime) HTTPAttach(ctr *Container, req *http.Request, w http.
 			if err != nil {
 				return err
 			}
+			// copy stdin is done, close it
+			if connErr := conn.CloseWrite(); connErr != nil {
+				logrus.Errorf("Unable to close conn: %v", connErr)
+			}
 		case <-cancel:
 			return nil
 		}

pkg/bindings/containers/attach.go

@@ -157,24 +157,24 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri
 	}
 
 	stdoutChan := make(chan error)
-	stdinChan := make(chan error)
+	stdinChan := make(chan error, 1) //stdin channel should not block
 
 	if isSet.stdin {
 		go func() {
 			logrus.Debugf("Copying STDIN to socket")
 
 			_, err := utils.CopyDetachable(socket, stdin, detachKeysInBytes)
-
 			if err != nil && err != define.ErrDetach {
 				logrus.Error("failed to write input to service: " + err.Error())
 			}
-			stdinChan <- err
-
-			if closeWrite, ok := socket.(CloseWriter); ok {
-				if err := closeWrite.CloseWrite(); err != nil {
-					logrus.Warnf("Failed to close STDIN for writing: %v", err)
+			if err == nil {
+				if closeWrite, ok := socket.(CloseWriter); ok {
+					if err := closeWrite.CloseWrite(); err != nil {
+						logrus.Warnf("Failed to close STDIN for writing: %v", err)
+					}
 				}
 			}
+			stdinChan <- err
 		}()
 	}
 

test/system/030-run.bats

@@ -723,4 +723,10 @@ EOF
     is "$output" "Error: strconv.ParseInt: parsing \"a\": invalid syntax"
 }
 
+@test "podman run closes stdin" {
+    random_1=$(random_string 25)
+    run_podman run -i --rm $IMAGE cat <<<"$random_1"
+    is "$output" "$random_1" "output matches STDIN"
+}
+
 # vim: filetype=sh