Skip to content

libpod: report locks that DeallocateAllLocks failed to remove - #29532

Draft
nishantbkl3345-ship-it wants to merge 1 commit into
podman-container-tools:mainfrom
nishantbkl3345-ship-it:fix-file-lock-deallocate-all-errors
Draft

libpod: report locks that DeallocateAllLocks failed to remove#29532
nishantbkl3345-ship-it wants to merge 1 commit into
podman-container-tools:mainfrom
nishantbkl3345-ship-it:fix-file-lock-deallocate-all-errors

Conversation

@nishantbkl3345-ship-it

Copy link
Copy Markdown

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 not helpful either: "Deallocating lock %s" is formatted with the path only. It never mentions that anything failed and drops err, 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 can still report success and go on to allocate new locks. The file backend is the default on FreeBSD, and it is selected on Linux by lock_type = "file" in containers.conf.

The unused lastErr dates back to commit 827ac0859f96 (lock: new lock type "file"), which added the backend.

Keep the newest failure in lastErr and return it, wrapped with the path so the caller can identify which lock is stuck, and log any failure it supersedes. This follows the same lastErr pattern used 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 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 are 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 both root and rootless test environments.

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

Does this PR introduce a user-facing change?

podman system renumber now reports an error when the file lock backend cannot remove a lock file, instead of reporting success.

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 827ac08 ("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 <nishantbkl3345-ship-it@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant