Ensure the cache volume locks are unlocked on all paths

... and use a more traditional error handling model, where responsibility for the cleanup passes to the caller _only_ if the called function succeeds. To reinforce that, hard-code nil returns on error paths instead of returning the locks. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
paaul5 · Oct 19, 2022 · 8356687 · 8356687
1 parent 6038220
commit 8356687
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 27 deletions.
diff --git a/cmd/buildah/run.go b/cmd/buildah/run.go
@@ -172,6 +172,7 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
 	if err != nil {
 		return err
 	}
+	defer internalParse.UnlockLockArray(targetLocks)
 	options.Mounts = mounts
 	// Run() will automatically clean them up.
 	options.ExternalImageMounts = mountedImages
@@ -190,9 +191,5 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
 		conditionallyAddHistory(builder, c, "%s %s", shell, strings.Join(args, " "))
 		return builder.Save()
 	}
-	// unlock if any locked files from this RUN statement
-	for _, lockfile := range targetLocks {
-		lockfile.Unlock()
-	}
 	return runerr
 }
diff --git a/internal/parse/parse.go b/internal/parse/parse.go
@@ -187,6 +187,8 @@ func GetBindMount(ctx *types.SystemContext, args []string, contextDir string, st
 }
 
 // GetCacheMount parses a single cache mount entry from the --mount flag.
+//
+// If this function succeeds and returns a non-nil lockfile.Locker, the caller must unlock it (when??).
 func GetCacheMount(args []string, store storage.Store, imageMountLabel string, additionalMountPoints map[string]internal.StageMountDetails) (specs.Mount, lockfile.Locker, error) {
 	var err error
 	var mode uint64
@@ -363,12 +365,18 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
 	}
 
 	var targetLock lockfile.Locker // = nil
+	succeeded := false
+	defer func() {
+		if !succeeded && targetLock != nil {
+			targetLock.Unlock()
+		}
+	}()
 	switch sharing {
 	case "locked":
 		// lock parent cache
 		lockfile, err := lockfile.GetLockfile(filepath.Join(buildahLockFilesDir, BuildahCacheLockfile))
 		if err != nil {
-			return newMount, targetLock, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err)
+			return newMount, nil, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err)
 		}
 		// Will be unlocked after the RUN step is executed.
 		lockfile.Lock()
@@ -378,7 +386,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
 		break
 	default:
 		// error out for unknown values
-		return newMount, targetLock, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err)
+		return newMount, nil, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err)
 	}
 
 	// buildkit parity: default sharing should be shared
@@ -396,10 +404,11 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
 
 	opts, err := parse.ValidateVolumeOpts(newMount.Options)
 	if err != nil {
-		return newMount, targetLock, err
+		return newMount, nil, err
 	}
 	newMount.Options = opts
 
+	succeeded = true
 	return newMount, targetLock, nil
 }
 
@@ -487,19 +496,34 @@ func Volume(volume string) (specs.Mount, error) {
 	return mount, nil
 }
 
+// UnlockLockArray is a helper for cleaning up after GetVolumes and the like.
+func UnlockLockArray(locks []lockfile.Locker) {
+	for _, lock := range locks {
+		lock.Unlock()
+	}
+}
+
 // GetVolumes gets the volumes from --volume and --mount
+//
+// If this function succeeds, the caller must unlock the returned lockfile.Lockers if any (when??).
 func GetVolumes(ctx *types.SystemContext, store storage.Store, volumes []string, mounts []string, contextDir string) ([]specs.Mount, []string, []lockfile.Locker, error) {
 	unifiedMounts, mountedImages, targetLocks, err := getMounts(ctx, store, mounts, contextDir)
 	if err != nil {
-		return nil, mountedImages, targetLocks, err
+		return nil, mountedImages, nil, err
 	}
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			UnlockLockArray(targetLocks)
+		}
+	}()
 	volumeMounts, err := getVolumeMounts(volumes)
 	if err != nil {
-		return nil, mountedImages, targetLocks, err
+		return nil, mountedImages, nil, err
 	}
 	for dest, mount := range volumeMounts {
 		if _, ok := unifiedMounts[dest]; ok {
-			return nil, mountedImages, targetLocks, fmt.Errorf("%v: %w", dest, errDuplicateDest)
+			return nil, mountedImages, nil, fmt.Errorf("%v: %w", dest, errDuplicateDest)
 		}
 		unifiedMounts[dest] = mount
 	}
@@ -508,19 +532,28 @@ func GetVolumes(ctx *types.SystemContext, store storage.Store, volumes []string,
 	for _, mount := range unifiedMounts {
 		finalMounts = append(finalMounts, mount)
 	}
+	succeeded = true
 	return finalMounts, mountedImages, targetLocks, nil
 }
 
 // getMounts takes user-provided input from the --mount flag and creates OCI
 // spec mounts.
 // buildah run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ...
 // buildah run --mount type=tmpfs,target=/dev/shm ...
+//
+// If this function succeeds, the caller must unlock the returned lockfile.Lockers if any (when??).
 func getMounts(ctx *types.SystemContext, store storage.Store, mounts []string, contextDir string) (map[string]specs.Mount, []string, []lockfile.Locker, error) {
 	// If `type` is not set default to "bind"
 	mountType := TypeBind
 	finalMounts := make(map[string]specs.Mount)
 	mountedImages := make([]string, 0)
 	targetLocks := make([]lockfile.Locker, 0)
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			UnlockLockArray(targetLocks)
+		}
+	}()
 
 	errInvalidSyntax := errors.New("incorrect mount format: should be --mount type=<bind|tmpfs>,[src=<host-dir>,]target=<ctr-dir>[,options]")
 
@@ -530,13 +563,13 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mounts []string, c
 	for _, mount := range mounts {
 		tokens := strings.Split(mount, ",")
 		if len(tokens) < 2 {
-			return nil, mountedImages, targetLocks, fmt.Errorf("%q: %w", mount, errInvalidSyntax)
+			return nil, mountedImages, nil, fmt.Errorf("%q: %w", mount, errInvalidSyntax)
 		}
 		for _, field := range tokens {
 			if strings.HasPrefix(field, "type=") {
 				kv := strings.Split(field, "=")
 				if len(kv) != 2 {
-					return nil, mountedImages, targetLocks, fmt.Errorf("%q: %w", mount, errInvalidSyntax)
+					return nil, mountedImages, nil, fmt.Errorf("%q: %w", mount, errInvalidSyntax)
 				}
 				mountType = kv[1]
 			}
@@ -545,39 +578,40 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mounts []string, c
 		case TypeBind:
 			mount, image, err := GetBindMount(ctx, tokens, contextDir, store, "", nil)
 			if err != nil {
-				return nil, mountedImages, targetLocks, err
+				return nil, mountedImages, nil, err
 			}
 			if _, ok := finalMounts[mount.Destination]; ok {
-				return nil, mountedImages, targetLocks, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
+				return nil, mountedImages, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
 			}
 			finalMounts[mount.Destination] = mount
 			mountedImages = append(mountedImages, image)
 		case TypeCache:
 			mount, tl, err := GetCacheMount(tokens, store, "", nil)
+			if err != nil {
+				return nil, mountedImages, nil, err
+			}
 			if tl != nil {
 				targetLocks = append(targetLocks, tl)
 			}
-			if err != nil {
-				return nil, mountedImages, targetLocks, err
-			}
 			if _, ok := finalMounts[mount.Destination]; ok {
-				return nil, mountedImages, targetLocks, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
+				return nil, mountedImages, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
 			}
 			finalMounts[mount.Destination] = mount
 		case TypeTmpfs:
 			mount, err := GetTmpfsMount(tokens)
 			if err != nil {
-				return nil, mountedImages, targetLocks, err
+				return nil, mountedImages, nil, err
 			}
 			if _, ok := finalMounts[mount.Destination]; ok {
-				return nil, mountedImages, targetLocks, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
+				return nil, mountedImages, nil, fmt.Errorf("%v: %w", mount.Destination, errDuplicateDest)
 			}
 			finalMounts[mount.Destination] = mount
 		default:
-			return nil, mountedImages, targetLocks, fmt.Errorf("invalid filesystem type %q", mountType)
+			return nil, mountedImages, nil, fmt.Errorf("invalid filesystem type %q", mountType)
 		}
 	}
 
+	succeeded = true
 	return finalMounts, mountedImages, targetLocks, nil
 }
 

diff --git a/run_common.go b/run_common.go
@@ -1256,6 +1256,7 @@ func init() {
 	reexec.Register(runUsingRuntimeCommand, runUsingRuntimeMain)
 }
 
+// If this succeeds, the caller must call cleanupMounts().
 func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath string, optionMounts []specs.Mount, bindFiles map[string]string, builtinVolumes, volumeMounts []string, runFileMounts []string, runMountInfo runMountInfo) (*runMountArtifacts, error) {
 	// Start building a new list of mounts.
 	var mounts []specs.Mount
@@ -1318,6 +1319,12 @@ func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath st
 	if err != nil {
 		return nil, err
 	}
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			internalParse.UnlockLockArray(mountArtifacts.TargetLocks)
+		}
+	}()
 	// Add temporary copies of the contents of volume locations at the
 	// volume locations, unless we already have something there.
 	builtins, err := runSetupBuiltinVolumes(b.MountLabel, mountPoint, cdir, builtinVolumes, int(rootUID), int(rootGID))
@@ -1353,6 +1360,7 @@ func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath st
 
 	// Set the list in the spec.
 	spec.Mounts = mounts
+	succeeded = true
 	return mountArtifacts, nil
 }
 
@@ -1444,6 +1452,8 @@ func cleanableDestinationListFromMounts(mounts []spec.Mount) []string {
 }
 
 // runSetupRunMounts sets up mounts that exist only in this RUN, not in subsequent runs
+//
+// If this function succeeds, the caller must unlock runMountArtifacts.TargetLocks (when??)
 func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMaps IDMaps) ([]spec.Mount, *runMountArtifacts, error) {
 	// If `type` is not set default to "bind"
 	mountType := internalParse.TypeBind
@@ -1455,6 +1465,12 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap
 	sshCount := 0
 	defaultSSHSock := ""
 	targetLocks := []lockfile.Locker{}
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			internalParse.UnlockLockArray(targetLocks)
+		}
+	}()
 	for _, mount := range mounts {
 		tokens := strings.Split(mount, ",")
 		for _, field := range tokens {
@@ -1526,6 +1542,7 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap
 			return nil, nil, fmt.Errorf("invalid mount type %q", mountType)
 		}
 	}
+	succeeded = true
 	artifacts := &runMountArtifacts{
 		RunMountTargets: mountTargets,
 		TmpFiles:        tmpFiles,
@@ -1864,8 +1881,6 @@ func (b *Builder) cleanupRunMounts(context *imageTypes.SystemContext, mountpoint
 		}
 	}
 	// unlock if any locked files from this RUN statement
-	for _, lockfile := range artifacts.TargetLocks {
-		lockfile.Unlock()
-	}
+	internalParse.UnlockLockArray(artifacts.TargetLocks)
 	return prevErr
 }
diff --git a/run_freebsd.go b/run_freebsd.go
@@ -309,6 +309,7 @@ func setupSpecialMountSpecChanges(spec *spec.Spec, shmSize string) ([]specs.Moun
 	return spec.Mounts, nil
 }
 
+// If this function succeeds and returns a non-nil lockfile.Locker, the caller must unlock it (when??).
 func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) {
 	return nil, nil, errors.New("cache mounts not supported on freebsd")
 }

diff --git a/run_linux.go b/run_linux.go
@@ -1197,18 +1197,25 @@ func checkIdsGreaterThan5(ids []spec.LinuxIDMapping) bool {
 	return false
 }
 
-// The caller should eventually unlock returned lockfile.Locker, if it is non-nil, even if this function fails.
+// If this function succeeds and returns a non-nil lockfile.Locker, the caller must unlock it (when??).
 func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) {
 	var optionMounts []specs.Mount
 	mount, targetLock, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints)
 	if err != nil {
-		return nil, targetLock, err
+		return nil, nil, err
 	}
+	succeeded := false
+	defer func() {
+		if !succeeded && targetLock != nil {
+			targetLock.Unlock()
+		}
+	}()
 	optionMounts = append(optionMounts, mount)
 	volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
 	if err != nil {
-		return nil, targetLock, err
+		return nil, nil, err
 	}
+	succeeded = true
 	return &volumes[0], targetLock, nil
 }