From 1074668ede14345678fe8220814a8fed1d432475 Mon Sep 17 00:00:00 2001 From: Umang Jain Date: Wed, 27 Jun 2018 02:41:43 +0530 Subject: [PATCH] lib/repo: cleanup_tmpdir should be executed after releasing lock file Here's a subtle bug in abort_transaction(): One of the policies of cleaning up is to skip the current boot's staging directory. The responsible function for this is cleanup_tmpdir() which tries to lock each of the tmpdir before deleting it. When it comes to the current boot's staging dir, it tries to lock the directory(again!) but fails as there is already a lockfile present. Just because the current boot's staging dir was meant to be skipped, the bug never surfaced up and wasn't catastrohpic. if (!_ostree_repo_try_lock_tmpdir (dfd, path, &lockfile, &did_lock, error)) return FALSE; if (!did_lock) return TRUE; /* Note early return */ ... if (g_str_has_prefix (path, self->stagedir_prefix)) return TRUE; /* Note early return */ The actual check for skipping staging dir for current boot was never reached because the function returned at did_lock failure. Therefore, execute cleanup_tmpdir() after releasing the lockfile in abort_transaction() so that cleanup_tmpdir gets a chance to lock current boot's staging directory and succeed. Closes: #1602 Approved by: jlebon --- src/libostree/ostree-repo-commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 2516ba3d..ce2f4deb 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2229,10 +2229,6 @@ ostree_repo_abort_transaction (OstreeRepo *self, if (!self->in_transaction) return TRUE; - /* Do not propagate failures from cleanup_tmpdir() immediately, as we want - * to clean up the rest of the internal transaction state first. */ - cleanup_tmpdir (self, cancellable, &cleanup_error); - if (self->loose_object_devino_hash) g_hash_table_remove_all (self->loose_object_devino_hash); @@ -2242,6 +2238,10 @@ ostree_repo_abort_transaction (OstreeRepo *self, glnx_tmpdir_unset (&self->commit_stagedir); glnx_release_lock_file (&self->commit_stagedir_lock); + /* Do not propagate failures from cleanup_tmpdir() immediately, as we want + * to clean up the rest of the internal transaction state first. */ + cleanup_tmpdir (self, cancellable, &cleanup_error); + self->in_transaction = FALSE; if (self->txn_locked)