From 6c8fb4180b663121b673013ae30d39570c8dd4da Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 10 Feb 2021 12:47:07 +0000 Subject: [PATCH 1/5] storage/reflink: quote logged filenames --- qubes/storage/reflink.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 8b9bfe0d..408a2748 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -388,7 +388,7 @@ def _make_dir(path): with suppress(FileExistsError): os.mkdir(path) _fsync_path(os.path.dirname(path)) - LOGGER.info('Created directory: %s', path) + LOGGER.info('Created directory: %r', path) return True return False @@ -396,13 +396,13 @@ def _remove_file(path): with suppress(FileNotFoundError): os.remove(path) _fsync_path(os.path.dirname(path)) - LOGGER.info('Removed file: %s', path) + LOGGER.info('Removed file: %r', path) def _remove_empty_dir(path): try: os.rmdir(path) _fsync_path(os.path.dirname(path)) - LOGGER.info('Removed empty directory: %s', path) + LOGGER.info('Removed empty directory: %r', path) except OSError as ex: if ex.errno not in (errno.ENOENT, errno.ENOTEMPTY): raise @@ -414,7 +414,7 @@ def _rename_file(src, dst): _fsync_path(dst_dir) if src_dir != dst_dir: _fsync_path(src_dir) - LOGGER.info('Renamed file: %s -> %s', src, dst) + LOGGER.info('Renamed file: %r -> %r', src, dst) def _resize_file(path, size): ''' Resize an existing file. ''' @@ -426,7 +426,7 @@ def _create_sparse_file(path, size): ''' Create an empty sparse file. ''' with _replace_file(path) as tmp: tmp.truncate(size) - LOGGER.info('Created sparse file: %s', tmp.name) + LOGGER.info('Created sparse file: %r', tmp.name) def _update_loopdev_sizes(img): ''' Resolve img; update the size of loop devices backed by it. ''' @@ -456,9 +456,9 @@ def _copy_file(src, dst): with _replace_file(dst) as tmp_io: with open(src, 'rb') as src_io: if _attempt_ficlone(src_io, tmp_io): - LOGGER.info('Reflinked file: %s -> %s', src, tmp_io.name) + LOGGER.info('Reflinked file: %r -> %r', src, tmp_io.name) return True - LOGGER.info('Copying file: %s -> %s', src, tmp_io.name) + LOGGER.info('Copying file: %r -> %r', src, tmp_io.name) cmd = 'cp', '--sparse=always', src, tmp_io.name p = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=False) From c988a2218b809c4216174f41367a467a8d29c2ac Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 10 Feb 2021 12:47:09 +0000 Subject: [PATCH 2/5] utils: take tweaked helper functions from storage/reflink replace_file(), rename_file(), and remove_file() now have optional 'logger' and 'log_level' (defaulting to DEBUG) arguments. replace_file() now has a required 'permissions' and an optional 'close_on_success' (defaulting to True) argument. Also, it doesn't create any directories; and in case of an exception, the tempfile is removed even when closing it raises another exception. remove_file() now returns a value: True if the file was removed, or False if it already didn't exist. (fsync_path() is unchanged.) !!! After cherry-picking for release4.0, consider a fixup !!! !!! adding 'import qubes.utils' to storage/reflink there !!! --- qubes/storage/reflink.py | 55 +++++++++---------------------------- qubes/utils.py | 58 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/qubes/storage/reflink.py b/qubes/storage/reflink.py index 408a2748..abd688c7 100644 --- a/qubes/storage/reflink.py +++ b/qubes/storage/reflink.py @@ -32,7 +32,7 @@ import logging import os import subprocess import tempfile -from contextlib import contextmanager, suppress +from contextlib import suppress import qubes.storage import qubes.utils @@ -224,7 +224,7 @@ class ReflinkVolume(qubes.storage.Volume): def _commit(self, path_from): self._add_revision() self._prune_revisions() - _fsync_path(path_from) + qubes.utils.fsync_path(path_from) _rename_file(path_from, self._path_clean) def _add_revision(self): @@ -354,32 +354,16 @@ class ReflinkVolume(qubes.storage.Volume): return 0 -@contextmanager def _replace_file(dst): - ''' Yield a tempfile whose name starts with dst, creating the last - directory component if necessary. If the block does not raise - an exception, safely rename the tempfile to dst. - ''' - tmp_dir, prefix = os.path.split(dst + '~') - _make_dir(tmp_dir) - tmp = tempfile.NamedTemporaryFile(dir=tmp_dir, prefix=prefix, delete=False) - try: - yield tmp - tmp.flush() - os.fsync(tmp.fileno()) - tmp.close() - _rename_file(tmp.name, dst) - except: - tmp.close() - _remove_file(tmp.name) - raise + _make_dir(os.path.dirname(dst)) + return qubes.utils.replace_file( + dst, permissions=0o600, log_level=logging.INFO) -def _fsync_path(path): - fd = os.open(path, os.O_RDONLY) # works for a file or a directory - try: - os.fsync(fd) - finally: - os.close(fd) +_rename_file = functools.partial( + qubes.utils.rename_file, log_level=logging.INFO) + +_remove_file = functools.partial( + qubes.utils.remove_file, log_level=logging.INFO) def _make_dir(path): ''' mkdir path, ignoring FileExistsError; return whether we @@ -387,35 +371,20 @@ def _make_dir(path): ''' with suppress(FileExistsError): os.mkdir(path) - _fsync_path(os.path.dirname(path)) + qubes.utils.fsync_path(os.path.dirname(path)) LOGGER.info('Created directory: %r', path) return True return False -def _remove_file(path): - with suppress(FileNotFoundError): - os.remove(path) - _fsync_path(os.path.dirname(path)) - LOGGER.info('Removed file: %r', path) - def _remove_empty_dir(path): try: os.rmdir(path) - _fsync_path(os.path.dirname(path)) + qubes.utils.fsync_path(os.path.dirname(path)) LOGGER.info('Removed empty directory: %r', path) except OSError as ex: if ex.errno not in (errno.ENOENT, errno.ENOTEMPTY): raise -def _rename_file(src, dst): - os.rename(src, dst) - dst_dir = os.path.dirname(dst) - src_dir = os.path.dirname(src) - _fsync_path(dst_dir) - if src_dir != dst_dir: - _fsync_path(src_dir) - LOGGER.info('Renamed file: %r -> %r', src, dst) - def _resize_file(path, size): ''' Resize an existing file. ''' with open(path, 'rb+') as file: diff --git a/qubes/utils.py b/qubes/utils.py index b6dd00df..fbb176fd 100644 --- a/qubes/utils.py +++ b/qubes/utils.py @@ -22,12 +22,16 @@ import asyncio import hashlib +import logging import random import string import os +import os.path import re import socket import subprocess +import tempfile +from contextlib import contextmanager, suppress import pkg_resources @@ -36,6 +40,8 @@ import docutils.core import docutils.io import qubes.exc +LOGGER = logging.getLogger('qubes.utils') + def get_timezone(): # fc18 @@ -186,6 +192,58 @@ def match_vm_name_with_special(vm, name): return name[len('@type:'):] == vm.__class__.__name__ return name == vm.name +@contextmanager +def replace_file(dst, *, permissions, close_on_success=True, + logger=LOGGER, log_level=logging.DEBUG): + ''' Yield a tempfile whose name starts with dst. If the block does + not raise an exception, apply permissions and persist the + tempfile to dst (which is allowed to already exist). Otherwise + ensure that the tempfile is cleaned up. + ''' + tmp_dir, prefix = os.path.split(dst + '~') + tmp = tempfile.NamedTemporaryFile(dir=tmp_dir, prefix=prefix, delete=False) + try: + yield tmp + tmp.flush() + os.fchmod(tmp.fileno(), permissions) + os.fsync(tmp.fileno()) + if close_on_success: + tmp.close() + rename_file(tmp.name, dst, logger=logger, log_level=log_level) + except: + try: + tmp.close() + finally: + remove_file(tmp.name, logger=logger, log_level=log_level) + raise + +def rename_file(src, dst, *, logger=LOGGER, log_level=logging.DEBUG): + ''' Durably rename src to dst. ''' + os.rename(src, dst) + dst_dir = os.path.dirname(dst) + src_dir = os.path.dirname(src) + fsync_path(dst_dir) + if src_dir != dst_dir: + fsync_path(src_dir) + logger.log(log_level, 'Renamed file: %r -> %r', src, dst) + +def remove_file(path, *, logger=LOGGER, log_level=logging.DEBUG): + ''' Durably remove the file at path, if it exists. Return whether + we removed it. ''' + with suppress(FileNotFoundError): + os.remove(path) + fsync_path(os.path.dirname(path)) + logger.log(log_level, 'Removed file: %r', path) + return True + return False + +def fsync_path(path): + fd = os.open(path, os.O_RDONLY) # works for a file or a directory + try: + os.fsync(fd) + finally: + os.close(fd) + @asyncio.coroutine def coro_maybe(value): if asyncio.iscoroutine(value): From 7159f206a5c0c874742bce60dfae926f9796f04d Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 10 Feb 2021 12:58:00 +0000 Subject: [PATCH 3/5] firewall: save firewall.xml with utils.replace_file() Don't rewrite the file in-place. Also change the error message from 'save error' to 'firewall save error'. --- qubes/firewall.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/qubes/firewall.py b/qubes/firewall.py index 6f70c8f1..b3b0a3af 100644 --- a/qubes/firewall.py +++ b/qubes/firewall.py @@ -31,6 +31,7 @@ import asyncio import lxml.etree import qubes +import qubes.utils import qubes.vm.qubesvm @@ -577,14 +578,13 @@ class Firewall: xml_tree = lxml.etree.ElementTree(xml_root) try: - old_umask = os.umask(0o002) - with open(firewall_conf, 'wb') as firewall_xml: - xml_tree.write(firewall_xml, encoding="UTF-8", - pretty_print=True) - os.umask(old_umask) + with qubes.utils.replace_file(firewall_conf, + permissions=0o664) as tmp_io: + xml_tree.write(tmp_io, encoding='UTF-8', pretty_print=True) except EnvironmentError as err: - self.vm.log.error("save error: {}".format(err)) - raise qubes.exc.QubesException('save error: {}'.format(err)) + msg='firewall save error: {}'.format(err) + self.vm.log.error(msg) + raise qubes.exc.QubesException(msg) self.vm.fire_event('firewall-changed') From 9b6d082673770710be661ba494c7c4b67d88820e Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 10 Feb 2021 12:58:01 +0000 Subject: [PATCH 4/5] app: use suppress() in simple cases --- qubes/app.py | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/qubes/app.py b/qubes/app.py index 5f0cd97c..f6cc3a92 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -33,6 +33,7 @@ import tempfile import time import traceback import uuid +from contextlib import suppress import asyncio import jinja2 @@ -280,11 +281,9 @@ class QubesHost: self.app.log.debug('QubesHost: no_cpus={} memory_total={}'.format( self.no_cpus, self.memory_total)) - try: + with suppress(NotImplementedError): self.app.log.debug('QubesHost: xen_free_memory={}'.format( self.get_free_xen_memory())) - except NotImplementedError: - pass @property def memory_total(self): @@ -1324,10 +1323,8 @@ class Qubes(qubes.PropertyHolder): """ # first search for index, verbatim - try: + with suppress(KeyError): return self.labels[label] - except KeyError: - pass # then search for name for i in self.labels.values(): @@ -1335,10 +1332,8 @@ class Qubes(qubes.PropertyHolder): return i # last call, if label is a number represented as str, search in indices - try: + with suppress(KeyError, ValueError): return self.labels[int(label)] - except (KeyError, ValueError): - pass raise qubes.exc.QubesLabelNotFoundError(label) @@ -1477,7 +1472,7 @@ class Qubes(qubes.PropertyHolder): # allow removed VM to reference itself continue for prop in obj.property_list(): - try: + with suppress(AttributeError): if isinstance(prop, qubes.vm.VMProperty) and \ getattr(obj, prop.__name__) == vm: self.log.error( @@ -1489,8 +1484,6 @@ class Qubes(qubes.PropertyHolder): 'see /var/log/qubes/qubes.log in dom0 for ' 'details'.format( vm.name)) - except AttributeError: - pass assignments = vm.get_provided_assignments() if assignments: @@ -1512,11 +1505,9 @@ class Qubes(qubes.PropertyHolder): 'updatevm', 'default_template', ): - try: + with suppress(AttributeError): if getattr(self, propname) == vm: delattr(self, propname) - except AttributeError: - pass @qubes.events.handler('property-pre-set:clockvm') def on_property_pre_set_clockvm(self, event, name, newvalue, oldvalue=None): From 12d117b20a81dcb5324fca247396704f38efa688 Mon Sep 17 00:00:00 2001 From: Rusty Bird Date: Wed, 10 Feb 2021 12:58:02 +0000 Subject: [PATCH 5/5] app: save qubes.xml with utils.replace_file() That takes care of the missing fsync() calls. Fixes QubesOS/qubes-issues#3376 --- qubes/app.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/qubes/app.py b/qubes/app.py index f6cc3a92..3d512d35 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -29,7 +29,6 @@ import logging import os import random import sys -import tempfile import time import traceback import uuid @@ -1102,18 +1101,12 @@ class Qubes(qubes.PropertyHolder): if not self.__locked_fh: self._acquire_lock(for_save=True) - fh_new = tempfile.NamedTemporaryFile( - prefix=self._store, delete=False) - lxml.etree.ElementTree(self.__xml__()).write( - fh_new, encoding='utf-8', pretty_print=True) - fh_new.flush() - try: - os.chown(fh_new.name, -1, grp.getgrnam('qubes').gr_gid) - os.chmod(fh_new.name, 0o660) - except KeyError: # group 'qubes' not found - # don't change mode if no 'qubes' group in the system - pass - os.rename(fh_new.name, self._store) + with qubes.utils.replace_file(self._store, permissions=0o660, + close_on_success=False) as fh_new: + lxml.etree.ElementTree(self.__xml__()).write( + fh_new, encoding='utf-8', pretty_print=True) + with suppress(KeyError): # group not found + os.fchown(fh_new.fileno(), -1, grp.getgrnam('qubes').gr_gid) # update stored mtime, in case of multiple save() calls without # loading qubes.xml again