diff options
author | Paul Holzinger <pholzing@redhat.com> | 2021-10-05 11:13:49 +0200 |
---|---|---|
committer | Matthew Heon <matthew.heon@pm.me> | 2021-10-19 14:08:58 -0400 |
commit | 2cd206d0fce477d123ab8b7acf19fdf34ca0a7aa (patch) | |
tree | c4e853dafd956eb34c0264fa7de4d097cc2528bb | |
parent | 37347c3216a1e613f470fbd933a4257f472b3d2f (diff) | |
download | podman-2cd206d0fce477d123ab8b7acf19fdf34ca0a7aa.tar.gz podman-2cd206d0fce477d123ab8b7acf19fdf34ca0a7aa.tar.bz2 podman-2cd206d0fce477d123ab8b7acf19fdf34ca0a7aa.zip |
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 <pholzing@redhat.com>
<MH: Fixed cherry-pick conflicts>
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
-rw-r--r-- | libpod/oci_attach_linux.go | 17 | ||||
-rw-r--r-- | libpod/oci_conmon_exec_linux.go | 7 | ||||
-rw-r--r-- | libpod/oci_conmon_linux.go | 4 | ||||
-rw-r--r-- | pkg/bindings/containers/attach.go | 14 | ||||
-rw-r--r-- | 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 de435b58a..702628aa8 100644 --- a/libpod/oci_attach_linux.go +++ b/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 } diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go index 5a7677b04..553c91833 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 c2b472f76..58aa3557a 100644 --- a/libpod/oci_conmon_linux.go +++ b/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 } diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go index 6efbcb57b..725d06648 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.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 }() } diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 3d9d834b3..8640d427a 100644 --- a/test/system/030-run.bats +++ b/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 |