From fbce7584d786ed99354a4b33a9c127abd673c3bb Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 5 Oct 2021 11:13:49 +0200 Subject: [PATCH] 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 #11856 Signed-off-by: Paul Holzinger --- libpod/oci_attach_linux.go | 17 +++++++++-------- libpod/oci_conmon_exec_linux.go | 7 ++++--- libpod/oci_conmon_linux.go | 4 ++++ pkg/bindings/containers/attach.go | 14 +++++++------- test/system/030-run.bats | 6 ++++++ 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/libpod/oci_attach_linux.go b/libpod/oci_attach_linux.go index d4d4a10767..1f2a28eade 100644 --- a/libpod/oci_attach_linux.go +++ b/libpod/oci_attach_linux.go @@ -93,7 +93,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 @@ -174,7 +174,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) { @@ -217,11 +217,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 }() @@ -274,7 +269,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: @@ -283,6 +278,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 } diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go index 822377bfe8..654306f92d 100644 --- a/libpod/oci_conmon_exec_linux.go +++ b/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 } diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index 71a7b29fa0..29ec4a92a6 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -701,6 +701,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 } diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go index abf58aaf92..c5f54c1afc 100644 --- a/pkg/bindings/containers/attach.go +++ b/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.Errorf("Failed to write input to service: %v", err) } - 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 }() } diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 4080f08b43..44c2ee509e 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -725,4 +725,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