From abd1c6d49f2b5c4c97a52f29fe5b9828402d8157 Mon Sep 17 00:00:00 2001 From: Ali Mirjamali Date: Fri, 18 Apr 2025 00:29:13 +0330 Subject: [PATCH] Assure backup restore/verify tmpdir is properly deleted The temporary directories remain when either: - The operation is cancelled at the summary screen (before actually starting restore/verify) - The operation fails early in the `restore_do` function. For example in `check_disk_space` or `_restore_vms_metadata`. Since deleting tmp directory in `restore_do` does not work properly (this is confirmed by the unittests), we check for its existence again once more in `__del__` destructor and delete it if it still exists. fixes: https://github.com/QubesOS/qubes-issues/issues/9387 --- qubesadmin/backup/restore.py | 11 ++++++++++- qubesadmin/tests/backup/__init__.py | 4 ++++ qubesadmin/tests/backup/backupcompatibility.py | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/qubesadmin/backup/restore.py b/qubesadmin/backup/restore.py index db8c2250..1a7f2188 100644 --- a/qubesadmin/backup/restore.py +++ b/qubesadmin/backup/restore.py @@ -956,7 +956,7 @@ def __init__(self, app, backup_location, backup_vm, passphrase, *, # due to possible location in ~/QubesIncoming, the prefix should not be # a valid VM name os.makedirs(tmpdir, exist_ok=True) - self.tmpdir = tempfile.mkdtemp(prefix="backup#restore-", dir=tmpdir) + self.tmpdir = tempfile.mkdtemp(prefix=".backup#restore-", dir=tmpdir) #: list of processes (Popen objects) to kill on cancel self.processes_to_kill_on_cancel = [] @@ -977,6 +977,15 @@ def __init__(self, app, backup_location, backup_vm, passphrase, *, #: VMs included in the backup self.backup_app = self._process_qubes_xml() + def __del__(self): + """Since deleting tmp directory in `restore_do` does not work properly + (this is confirmed by the unittests), we check for its existence again + once more and delete it if it still exists. + """ + if self.log.getEffectiveLevel() > logging.DEBUG: + if os.path.exists(self.tmpdir): + shutil.rmtree(self.tmpdir) + def _start_retrieval_process(self, filelist, limit_count, limit_bytes): """Retrieve backup stream and extract it to :py:attr:`tmpdir` diff --git a/qubesadmin/tests/backup/__init__.py b/qubesadmin/tests/backup/__init__.py index 14164332..dfc73fd0 100644 --- a/qubesadmin/tests/backup/__init__.py +++ b/qubesadmin/tests/backup/__init__.py @@ -193,6 +193,10 @@ def restore_backup(self, source=None, appvm=None, options=None, '\n'.join(errors)) if not appvm and not os.path.isdir(backupfile): os.unlink(backupfile) + tmpdir = getattr(restore_op, "tmpdir", None) + del restore_op + if tmpdir: + self.assertFalse(os.path.exists(tmpdir)) def create_sparse(self, path, size, signature=b''): with open(path, "wb") as f_img: diff --git a/qubesadmin/tests/backup/backupcompatibility.py b/qubesadmin/tests/backup/backupcompatibility.py index 3c5dab88..c303b050 100644 --- a/qubesadmin/tests/backup/backupcompatibility.py +++ b/qubesadmin/tests/backup/backupcompatibility.py @@ -1905,7 +1905,7 @@ def test_230_r4_compressed(self): @unittest.skipUnless(shutil.which('scrypt'), "scrypt not installed") - def test_230_r4_custom_cmpression(self): + def test_230_r4_custom_compression(self): self.create_v4_backup("bzip2") self.app.expected_calls[('dom0', 'admin.vm.List', None, None)] = (