Fix: Introduce StagingLockfile to resolve overlay staging lock memory leak

drivers/overlay/overlay.go

@@ -23,14 +23,14 @@ import (
 	"github.com/containers/storage/drivers/overlayutils"
 	"github.com/containers/storage/drivers/quota"
 	"github.com/containers/storage/internal/dedup"
+	"github.com/containers/storage/internal/staging_lockfile"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chrootarchive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/fileutils"
 	"github.com/containers/storage/pkg/fsutils"
 	"github.com/containers/storage/pkg/idmap"
 	"github.com/containers/storage/pkg/idtools"
-	"github.com/containers/storage/pkg/lockfile"
 	"github.com/containers/storage/pkg/mount"
 	"github.com/containers/storage/pkg/parsers"
 	"github.com/containers/storage/pkg/system"
@@ -133,7 +133,7 @@ type Driver struct {
 	stagingDirsLocksMutex sync.Mutex
 	// stagingDirsLocks access is not thread safe, it is required that callers take
 	// stagingDirsLocksMutex on each access to guard against concurrent map writes.
-	stagingDirsLocks map[string]*lockfile.LockFile
+	stagingDirsLocks map[string]*staging_lockfile.StagingLockFile
 
 	supportsIDMappedMounts *bool
 }
@@ -442,7 +442,7 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error)
 		usingComposefs:        opts.useComposefs,
 		options:               *opts,
 		stagingDirsLocksMutex: sync.Mutex{},
-		stagingDirsLocks:      make(map[string]*lockfile.LockFile),
+		stagingDirsLocks:      make(map[string]*staging_lockfile.StagingLockFile),
 	}
 
 	d.naiveDiff = graphdriver.NewNaiveDiffDriver(d, graphdriver.NewNaiveLayerIDMapUpdater(d))
@@ -874,7 +874,9 @@ func (d *Driver) Cleanup() error {
 func (d *Driver) pruneStagingDirectories() bool {
 	d.stagingDirsLocksMutex.Lock()
 	for _, lock := range d.stagingDirsLocks {
-		lock.Unlock()
+		if err := lock.UnlockAndDelete(); err != nil {
+			logrus.Warnf("Failed to unlock and delete staging lock file: %v", err)
+		}
 	}
 	clear(d.stagingDirsLocks)
 	d.stagingDirsLocksMutex.Unlock()
@@ -886,17 +888,15 @@ func (d *Driver) pruneStagingDirectories() bool {
 	if err == nil {
 		for _, dir := range dirs {
 			stagingDirToRemove := filepath.Join(stagingDirBase, dir.Name())
-			lock, err := lockfile.GetLockFile(filepath.Join(stagingDirToRemove, stagingLockFile))
+			lock, err := staging_lockfile.TryLockPath(filepath.Join(stagingDirToRemove, stagingLockFile))
 			if err != nil {
 				anyPresent = true
 				continue
 			}
-			if err := lock.TryLock(); err != nil {
-				anyPresent = true
-				continue
-			}
 			_ = os.RemoveAll(stagingDirToRemove)
-			lock.Unlock()
+			if err := lock.UnlockAndDelete(); err != nil {
+				logrus.Warnf("Failed to unlock and delete staging lock file: %v", err)
+			}
 		}
 	}
 	return anyPresent
@@ -2178,7 +2178,10 @@ func (d *Driver) CleanupStagingDirectory(stagingDirectory string) error {
 	d.stagingDirsLocksMutex.Lock()
 	if lock, ok := d.stagingDirsLocks[parentStagingDir]; ok {
 		delete(d.stagingDirsLocks, parentStagingDir)
-		lock.Unlock()
+		if err := lock.UnlockAndDelete(); err != nil {
+			d.stagingDirsLocksMutex.Unlock()
+			return err
+		}
 	}
 	d.stagingDirsLocksMutex.Unlock()
 
@@ -2233,7 +2236,7 @@ func (d *Driver) ApplyDiffWithDiffer(options *graphdriver.ApplyDiffWithDifferOpt
 		return graphdriver.DriverWithDifferOutput{}, err
 	}
 
-	lock, err := lockfile.GetLockFile(filepath.Join(layerDir, stagingLockFile))
+	lock, err := staging_lockfile.TryLockPath(filepath.Join(layerDir, stagingLockFile))
 	if err != nil {
 		return graphdriver.DriverWithDifferOutput{}, err
 	}
@@ -2242,13 +2245,14 @@ func (d *Driver) ApplyDiffWithDiffer(options *graphdriver.ApplyDiffWithDifferOpt
 			d.stagingDirsLocksMutex.Lock()
 			delete(d.stagingDirsLocks, layerDir)
 			d.stagingDirsLocksMutex.Unlock()
-			lock.Unlock()
+			if err := lock.UnlockAndDelete(); err != nil {
+				errRet = errors.Join(errRet, err)
+			}
 		}
 	}()
 	d.stagingDirsLocksMutex.Lock()
 	d.stagingDirsLocks[layerDir] = lock
 	d.stagingDirsLocksMutex.Unlock()
-	lock.Lock()
 
 	logrus.Debugf("Applying differ in %s", applyDir)
 
@@ -2274,15 +2278,17 @@ func (d *Driver) ApplyDiffWithDiffer(options *graphdriver.ApplyDiffWithDifferOpt
 }
 
 // ApplyDiffFromStagingDirectory applies the changes using the specified staging directory.
-func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *graphdriver.DriverWithDifferOutput, options *graphdriver.ApplyDiffWithDifferOpts) error {
+func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *graphdriver.DriverWithDifferOutput, options *graphdriver.ApplyDiffWithDifferOpts) (errRet error) {
 	stagingDirectory := diffOutput.Target
 	parentStagingDir := filepath.Dir(stagingDirectory)
 
 	defer func() {
 		d.stagingDirsLocksMutex.Lock()
 		if lock, ok := d.stagingDirsLocks[parentStagingDir]; ok {
 			delete(d.stagingDirsLocks, parentStagingDir)
-			lock.Unlock()
+			if err := lock.UnlockAndDelete(); err != nil {
+				errRet = errors.Join(errRet, err)
+			}
 		}
 		d.stagingDirsLocksMutex.Unlock()
 	}()

internal/rawfilelock/rawfilelock.go

@@ -0,0 +1,64 @@
+package rawfilelock
+
+import (
+	"os"
+)
+
+type LockType byte
+
+const (
+	ReadLock LockType = iota
+	WriteLock
+)
+
+type FileHandle = fileHandle
+
+// OpenLock opens a file for locking
+// WARNING: This is the underlying file locking primitive of the OS;
+// because closing FileHandle releases the lock, it is not suitable for use
+// if there is any chance of two concurrent goroutines attempting to use the same lock.
+// Most users should use the higher-level operations from internal/staging_lockfile or pkg/lockfile.
+func OpenLock(path string, readOnly bool) (FileHandle, error) {
+	flags := os.O_CREATE
+	if readOnly {
+		flags |= os.O_RDONLY
+	} else {
+		flags |= os.O_RDWR
+	}
+
+	fd, err := openHandle(path, flags)
+	if err == nil {
+		return fd, nil
+	}
+
+	return fd, &os.PathError{Op: "open", Path: path, Err: err}
+}
+
+// TryLockFile attempts to lock a file handle
+func TryLockFile(fd FileHandle, lockType LockType) error {
+	return lockHandle(fd, lockType, true)
+}
+
+// LockFile locks a file handle
+func LockFile(fd FileHandle, lockType LockType) error {
+	return lockHandle(fd, lockType, false)
+}
+
+// UnlockAndClose unlocks and closes a file handle
+func UnlockAndCloseHandle(fd FileHandle) {
+	unlockAndCloseHandle(fd)
+}
+
+// CloseHandle closes a file handle without unlocking
+//
+// WARNING: This is a last-resort function for error handling only!
+// On Unix systems, closing a file descriptor automatically releases any locks,
+// so "closing without unlocking" is impossible. This function will release
+// the lock as a side effect of closing the file.
+//
+// This function should only be used in error paths where the lock state
+// is already corrupted or when giving up on lock management entirely.
+// Normal code should use UnlockAndCloseHandle instead.
+func CloseHandle(fd FileHandle) {
+	closeHandle(fd)
+}

internal/rawfilelock/rawfilelock_test.go

@@ -0,0 +1,86 @@
+package rawfilelock
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestOpenLock(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name    string
+		prepare func() (path string, readOnly bool)
+	}{
+		{
+			name: "file exists (read/write)",
+			prepare: func() (string, bool) {
+				tempFile, err := os.CreateTemp(t.TempDir(), "lock-")
+				require.NoError(t, err)
+				tempFile.Close()
+				return tempFile.Name(), false
+			},
+		},
+		{
+			name: "file exists readonly (readonly)",
+			prepare: func() (string, bool) {
+				tempFile, err := os.CreateTemp(t.TempDir(), "lock-")
+				require.NoError(t, err)
+				tempFile.Close()
+				return tempFile.Name(), true
+			},
+		},
+		{
+			name: "base dir exists (read/write)",
+			prepare: func() (string, bool) {
+				tempDir := os.TempDir()
+				require.DirExists(t, tempDir)
+				return filepath.Join(tempDir, "test-1.lock"), false
+			},
+		},
+	} {
+		path, readOnly := tc.prepare()
+
+		fd, err := OpenLock(path, readOnly)
+		require.NoError(t, err, tc.name)
+		UnlockAndCloseHandle(fd)
+
+		fd, err = OpenLock(path, readOnly)
+		require.NoError(t, err)
+		UnlockAndCloseHandle(fd)
+
+		require.Nil(t, os.RemoveAll(path))
+	}
+}
+
+func TestOpenLockNotCreateParentDir(t *testing.T) {
+	tmpDir := t.TempDir()
+	lockPath := filepath.Join(tmpDir, "lockfile")
+	fd, err := OpenLock(lockPath, false)
+	require.NoError(t, err)
+	UnlockAndCloseHandle(fd)
+
+	lockPath = filepath.Join(tmpDir, "subdir", "lockfile")
+	_, err = OpenLock(lockPath, false)
+	require.Error(t, err)
+}
+
+func TestTryLockFileAndLockFile(t *testing.T) {
+	tmpFile, err := os.CreateTemp(t.TempDir(), "lockfile")
+	require.NoError(t, err)
+	defer os.Remove(tmpFile.Name())
+	fd, err := OpenLock(tmpFile.Name(), false)
+	require.NoError(t, err)
+
+	require.NoError(t, TryLockFile(fd, WriteLock))
+	UnlockAndCloseHandle(fd)
+
+	fd2, err := OpenLock(tmpFile.Name(), false)
+	require.NoError(t, err)
+
+	require.NoError(t, LockFile(fd2, WriteLock))
+	UnlockAndCloseHandle(fd2)
+}

internal/rawfilelock/rawfilelock_unix.go

@@ -0,0 +1,49 @@
+//go:build !windows
+
+package rawfilelock
+
+import (
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+type fileHandle uintptr
+
+func openHandle(path string, mode int) (fileHandle, error) {
+	mode |= unix.O_CLOEXEC
+	fd, err := unix.Open(path, mode, 0o644)
+	return fileHandle(fd), err
+}
+
+func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error {
+	fType := unix.F_RDLCK
+	if lType != ReadLock {
+		fType = unix.F_WRLCK
+	}
+	lk := unix.Flock_t{
+		Type:   int16(fType),
+		Whence: int16(unix.SEEK_SET),
+		Start:  0,
+		Len:    0,
+	}
+	cmd := unix.F_SETLKW
+	if nonblocking {
+		cmd = unix.F_SETLK
+	}
+	for {
+		err := unix.FcntlFlock(uintptr(fd), cmd, &lk)
+		if err == nil || nonblocking {
+			return err
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+}
+
+func unlockAndCloseHandle(fd fileHandle) {
+	unix.Close(int(fd))
+}
+
+func closeHandle(fd fileHandle) {
+	unix.Close(int(fd))
+}

internal/rawfilelock/rawfilelock_windows.go

@@ -0,0 +1,48 @@
+//go:build windows
+
+package rawfilelock
+
+import (
+	"golang.org/x/sys/windows"
+)
+
+const (
+	reserved = 0
+	allBytes = ^uint32(0)
+)
+
+type fileHandle windows.Handle
+
+func openHandle(path string, mode int) (fileHandle, error) {
+	mode |= windows.O_CLOEXEC
+	fd, err := windows.Open(path, mode, windows.S_IWRITE)
+	return fileHandle(fd), err
+}
+
+func lockHandle(fd fileHandle, lType LockType, nonblocking bool) error {
+	flags := 0
+	if lType != ReadLock {
+		flags = windows.LOCKFILE_EXCLUSIVE_LOCK
+	}
+	if nonblocking {
+		flags |= windows.LOCKFILE_FAIL_IMMEDIATELY
+	}
+	ol := new(windows.Overlapped)
+	if err := windows.LockFileEx(windows.Handle(fd), uint32(flags), reserved, allBytes, allBytes, ol); err != nil {
+		if nonblocking {
+			return err
+		}
+		panic(err)
+	}
+	return nil
+}
+
+func unlockAndCloseHandle(fd fileHandle) {
+	ol := new(windows.Overlapped)
+	windows.UnlockFileEx(windows.Handle(fd), reserved, allBytes, allBytes, ol)
+	closeHandle(fd)
+}
+
+func closeHandle(fd fileHandle) {
+	windows.Close(windows.Handle(fd))
+}