From cb58bb885ad3a0cb2470a57f3e78eeaa909ceb87 Mon Sep 17 00:00:00 2001 From: yanxuean Date: Fri, 5 Jan 2018 22:43:24 +0800 Subject: [PATCH] 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 --- container_opts_unix.go | 19 +--- diff/walking/differ.go | 251 ++++++++++++++++++----------------------- mount/mount.go | 49 ++++++++ oci/spec_opts_unix.go | 110 +++++++----------- 4 files changed, 200 insertions(+), 229 deletions(-) diff --git a/container_opts_unix.go b/container_opts_unix.go index b678033b7..6e9ed1ce2 100644 --- a/container_opts_unix.go +++ b/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") - if err != nil { - 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 + return mount.WithTempMount(mounts, func(root string) error { + return filepath.Walk(root, incrementFS(root, uid, gid)) + }) } func incrementFS(root string, uidInc, gidInc uint32) filepath.WalkFunc { diff --git a/diff/walking/differ.go b/diff/walking/differ.go index b1afbe930..2aab318fa 100644 --- a/diff/walking/differ.go +++ b/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-") - if err != nil { - return emptyDesc, errors.Wrap(err, "failed to create temporary directory") - } - // 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) + var ocidesc ocispec.Descriptor + if err := mount.WithTempMount(mounts, func(root string) error { + ra, err := s.store.ReaderAt(ctx, desc.Digest) if err != nil { - return emptyDesc, err + return errors.Wrap(err, "failed to get reader from content store") } - defer ds.Close() - r = ds - } + defer ra.Close() - digester := digest.Canonical.Digester() - rc := &readCounter{ - r: io.TeeReader(r, digester.Hash()), - } + r := content.NewReader(ra) + if isCompressed { + 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 } - - // 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 + return ocidesc, 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-") - if err != nil { - return emptyDesc, errors.Wrap(err, "failed to create temporary directory") - } - defer os.Remove(bDir) - - if err := mount.All(lower, aDir); err != nil { - return emptyDesc, errors.Wrap(err, "failed to mount") - } - defer func() { - if uerr := mount.Unmount(aDir, 0); uerr != nil { - if err == nil { - err = uerr + var ocidesc ocispec.Descriptor + if err := mount.WithTempMount(lower, func(lowerRoot string) error { + return mount.WithTempMount(upper, func(upperRoot string) error { + var newReference bool + if config.Reference == "" { + newReference = true + config.Reference = uniqueRef() } - } - }() - if err := mount.All(upper, bDir); err != nil { - return emptyDesc, errors.Wrap(err, "failed to mount") - } - defer func() { - if uerr := mount.Unmount(bDir, 0); uerr != nil { - if err == nil { - err = uerr + cw, err := s.store.Writer(ctx, config.Reference, 0, "") + if err != nil { + return errors.Wrap(err, "failed to open writer") } - } - }() - - var newReference bool - if config.Reference == "" { - newReference = true - config.Reference = uniqueRef() - } - - cw, err := s.store.Writer(ctx, config.Reference, 0, "") - if err != nil { - return emptyDesc, errors.Wrap(err, "failed to open writer") - } - 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") + 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 !newReference { + if err := cw.Truncate(0); err != nil { + return err } } - } - }() - if !newReference { - if err := cw.Truncate(0); err != nil { - return emptyDesc, err - } + + if isCompressed { + dgstr := digest.SHA256.Digester() + compressed, err := compression.CompressStream(cw, compression.Gzip) + 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 { - 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 + return ocidesc, nil } type readCounter struct { diff --git a/mount/mount.go b/mount/mount.go index 94c2c9f92..0d06cf934 100644 --- a/mount/mount.go +++ b/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 +} diff --git a/oci/spec_opts_unix.go b/oci/spec_opts_unix.go index 865aff29a..c344a1bd9 100644 --- a/oci/spec_opts_unix.go +++ b/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 err - } - defer os.Remove(root) - for _, m := range mounts { - if err := m.Mount(root); err != nil { + + return mount.WithTempMount(mounts, func(root string) error { + ppath, err := fs.RootPath(root, "/etc/passwd") + if err != nil { return err } - } - defer func() { - if uerr := mount.Unmount(root, 0); uerr != nil { - if err == nil { - err = uerr + f, err := os.Open(ppath) + if err != nil { + if os.IsNotExist(err) { + s.Process.User.UID, s.Process.User.GID = uid, uid + return nil } + return err } - }() - ppath, err := fs.RootPath(root, "/etc/passwd") - if err != nil { - return err - } - f, err := os.Open(ppath) - if err != nil { - if os.IsNotExist(err) { + 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 } - return err - } - 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 + u := users[0] + s.Process.User.UID, s.Process.User.GID = uint32(u.Uid), uint32(u.Gid) 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") - if err != nil { - return err - } - defer os.Remove(root) - for _, m := range mounts { - if err := m.Mount(root); err != nil { + return mount.WithTempMount(mounts, func(root string) error { + ppath, err := fs.RootPath(root, "/etc/passwd") + if err != nil { return err } - } - defer func() { - if uerr := mount.Unmount(root, 0); uerr != nil { - if err == nil { - err = uerr - } + f, err := os.Open(ppath) + if err != nil { + return err } - }() - ppath, err := fs.RootPath(root, "/etc/passwd") - if err != nil { - return err - } - f, err := os.Open(ppath) - if err != nil { - return err - } - defer f.Close() - users, err := user.ParsePasswdFilter(f, func(u user.User) bool { - return u.Name == username + defer f.Close() + users, err := user.ParsePasswdFilter(f, func(u user.User) bool { + return u.Name == username + }) + 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 }) - 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 } }