From 0afbb4a4b01cd2b1743c708e3468e3a62467307b Mon Sep 17 00:00:00 2001 From: nishantbkl3345-ship-it Date: Sat, 15 Aug 2026 15:14:10 +0530 Subject: [PATCH] libpod: report locks that DeallocateAllLocks failed to remove DeallocateAllLocks() declares lastErr and returns it, but never assigns to it. When os.Remove() fails on a lock file the function logs a line and carries on, so it always returns nil and the caller is told every lock was freed. The log line is no help either: "Deallocating lock %s" is formatted with the path only. It never mentions that anything failed and it drops err on the floor, so the reason - EACCES, EROFS, EBUSY, ENOTEMPTY - is lost. The user-visible effect is on "podman system renumber", which calls FreeAllLocks() before it reassigns lock IDs (libpod/runtime_renumber.go). With the file lock backend a renumber that could not clear the old lock directory still reports success and goes on to allocate new locks, and the only trace is a log line that does not say anything went wrong. The file backend is not exotic: it is the default on FreeBSD, and it is selected on Linux by lock_type = "file" in containers.conf. Both problems date back to commit 827ac0859f96 ("lock: new lock type \"file\"") which added the backend with lastErr already unused. Keep the newest failure in lastErr and return it, wrapped with the path so the caller can say which lock is stuck, and log any failure it supersedes. This is the same shape used for lastErr elsewhere in libpod, for example in ExecHTTPStartAndAttach() in libpod/container_exec.go. Removal still continues over the remaining locks, and a lock file that is already gone is still skipped, so a successful deallocation is unchanged. The test makes one entry of the lock directory impossible to remove and checks that DeallocateAllLocks() returns an error naming it, and that the other locks were freed regardless. A non-empty directory is used to provoke the failure because os.Remove() rejects it with ENOTEMPTY rather than a permission error, which keeps the test working in the root unit test job as well as the rootless one. Signed-off-by: nishantbkl3345-ship-it --- libpod/lock/file/file_lock.go | 5 ++++- libpod/lock/file/file_lock_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/libpod/lock/file/file_lock.go b/libpod/lock/file/file_lock.go index 27f1023a6f9..200305a3b88 100644 --- a/libpod/lock/file/file_lock.go +++ b/libpod/lock/file/file_lock.go @@ -143,7 +143,10 @@ func (locks *FileLocks) DeallocateAllLocks() error { if errors.Is(err, fs.ErrNotExist) { continue } - logrus.Errorf("Deallocating lock %s", p) + if lastErr != nil { + logrus.Errorf("Deallocating locks: %v", lastErr) + } + lastErr = fmt.Errorf("deallocating lock %s: %w", p, err) } } return lastErr diff --git a/libpod/lock/file/file_lock_test.go b/libpod/lock/file/file_lock_test.go index 6fe2e10e3b4..b8349cd0a09 100644 --- a/libpod/lock/file/file_lock_test.go +++ b/libpod/lock/file/file_lock_test.go @@ -42,6 +42,36 @@ func TestCreateAndDeallocate(t *testing.T) { assert.NoError(t, err) } +// Test that DeallocateAllLocks reports a lock it could not remove instead of +// returning success. +func TestDeallocateAllLocksError(t *testing.T) { + d := t.TempDir() + + lockDir := filepath.Join(d, "locks") + l, err := CreateFileLock(lockDir) + assert.NoError(t, err) + + lock, err := l.AllocateLock() + assert.NoError(t, err) + + // Make one entry impossible to remove. os.Remove refuses to remove a + // non-empty directory, which fails no matter which user the tests run + // as - a permission-based failure would be skipped by root. The name + // sorts before the numeric lock files so that DeallocateAllLocks(), + // which walks the directory in sorted order, hits it first. + stuck := filepath.Join(lockDir, "!stuck") + assert.NoError(t, os.Mkdir(stuck, 0o700)) + assert.NoError(t, os.WriteFile(filepath.Join(stuck, "child"), nil, 0o600)) + + err = l.DeallocateAllLocks() + assert.ErrorContains(t, err, stuck) + + // The failure must not stop the remaining locks from being deallocated. + // AllocateGivenLock() creates the lock file with O_EXCL, so it only + // succeeds if the lock really was removed. + assert.NoError(t, l.AllocateGivenLock(lock)) +} + // Test that creating and destroying locks work func TestLockAndUnlock(t *testing.T) { d := t.TempDir()