Repository: moby/moby (language: go)

Issue:
libcontainerd: Fix racy error override
Related to:
- https://github.com/moby/moby/pull/43564

Previous change removed declaration of the new error variable in the
subroutine scope and turned this statement into overriding an error from
the upper scope.


This results in a data race:
<details>
<summary> SHOW ME </summary>

```
==================
WARNING: DATA RACE
Read at 0x00c000929aa0 by goroutine 43:
  github.com/docker/docker/libcontainerd/supervisor.(*remote).monitorDaemon()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:318 +0x744
  github.com/docker/docker/libcontainerd/supervisor.Start.func1()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:95 +0x4c

Previous write at 0x00c000929aa0 by goroutine 45:
  github.com/docker/docker/libcontainerd/supervisor.(*remote).startContainerd.func1()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:193 +0xb8

Goroutine 43 (running) created at:
  github.com/docker/docker/libcontainerd/supervisor.Start()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:95 +0x578
  main.(*DaemonCli).initContainerd()
      /go/src/github.com/docker/docker/cmd/dockerd/daemon_unix.go:153 +0x244
  main.(*DaemonCli).start()
      /go/src/github.com/docker/docker/cmd/dockerd/daemon.go:170 +0x72c
  main.runDaemon()
      /go/src/github.com/docker/docker/cmd/dockerd/docker_unix.go:14 +0xd4
  main.newDaemonCommand.func1()
      /go/src/github.com/docker/docker/cmd/dockerd/docker.go:37 +0xd8
  github.com/docker/docker/vendor/github.com/spf13/cobra.(*Command).execute()
      /go/src/github.com/docker/docker/vendor/github.com/spf13/cobra/command.go:916 +0x7c8
  github.com/docker/docker/vendor/github.com/spf13/cobra.(*Command).ExecuteC()
      /go/src/github.com/docker/docker/vendor/github.com/spf13/cobra/command.go:1044 +0x480
  github.com/docker/docker/vendor/github.com/spf13/cobra.(*Command).Execute()
      /go/src/github.com/docker/docker/vendor/github.com/spf13/cobra/command.go:968 +0x288
  main.main()
      /go/src/github.com/docker/docker/cmd/dockerd/docker.go:102 +0x28c

Goroutine 45 (running) created at:
  github.com/docker/docker/libcontainerd/supervisor.(*remote).startContainerd()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:177 +0x798
  github.com/docker/docker/libcontainerd/supervisor.(*remote).monitorDaemon()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:286 +0x274
  github.com/docker/docker/libcontainerd/supervisor.Start.func1()
      /go/src/github.com/docker/docker/libcontainerd/supervisor/remote_daemon.go:95 +0x4c
==================
```

</details>

See the previous code for reference:
https://github.com/moby/moby/blob/57d2d6ef621cc126ca904e8fc98fbacbd345790a/libcontainerd/remote/client.go#L630-L646

<!--
Please make sure you've read and understood our contributing guidelines;
https://github.com/moby/moby/blob/master/CONTRIBUTING.md

** Make sure all your commits include a signature generated with `git commit -s` **

For additional information on our contributing process, read our contributing
guide https://docs.docker.com/opensource/code/

If this is a bug fix, make sure your description includes "fixes #xxxx", or
"closes #xxxx"

Please provide the following information:
-->

**- What I did**

**- How I did it**

**- How to verify it**

**- Description for the changelog**
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->


**- A picture of a cute animal (not mandatory but encouraged)**




Discussion / hints:
Slightly confused though; if this is now a local `err`, what is the purpose of line 521? https://github.com/moby/moby/pull/45261/files#diff-c3c703e02023ce6a127448365c946281594c6ee77acf7981fbfba77d3d97a89cR521
Hmm, yeah, the original code was also questionable. This PR will at least return the code to the status quo.
It's possible my refactor could have eliminated the possibility of the deadlock that the close-in-a-goroutine code is attempting to remedy. I'll need to dig a bit more to confirm.
Yeah, so trying to make sense of it; first we do a `pipe.Close()` and assign the result to `err`. Then  we do `p.CloseIO`, which we now assign to a new `err`, but that variable is not used anywhere (other than to check what error it was, and to make it `nil` if it was `trasnport is closing`)

```go
io.Stdin = ioutils.NewWriteCloserWrapper(pipe, func() error {
    stdinOnce.Do(func() {
        err = pipe.Close()
        // Do the rest in a new routine to avoid a deadlock if the
        // Exec/Start call failed.
        go func() {
            p, ok := <-stdinCloseSync
            if !ok {
                return
            }
            err := p.CloseIO(context.Background(), containerd.WithStdinCloser)
            if err != nil && strings.Contains(err.Error(), "transport is closing") {
                err = nil
            }
        }()
    })
    return err
})
```

Was the second meant to overwrite the first one _only_ if it didn't produce an error?

At least currently, it seems that this block;

```go
            err := p.CloseIO(context.Background(), containerd.WithStdinCloser)
            if err != nil && strings.Contains(err.Error(), "transport is closing") {
                err = nil
            }
```

Effectively is equivalent to

```go
            _ = p.CloseIO(context.Background(), containerd.WithStdinCloser)
```
I suspect the intention was more along the lines of
```go
io.Stdin = ioutils.NewWriteCloserWrapper(pipe, func() error {
    stdinOnce.Do(func() {
        err = pipe.Close()
        if err != nil {
            return
        }
        p, ok := <-stdinCloseSync
        if !ok {
            return
        }
        err = p.CloseIO(context.Background(), containerd.WithStdinCloser)
        if err != nil && strings.Contains(err.Error(), "transport is closing") {
            err = nil
        }
    })
    return err
})
```
but that would deadlock on `<-stdinCloseSync` when `io.Close()` is called if the `attachStdio` call fails. There must be a race-free way to avoid the deadlock which also propagates errors as intended.
code before the refactor was; https://github.com/moby/moby/blob/ab0eb8fcf6fe6b4ee12393edcd6465501b5c66a1/libcontainerd/client_linux.go#L126-L137

Fix patch:
diff --git a/libcontainerd/remote/client.go b/libcontainerd/remote/client.go
index 23c008d93110f..7e9234ca610f4 100644
--- a/libcontainerd/remote/client.go
+++ b/libcontainerd/remote/client.go
@@ -28,6 +28,7 @@ import (
 	"github.com/docker/docker/libcontainerd/queue"
 	libcontainerdtypes "github.com/docker/docker/libcontainerd/types"
 	"github.com/docker/docker/pkg/ioutils"
+	"github.com/hashicorp/go-multierror"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
@@ -502,27 +503,43 @@ func (c *container) createIO(fifos *cio.FIFOSet, stdinCloseSync chan containerd.
 
 	if io.Stdin != nil {
 		var (
-			err       error
+			closeErr  error
 			stdinOnce sync.Once
 		)
 		pipe := io.Stdin
 		io.Stdin = ioutils.NewWriteCloserWrapper(pipe, func() error {
 			stdinOnce.Do(func() {
-				err = pipe.Close()
-				// Do the rest in a new routine to avoid a deadlock if the
-				// Exec/Start call failed.
-				go func() {
-					p, ok := <-stdinCloseSync
+				closeErr = pipe.Close()
+
+				select {
+				case p, ok := <-stdinCloseSync:
 					if !ok {
 						return
 					}
-					err = p.CloseIO(context.Background(), containerd.WithStdinCloser)
-					if err != nil && strings.Contains(err.Error(), "transport is closing") {
-						err = nil
+					if err := closeStdin(context.Background(), p); err != nil {
+						if closeErr != nil {
+							closeErr = multierror.Append(closeErr, err)
+						} else {
+							// Avoid wrapping a single error in a multierror.
+							closeErr = err
+						}
 					}
-				}()
+				default:
+					// The process wasn't ready. Close its stdin asynchronously.
+					go func() {
+						p, ok := <-stdinCloseSync
+						if !ok {
+							return
+						}
+						if err := closeStdin(context.Background(), p); err != nil {
+							c.client.logger.WithError(err).
+								WithField("container", c.c8dCtr.ID()).
+								Error("failed to close container stdin")
+						}
+					}()
+				}
 			})
-			return err
+			return closeErr
 		})
 	}
 
@@ -534,6 +551,14 @@ func (c *container) createIO(fifos *cio.FIFOSet, stdinCloseSync chan containerd.
 	return rio, err
 }
 
+func closeStdin(ctx context.Context, p containerd.Process) error {
+	err := p.CloseIO(ctx, containerd.WithStdinCloser)
+	if err != nil && strings.Contains(err.Error(), "transport is closing") {
+		err = nil
+	}
+	return err
+}
+
 func (c *client) processEvent(ctx context.Context, et libcontainerdtypes.EventType, ei libcontainerdtypes.EventInfo) {
 	c.eventQ.Append(ei.ContainerID, func() {
 		err := c.backend.ProcessEvent(ei.ContainerID, et, ei)


Test patch: