solve incorrect unmount

1. add WithTempMount for better unmount and remove
2. solve incorrect unmount for
   diff.DiffMounts,
   diff.Apply,
   oci.WithUsername,
   oci.WithUserID,
   remapRootFS

Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

container_opts_unix.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"syscall"
@@ -188,21 +187,9 @@ func withRemappedSnapshotBase(id string, i Image, uid, gid uint32, readonly bool
 }
 
 func remapRootFS(mounts []mount.Mount, uid, gid uint32) error {
-	root, err := ioutil.TempDir("", "ctd-remap")
+	return mount.WithTempMount(mounts, func(root string) error {
-	if err != nil {
+		return filepath.Walk(root, incrementFS(root, uid, gid))
-		return err
+	})
-	}
-	defer os.Remove(root)
-	for _, m := range mounts {
-		if err := m.Mount(root); err != nil {
-			return err
-		}
-	}
-	err = filepath.Walk(root, incrementFS(root, uid, gid))
-	if uerr := mount.Unmount(root, 0); err == nil {
-		err = uerr
-	}
-	return err
 }
 
 func incrementFS(root string, uidInc, gidInc uint32) filepath.WalkFunc {

diff/walking/differ.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"os"
 	"strings"
 	"time"
 
@@ -90,60 +89,49 @@ func (s *walkingDiff) Apply(ctx context.Context, desc ocispec.Descriptor, mounts
 		}
 	}
 
-	dir, err := ioutil.TempDir("", "extract-")
+	var ocidesc ocispec.Descriptor
-	if err != nil {
+	if err := mount.WithTempMount(mounts, func(root string) error {
-		return emptyDesc, errors.Wrap(err, "failed to create temporary directory")
+		ra, err := s.store.ReaderAt(ctx, desc.Digest)
-	}
-	// We change RemoveAll to Remove so that we either leak a temp dir
-	// if it fails but not RM snapshot data. refer to #1868 #1785
-	defer os.Remove(dir)
-
-	if err := mount.All(mounts, dir); err != nil {
-		return emptyDesc, errors.Wrap(err, "failed to mount")
-	}
-	defer func() {
-		if uerr := mount.Unmount(dir, 0); uerr != nil {
-			if err == nil {
-				err = uerr
-			}
-		}
-	}()
-
-	ra, err := s.store.ReaderAt(ctx, desc.Digest)
-	if err != nil {
-		return emptyDesc, errors.Wrap(err, "failed to get reader from content store")
-	}
-	defer ra.Close()
-
-	r := content.NewReader(ra)
-	if isCompressed {
-		ds, err := compression.DecompressStream(r)
 		if err != nil {
-			return emptyDesc, err
+			return errors.Wrap(err, "failed to get reader from content store")
 		}
-		defer ds.Close()
+		defer ra.Close()
-		r = ds
-	}
 
-	digester := digest.Canonical.Digester()
+		r := content.NewReader(ra)
-	rc := &readCounter{
+		if isCompressed {
-		r: io.TeeReader(r, digester.Hash()),
+			ds, err := compression.DecompressStream(r)
-	}
+			if err != nil {
+				return err
+			}
+			defer ds.Close()
+			r = ds
+		}
 
-	if _, err := archive.Apply(ctx, dir, rc); err != nil {
+		digester := digest.Canonical.Digester()
+		rc := &readCounter{
+			r: io.TeeReader(r, digester.Hash()),
+		}
+
+		if _, err := archive.Apply(ctx, root, rc); err != nil {
+			return err
+		}
+
+		// Read any trailing data
+		if _, err := io.Copy(ioutil.Discard, rc); err != nil {
+			return err
+		}
+
+		ocidesc = ocispec.Descriptor{
+			MediaType: ocispec.MediaTypeImageLayer,
+			Size:      rc.c,
+			Digest:    digester.Digest(),
+		}
+		return nil
+
+	}); err != nil {
 		return emptyDesc, err
 	}
-
+	return ocidesc, nil
-	// Read any trailing data
-	if _, err := io.Copy(ioutil.Discard, rc); err != nil {
-		return emptyDesc, err
-	}
-
-	return ocispec.Descriptor{
-		MediaType: ocispec.MediaTypeImageLayer,
-		Size:      rc.c,
-		Digest:    digester.Digest(),
-	}, nil
 }
 
 // DiffMounts creates a diff between the given mounts and uploads the result
@@ -168,108 +156,85 @@ func (s *walkingDiff) DiffMounts(ctx context.Context, lower, upper []mount.Mount
 	default:
 		return emptyDesc, errors.Wrapf(errdefs.ErrNotImplemented, "unsupported diff media type: %v", config.MediaType)
 	}
-	aDir, err := ioutil.TempDir("", "left-")
-	if err != nil {
-		return emptyDesc, errors.Wrap(err, "failed to create temporary directory")
-	}
-	defer os.Remove(aDir)
 
-	bDir, err := ioutil.TempDir("", "right-")
+	var ocidesc ocispec.Descriptor
-	if err != nil {
+	if err := mount.WithTempMount(lower, func(lowerRoot string) error {
-		return emptyDesc, errors.Wrap(err, "failed to create temporary directory")
+		return mount.WithTempMount(upper, func(upperRoot string) error {
-	}
+			var newReference bool
-	defer os.Remove(bDir)
+			if config.Reference == "" {
-
+				newReference = true
-	if err := mount.All(lower, aDir); err != nil {
+				config.Reference = uniqueRef()
-		return emptyDesc, errors.Wrap(err, "failed to mount")
-	}
-	defer func() {
-		if uerr := mount.Unmount(aDir, 0); uerr != nil {
-			if err == nil {
-				err = uerr
 			}
-		}
-	}()
 
-	if err := mount.All(upper, bDir); err != nil {
+			cw, err := s.store.Writer(ctx, config.Reference, 0, "")
-		return emptyDesc, errors.Wrap(err, "failed to mount")
+			if err != nil {
-	}
+				return errors.Wrap(err, "failed to open writer")
-	defer func() {
-		if uerr := mount.Unmount(bDir, 0); uerr != nil {
-			if err == nil {
-				err = uerr
 			}
-		}
+			defer func() {
-	}()
+				if err != nil {
-
+					cw.Close()
-	var newReference bool
+					if newReference {
-	if config.Reference == "" {
+						if err := s.store.Abort(ctx, config.Reference); err != nil {
-		newReference = true
+							log.G(ctx).WithField("ref", config.Reference).Warnf("failed to delete diff upload")
-		config.Reference = uniqueRef()
+						}
-	}
+					}
-
+				}
-	cw, err := s.store.Writer(ctx, config.Reference, 0, "")
+			}()
-	if err != nil {
+			if !newReference {
-		return emptyDesc, errors.Wrap(err, "failed to open writer")
+				if err := cw.Truncate(0); err != nil {
-	}
+					return err
-	defer func() {
-		if err != nil {
-			cw.Close()
-			if newReference {
-				if err := s.store.Abort(ctx, config.Reference); err != nil {
-					log.G(ctx).WithField("ref", config.Reference).Warnf("failed to delete diff upload")
 				}
 			}
-		}
+
-	}()
+			if isCompressed {
-	if !newReference {
+				dgstr := digest.SHA256.Digester()
-		if err := cw.Truncate(0); err != nil {
+				compressed, err := compression.CompressStream(cw, compression.Gzip)
-			return emptyDesc, err
+				if err != nil {
-		}
+					return errors.Wrap(err, "failed to get compressed stream")
+				}
+				err = archive.WriteDiff(ctx, io.MultiWriter(compressed, dgstr.Hash()), lowerRoot, upperRoot)
+				compressed.Close()
+				if err != nil {
+					return errors.Wrap(err, "failed to write compressed diff")
+				}
+
+				if config.Labels == nil {
+					config.Labels = map[string]string{}
+				}
+				config.Labels["containerd.io/uncompressed"] = dgstr.Digest().String()
+			} else {
+				if err = archive.WriteDiff(ctx, cw, lowerRoot, upperRoot); err != nil {
+					return errors.Wrap(err, "failed to write diff")
+				}
+			}
+
+			var commitopts []content.Opt
+			if config.Labels != nil {
+				commitopts = append(commitopts, content.WithLabels(config.Labels))
+			}
+
+			dgst := cw.Digest()
+			if err := cw.Commit(ctx, 0, dgst, commitopts...); err != nil {
+				return errors.Wrap(err, "failed to commit")
+			}
+
+			info, err := s.store.Info(ctx, dgst)
+			if err != nil {
+				return errors.Wrap(err, "failed to get info from content store")
+			}
+
+			ocidesc = ocispec.Descriptor{
+				MediaType: config.MediaType,
+				Size:      info.Size,
+				Digest:    info.Digest,
+			}
+			return nil
+		})
+	}); err != nil {
+		return emptyDesc, err
 	}
 
-	if isCompressed {
+	return ocidesc, nil
-		dgstr := digest.SHA256.Digester()
-		compressed, err := compression.CompressStream(cw, compression.Gzip)
-		if err != nil {
-			return emptyDesc, errors.Wrap(err, "failed to get compressed stream")
-		}
-		err = archive.WriteDiff(ctx, io.MultiWriter(compressed, dgstr.Hash()), aDir, bDir)
-		compressed.Close()
-		if err != nil {
-			return emptyDesc, errors.Wrap(err, "failed to write compressed diff")
-		}
-
-		if config.Labels == nil {
-			config.Labels = map[string]string{}
-		}
-		config.Labels["containerd.io/uncompressed"] = dgstr.Digest().String()
-	} else {
-		if err = archive.WriteDiff(ctx, cw, aDir, bDir); err != nil {
-			return emptyDesc, errors.Wrap(err, "failed to write diff")
-		}
-	}
-
-	var commitopts []content.Opt
-	if config.Labels != nil {
-		commitopts = append(commitopts, content.WithLabels(config.Labels))
-	}
-
-	dgst := cw.Digest()
-	if err := cw.Commit(ctx, 0, dgst, commitopts...); err != nil {
-		return emptyDesc, errors.Wrap(err, "failed to commit")
-	}
-
-	info, err := s.store.Info(ctx, dgst)
-	if err != nil {
-		return emptyDesc, errors.Wrap(err, "failed to get info from content store")
-	}
-
-	return ocispec.Descriptor{
-		MediaType: config.MediaType,
-		Size:      info.Size,
-		Digest:    info.Digest,
-	}, nil
 }
 
 type readCounter struct {

mount/mount.go

@@ -1,5 +1,13 @@
 package mount
 
+import (
+	"io/ioutil"
+	"os"
+
+	"github.com/containerd/containerd/log"
+	"github.com/pkg/errors"
+)
+
 // Mount is the lingua franca of containerd. A mount represents a
 // serialized mount syscall. Components either emit or consume mounts.
 type Mount struct {
@@ -22,3 +30,44 @@ func All(mounts []Mount, target string) error {
 	}
 	return nil
 }
+
+// WithTempMount mounts the provided mounts to a temp dir, and pass the temp dir to f.
+// The mounts are valid during the call to the f.
+// Finally we will unmount and remove the temp dir regardless of the result of f.
+func WithTempMount(mounts []Mount, f func(root string) error) (err error) {
+	root, uerr := ioutil.TempDir("", "containerd-WithTempMount")
+	if uerr != nil {
+		return errors.Wrapf(uerr, "failed to create temp dir for %v", mounts)
+	}
+	// We use Remove here instead of RemoveAll.
+	// The RemoveAll will delete the temp dir and all children it contains.
+	// When the Unmount fails, if we use RemoveAll, We will incorrectly delete data from mounted dir.
+	// if we use Remove,even though we won't successfully delete the temp dir,
+	// but we only leak a temp dir, we don't loss data from mounted dir.
+	// For details, please refer to #1868 #1785.
+	defer func() {
+		if uerr = os.Remove(root); uerr != nil {
+			log.L.Errorf("Failed to remove the temp dir %s: %v", root, uerr)
+		}
+	}()
+
+	// We should do defer first, if not we will not do Unmount when only a part of Mounts are failed.
+	defer func() {
+		if uerr = UnmountAll(root, 0); uerr != nil {
+			uerr = errors.Wrapf(uerr, "failed to unmount %s", root)
+			if err == nil {
+				err = uerr
+			} else {
+				err = errors.Wrap(err, uerr.Error())
+			}
+		}
+	}()
+	if uerr = All(mounts, root); uerr != nil {
+		return errors.Wrapf(uerr, "failed to mount %s", root)
+	}
+
+	if uerr = f(root); uerr != nil {
+		return errors.Wrapf(uerr, "failed to f(%s)", root)
+	}
+	return nil
+}

oci/spec_opts_unix.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -271,49 +270,35 @@ func WithUserID(uid uint32) SpecOpts {
 		if err != nil {
 			return err
 		}
-		root, err := ioutil.TempDir("", "ctd-username")
+
-		if err != nil {
+		return mount.WithTempMount(mounts, func(root string) error {
-			return err
+			ppath, err := fs.RootPath(root, "/etc/passwd")
-		}
+			if err != nil {
-		defer os.Remove(root)
-		for _, m := range mounts {
-			if err := m.Mount(root); err != nil {
 				return err
 			}
-		}
+			f, err := os.Open(ppath)
-		defer func() {
+			if err != nil {
-			if uerr := mount.Unmount(root, 0); uerr != nil {
+				if os.IsNotExist(err) {
-				if err == nil {
+					s.Process.User.UID, s.Process.User.GID = uid, uid
-					err = uerr
+					return nil
 				}
+				return err
 			}
-		}()
+			defer f.Close()
-		ppath, err := fs.RootPath(root, "/etc/passwd")
+			users, err := user.ParsePasswdFilter(f, func(u user.User) bool {
-		if err != nil {
+				return u.Uid == int(uid)
-			return err
+			})
-		}
+			if err != nil {
-		f, err := os.Open(ppath)
+				return err
-		if err != nil {
+			}
-			if os.IsNotExist(err) {
+			if len(users) == 0 {
 				s.Process.User.UID, s.Process.User.GID = uid, uid
 				return nil
 			}
-			return err
+			u := users[0]
-		}
+			s.Process.User.UID, s.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
-		defer f.Close()
-		users, err := user.ParsePasswdFilter(f, func(u user.User) bool {
-			return u.Uid == int(uid)
-		})
-		if err != nil {
-			return err
-		}
-		if len(users) == 0 {
-			s.Process.User.UID, s.Process.User.GID = uid, uid
 			return nil
-		}
+		})
-		u := users[0]
-		s.Process.User.UID, s.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
-		return nil
 	}
 }
 
@@ -334,43 +319,28 @@ func WithUsername(username string) SpecOpts {
 		if err != nil {
 			return err
 		}
-		root, err := ioutil.TempDir("", "ctd-username")
+		return mount.WithTempMount(mounts, func(root string) error {
-		if err != nil {
+			ppath, err := fs.RootPath(root, "/etc/passwd")
-			return err
+			if err != nil {
-		}
-		defer os.Remove(root)
-		for _, m := range mounts {
-			if err := m.Mount(root); err != nil {
 				return err
 			}
-		}
+			f, err := os.Open(ppath)
-		defer func() {
+			if err != nil {
-			if uerr := mount.Unmount(root, 0); uerr != nil {
+				return err
-				if err == nil {
-					err = uerr
-				}
 			}
-		}()
+			defer f.Close()
-		ppath, err := fs.RootPath(root, "/etc/passwd")
+			users, err := user.ParsePasswdFilter(f, func(u user.User) bool {
-		if err != nil {
+				return u.Name == username
-			return err
+			})
-		}
+			if err != nil {
-		f, err := os.Open(ppath)
+				return err
-		if err != nil {
+			}
-			return err
+			if len(users) == 0 {
-		}
+				return errors.Errorf("no users found for %s", username)
-		defer f.Close()
+			}
-		users, err := user.ParsePasswdFilter(f, func(u user.User) bool {
+			u := users[0]
-			return u.Name == username
+			s.Process.User.UID, s.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
+			return nil
 		})
-		if err != nil {
-			return err
-		}
-		if len(users) == 0 {
-			return errors.Errorf("no users found for %s", username)
-		}
-		u := users[0]
-		s.Process.User.UID, s.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
-		return nil
 	}
 }