fix possible double release on mountable

Refactor the interface to avoid such issues in the future.

BuildKit own mounts are stateless and not affected but
a different mountable implementation could get confused.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

cache/blobs/blobs.go

@@ -78,27 +78,32 @@ func getDiffPairs(ctx context.Context, contentStore content.Store, snapshotter s
 			// reference needs to be committed
 			parent := ref.Parent()
 			var lower []mount.Mount
+			var release func() error
 			if parent != nil {
 				defer parent.Release(context.TODO())
 				m, err := parent.Mount(ctx, true)
 				if err != nil {
 					return nil, err
 				}
-				lower, err = m.Mount()
+				lower, release, err = m.Mount()
 				if err != nil {
 					return nil, err
 				}
-				defer m.Release()
+				if release != nil {
+					defer release()
+				}
 			}
 			m, err := ref.Mount(ctx, true)
 			if err != nil {
 				return nil, err
 			}
-			upper, err := m.Mount()
+			upper, release, err := m.Mount()
 			if err != nil {
 				return nil, err
 			}
-			defer m.Release()
+			if release != nil {
+				defer release()
+			}
 			descr, err := differ.Compare(ctx, lower, upper,
 				diff.WithMediaType(ocispec.MediaTypeImageLayerGzip),
 				diff.WithReference(ref.ID()),

cache/refs.go

@@ -424,10 +424,10 @@ type readOnlyMounter struct {
 	snapshot.Mountable
 }
 
-func (m *readOnlyMounter) Mount() ([]mount.Mount, error) {
-	mounts, err := m.Mountable.Mount()
+func (m *readOnlyMounter) Mount() ([]mount.Mount, func() error, error) {
+	mounts, release, err := m.Mountable.Mount()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	for i, m := range mounts {
 		if m.Type == "overlay" {
@@ -443,7 +443,7 @@ func (m *readOnlyMounter) Mount() ([]mount.Mount, error) {
 		opts = append(opts, "ro")
 		mounts[i].Options = opts
 	}
-	return mounts, nil
+	return mounts, release, nil
 }
 
 func readonlyOverlay(opt []string) []string {

executor/containerdexecutor/executor.go

@@ -69,11 +69,13 @@ func (w containerdExecutor) Exec(ctx context.Context, meta executor.Meta, root c
 		return err
 	}
 
-	rootMounts, err := mountable.Mount()
+	rootMounts, release, err := mountable.Mount()
 	if err != nil {
 		return err
 	}
-	defer mountable.Release()
+	if release != nil {
+		defer release()
+	}
 
 	var sgids []uint32
 	uid, gid, err := oci.ParseUIDGID(meta.User)

executor/oci/spec_unix.go

@@ -136,12 +136,12 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
 			releaseAll()
 			return nil, nil, errors.Wrapf(err, "failed to mount %s", m.Dest)
 		}
-		mounts, err := mountable.Mount()
+		mounts, release, err := mountable.Mount()
 		if err != nil {
 			releaseAll()
 			return nil, nil, errors.WithStack(err)
 		}
-		releasers = append(releasers, mountable.Release)
+		releasers = append(releasers, release)
 		for _, mount := range mounts {
 			mount, err = sm.subMount(mount, m.Selector)
 			if err != nil {

executor/runcexecutor/executor.go

@@ -155,11 +155,13 @@ func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache.
 		return err
 	}
 
-	rootMount, err := mountable.Mount()
+	rootMount, release, err := mountable.Mount()
 	if err != nil {
 		return err
 	}
-	defer mountable.Release()
+	if release != nil {
+		defer release()
+	}
 
 	id := identity.NewID()
 	bundle := filepath.Join(w.root, id)

snapshot/localmounter.go

@@ -31,6 +31,7 @@ type localMounter struct {
 	mounts    []mount.Mount
 	mountable Mountable
 	target    string
+	release   func() error
 }
 
 func (lm *localMounter) Mount() (string, error) {
@@ -38,11 +39,12 @@ func (lm *localMounter) Mount() (string, error) {
 	defer lm.mu.Unlock()
 
 	if lm.mounts == nil {
-		mounts, err := lm.mountable.Mount()
+		mounts, release, err := lm.mountable.Mount()
 		if err != nil {
 			return "", err
 		}
 		lm.mounts = mounts
+		lm.release = release
 	}
 
 	if len(lm.mounts) == 1 && (lm.mounts[0].Type == "bind" || lm.mounts[0].Type == "rbind") {

snapshot/localmounter_unix.go

@@ -21,8 +21,8 @@ func (lm *localMounter) Unmount() error {
 		lm.target = ""
 	}
 
-	if lm.mountable != nil {
-		return lm.mountable.Release()
+	if lm.release != nil {
+		return lm.release()
 	}
 
 	return nil

snapshot/localmounter_windows.go

@@ -18,8 +18,8 @@ func (lm *localMounter) Unmount() error {
 		lm.target = ""
 	}
 
-	if lm.mountable != nil {
-		return lm.mountable.Release()
+	if lm.release != nil {
+		return lm.release()
 	}
 
 	return nil

snapshot/snapshotter.go

@@ -12,8 +12,7 @@ import (
 
 type Mountable interface {
 	// ID() string
-	Mount() ([]mount.Mount, error)
-	Release() error
+	Mount() ([]mount.Mount, func() error, error)
 	IdentityMapping() *idtools.IdentityMapping
 }
 
@@ -85,12 +84,8 @@ type staticMountable struct {
 	idmap  *idtools.IdentityMapping
 }
 
-func (m *staticMountable) Mount() ([]mount.Mount, error) {
-	return m.mounts, nil
-}
-
-func (cm *staticMountable) Release() error {
-	return nil
+func (m *staticMountable) Mount() ([]mount.Mount, func() error, error) {
+	return m.mounts, func() error { return nil }, nil
 }
 
 func (cm *staticMountable) IdentityMapping() *idtools.IdentityMapping {
@@ -122,12 +117,12 @@ func (cs *containerdSnapshotter) release() error {
 }
 
 func (cs *containerdSnapshotter) returnMounts(mf Mountable) ([]mount.Mount, error) {
-	mounts, err := mf.Mount()
+	mounts, release, err := mf.Mount()
 	if err != nil {
 		return nil, err
 	}
 	cs.mu.Lock()
-	cs.releasers = append(cs.releasers, mf.Release)
+	cs.releasers = append(cs.releasers, release)
 	cs.mu.Unlock()
 	return mounts, nil
 }

solver/llbsolver/file/refmanager.go

@@ -47,12 +47,12 @@ func (rm *RefManager) Commit(ctx context.Context, mount fileoptypes.Mount) (file
 	if !ok {
 		return nil, errors.Errorf("invalid mount type %T", mount)
 	}
-	if err := m.m.Release(); err != nil {
-		return nil, err
-	}
 	if m.mr == nil {
 		return nil, errors.Errorf("invalid mount without active ref for commit")
 	}
+	defer func() {
+		m.mr = nil
+	}()
 	return m.mr.Commit(ctx)
 }
 
@@ -62,7 +62,6 @@ type Mount struct {
 }
 
 func (m *Mount) Release(ctx context.Context) error {
-	m.m.Release()
 	if m.mr != nil {
 		return m.mr.Release(ctx)
 	}

solver/llbsolver/ops/exec.go

@@ -349,12 +349,11 @@ func (sm *sshMount) Mount(ctx context.Context, readonly bool) (snapshot.Mountabl
 }
 
 type sshMountInstance struct {
-	sm      *sshMount
-	cleanup func() error
-	idmap   *idtools.IdentityMapping
+	sm    *sshMount
+	idmap *idtools.IdentityMapping
 }
 
-func (sm *sshMountInstance) Mount() ([]mount.Mount, error) {
+func (sm *sshMountInstance) Mount() ([]mount.Mount, func() error, error) {
 	ctx, cancel := context.WithCancel(context.TODO())
 
 	uid := int(sm.sm.mount.SSHOpt.Uid)
@@ -366,7 +365,7 @@ func (sm *sshMountInstance) Mount() ([]mount.Mount, error) {
 			GID: gid,
 		})
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		uid = identity.UID
 		gid = identity.GID
@@ -380,9 +379,9 @@ func (sm *sshMountInstance) Mount() ([]mount.Mount, error) {
 	})
 	if err != nil {
 		cancel()
-		return nil, err
+		return nil, nil, err
 	}
-	sm.cleanup = func() error {
+	release := func() error {
 		var err error
 		if cleanup != nil {
 			err = cleanup()
@@ -395,16 +394,7 @@ func (sm *sshMountInstance) Mount() ([]mount.Mount, error) {
 		Type:    "bind",
 		Source:  sock,
 		Options: []string{"rbind"},
-	}}, nil
-}
-
-func (sm *sshMountInstance) Release() error {
-	if sm.cleanup != nil {
-		if err := sm.cleanup(); err != nil {
-			return err
-		}
-	}
-	return nil
+	}}, release, nil
 }
 
 func (sm *sshMountInstance) IdentityMapping() *idtools.IdentityMapping {
@@ -462,14 +452,18 @@ type secretMountInstance struct {
 	idmap *idtools.IdentityMapping
 }
 
-func (sm *secretMountInstance) Mount() ([]mount.Mount, error) {
+func (sm *secretMountInstance) Mount() ([]mount.Mount, func() error, error) {
 	dir, err := ioutil.TempDir("", "buildkit-secrets")
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to create temp dir")
+		return nil, nil, errors.Wrap(err, "failed to create temp dir")
+	}
+	cleanupDir := func() error {
+		return os.RemoveAll(dir)
 	}
 
 	if err := os.Chmod(dir, 0711); err != nil {
-		return nil, err
+		cleanupDir()
+		return nil, nil, err
 	}
 
 	tmpMount := mount.Mount{
@@ -483,15 +477,23 @@ func (sm *secretMountInstance) Mount() ([]mount.Mount, error) {
 	}
 
 	if err := mount.All([]mount.Mount{tmpMount}, dir); err != nil {
-		return nil, errors.Wrap(err, "unable to setup secret mount")
+		cleanupDir()
+		return nil, nil, errors.Wrap(err, "unable to setup secret mount")
 	}
 	sm.root = dir
 
+	cleanup := func() error {
+		if err := mount.Unmount(dir, 0); err != nil {
+			return err
+		}
+		return cleanupDir()
+	}
+
 	randID := identity.NewID()
 	fp := filepath.Join(dir, randID)
 	if err := ioutil.WriteFile(fp, sm.sm.data, 0600); err != nil {
-		sm.Release()
-		return nil, err
+		cleanup()
+		return nil, nil, err
 	}
 
 	uid := int(sm.sm.mount.SecretOpt.Uid)
@@ -503,35 +505,28 @@ func (sm *secretMountInstance) Mount() ([]mount.Mount, error) {
 			GID: gid,
 		})
 		if err != nil {
-			return nil, err
+			cleanup()
+			return nil, nil, err
 		}
 		uid = identity.UID
 		gid = identity.GID
 	}
 
 	if err := os.Chown(fp, uid, gid); err != nil {
-		return nil, err
+		cleanup()
+		return nil, nil, err
 	}
 
 	if err := os.Chmod(fp, os.FileMode(sm.sm.mount.SecretOpt.Mode&0777)); err != nil {
-		return nil, err
+		cleanup()
+		return nil, nil, err
 	}
 
 	return []mount.Mount{{
 		Type:    "bind",
 		Source:  fp,
 		Options: []string{"ro", "rbind"},
-	}}, nil
-}
-
-func (sm *secretMountInstance) Release() error {
-	if sm.root != "" {
-		if err := mount.Unmount(sm.root, 0); err != nil {
-			return err
-		}
-		return os.RemoveAll(sm.root)
-	}
-	return nil
+	}}, cleanup, nil
 }
 
 func (sm *secretMountInstance) IdentityMapping() *idtools.IdentityMapping {
@@ -767,7 +762,7 @@ type tmpfsMount struct {
 	idmap    *idtools.IdentityMapping
 }
 
-func (m *tmpfsMount) Mount() ([]mount.Mount, error) {
+func (m *tmpfsMount) Mount() ([]mount.Mount, func() error, error) {
 	opt := []string{"nosuid"}
 	if m.readonly {
 		opt = append(opt, "ro")
@@ -776,10 +771,7 @@ func (m *tmpfsMount) Mount() ([]mount.Mount, error) {
 		Type:    "tmpfs",
 		Source:  "tmpfs",
 		Options: opt,
-	}}, nil
-}
-func (m *tmpfsMount) Release() error {
-	return nil
+	}}, func() error { return nil }, nil
 }
 
 func (m *tmpfsMount) IdentityMapping() *idtools.IdentityMapping {