Merge pull request #4123 from estesp/no-del-rootdir-ios

Correct logic of FIFO cleanup

container.go

@@ -32,6 +32,7 @@ import (
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/runtime/v2/runc/options"
+	"github.com/containerd/containerd/sys"
 	"github.com/containerd/typeurl"
 	prototypes "github.com/gogo/protobuf/types"
 	ver "github.com/opencontainers/image-spec/specs-go"
@@ -422,14 +423,33 @@ func attachExistingIO(response *tasks.GetResponse, ioAttach cio.Attach) (cio.IO,
 
 // loadFifos loads the containers fifos
 func loadFifos(response *tasks.GetResponse) *cio.FIFOSet {
-	path := getFifoDir([]string{
+	fifos := []string{
 		response.Process.Stdin,
 		response.Process.Stdout,
 		response.Process.Stderr,
-	})
-	closer := func() error {
-		return os.RemoveAll(path)
 	}
+	closer := func() error {
+		var (
+			err  error
+			dirs = map[string]struct{}{}
+		)
+		for _, fifo := range fifos {
+			if isFifo, _ := sys.IsFifo(fifo); isFifo {
+				if rerr := os.Remove(fifo); err == nil {
+					err = rerr
+				}
+				dirs[filepath.Dir(fifo)] = struct{}{}
+			}
+		}
+		for dir := range dirs {
+			// we ignore errors here because we don't
+			// want to remove the directory if it isn't
+			// empty
+			os.Remove(dir)
+		}
+		return err
+	}
+
 	return cio.NewFIFOSet(cio.Config{
 		Stdin:    response.Process.Stdin,
 		Stdout:   response.Process.Stdout,
@@ -437,14 +457,3 @@ func loadFifos(response *tasks.GetResponse) *cio.FIFOSet {
 		Terminal: response.Process.Terminal,
 	}, closer)
 }
-
-// getFifoDir looks for any non-empty path for a stdio fifo
-// and returns the dir for where it is located
-func getFifoDir(paths []string) string {
-	for _, p := range paths {
-		if p != "" {
-			return filepath.Dir(p)
-		}
-	}
-	return ""
-}

pkg/process/io.go

@@ -33,6 +33,7 @@ import (
 	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/pkg/stdio"
+	"github.com/containerd/containerd/sys"
 	"github.com/containerd/fifo"
 	runc "github.com/containerd/go-runc"
 	"github.com/pkg/errors"
@@ -174,7 +175,7 @@ func copyPipes(ctx context.Context, rio runc.IO, stdin, stdout, stderr string, w
 			},
 		},
 	} {
-		ok, err := isFifo(i.name)
+		ok, err := sys.IsFifo(i.name)
 		if err != nil {
 			return err
 		}
@@ -240,22 +241,6 @@ func (c *countingWriteCloser) Close() error {
 	return c.WriteCloser.Close()
 }
 
-// isFifo checks if a file is a fifo
-// if the file does not exist then it returns false
-func isFifo(path string) (bool, error) {
-	stat, err := os.Stat(path)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, err
-	}
-	if stat.Mode()&os.ModeNamedPipe == os.ModeNamedPipe {
-		return true, nil
-	}
-	return false, nil
-}
-
 // NewBinaryIO runs a custom binary process for pluggable shim logging
 func NewBinaryIO(ctx context.Context, id string, uri *url.URL) (runc.IO, error) {
 	ns, err := namespaces.NamespaceRequired(ctx)

sys/filesys.go

@@ -0,0 +1,35 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package sys
+
+import "os"
+
+// IsFifo checks if a file is a (named pipe) fifo
+// if the file does not exist then it returns false
+func IsFifo(path string) (bool, error) {
+	stat, err := os.Stat(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	if stat.Mode()&os.ModeNamedPipe == os.ModeNamedPipe {
+		return true, nil
+	}
+	return false, nil
+}