From 4edfb3ef65ee2522f5b09c88793095ebee886bab Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 7 Nov 2018 00:35:09 +0100 Subject: [PATCH] [Windows] Handle file renaming when the destination path already exists (#6415) On Linux, you can invoke os.rename(src, dst) even if dst already exists. In this case, destination file will be atomically replaced by the source file. On Windows, this will lead to an OSError because changes are not atomic. This cause certbot renew to fail in particular, because the old certificate configuration needs to be replace by the new when a certificate is effectively renewed. One could use the cross-platform function os.replace, but it is available only on Python >= 3.3. This PR add a function in compat to handle correctly this case on Windows, and delegating everything else to os.rename. * Cross platform compatible os.rename (we can use os.replace if its python 3) * Use os.replace instead of custom non-atomic code. * Avoid errors for lint and mypy. Add a test. --- certbot/compat.py | 24 ++++++++++++++++++++++++ certbot/reverter.py | 2 +- certbot/storage.py | 3 ++- certbot/tests/compat_test.py | 21 +++++++++++++++++++++ certbot/tests/reverter_test.py | 2 +- 5 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 certbot/tests/compat_test.py diff --git a/certbot/compat.py b/certbot/compat.py index e3e1bc4e1..d42febe81 100644 --- a/certbot/compat.py +++ b/certbot/compat.py @@ -65,6 +65,30 @@ def os_geteuid(): # Windows specific return 0 +def os_rename(src, dst): + """ + Rename a file to a destination path and handles situations where the destination exists. + + :param str src: The current file path. + :param str dst: The new file path. + """ + try: + os.rename(src, dst) + except OSError as err: + # Windows specific, renaming a file on an existing path is not possible. + # On Python 3, the best fallback with atomic capabilities we have is os.replace. + if err.errno != errno.EEXIST: + # Every other error is a legitimate exception. + raise + if not hasattr(os, 'replace'): # pragma: no cover + # We should never go on this line. Either we are on Linux and os.rename has succeeded, + # either we are on Windows, and only Python >= 3.4 is supported where os.replace is + # available. + raise RuntimeError('Error: tried to run os_rename on Python < 3.3. ' + 'Certbot supports only Python 3.4 >= on Windows.') + getattr(os, 'replace')(src, dst) + + def readline_with_timeout(timeout, prompt): """ Read user input to return the first line entered, or raise after specified timeout. diff --git a/certbot/reverter.py b/certbot/reverter.py index 5d56615fd..919037358 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -576,7 +576,7 @@ class Reverter(object): timestamp = self._checkpoint_timestamp() final_dir = os.path.join(self.config.backup_dir, timestamp) try: - os.rename(self.config.in_progress_dir, final_dir) + compat.os_rename(self.config.in_progress_dir, final_dir) return except OSError: logger.warning("Extreme, unexpected race condition, retrying (%s)", timestamp) diff --git a/certbot/storage.py b/certbot/storage.py index c16ea35b8..4b8110072 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -14,6 +14,7 @@ import six import certbot from certbot import cli +from certbot import compat from certbot import constants from certbot import crypto_util from certbot import errors @@ -188,7 +189,7 @@ def update_configuration(lineagename, archive_dir, target, cli_config): # Save only the config items that are relevant to renewal values = relevant_values(vars(cli_config.namespace)) write_renewal_config(config_filename, temp_filename, archive_dir, target, values) - os.rename(temp_filename, config_filename) + compat.os_rename(temp_filename, config_filename) return configobj.ConfigObj(config_filename) diff --git a/certbot/tests/compat_test.py b/certbot/tests/compat_test.py new file mode 100644 index 000000000..552aa5645 --- /dev/null +++ b/certbot/tests/compat_test.py @@ -0,0 +1,21 @@ +"""Tests for certbot.compat.""" +import os + +from certbot import compat +import certbot.tests.util as test_util + +class OsReplaceTest(test_util.TempDirTestCase): + """Test to ensure consistent behavior of os_rename method""" + + def test_os_rename_to_existing_file(self): + """Ensure that os_rename will effectively rename src into dst for all platforms.""" + src = os.path.join(self.tempdir, 'src') + dst = os.path.join(self.tempdir, 'dst') + open(src, 'w').close() + open(dst, 'w').close() + + # On Windows, a direct call to os.rename will fail because dst already exists. + compat.os_rename(src, dst) + + self.assertFalse(os.path.exists(src)) + self.assertTrue(os.path.exists(dst)) diff --git a/certbot/tests/reverter_test.py b/certbot/tests/reverter_test.py index 999c6225c..d04e3c641 100644 --- a/certbot/tests/reverter_test.py +++ b/certbot/tests/reverter_test.py @@ -356,7 +356,7 @@ class TestFullCheckpointsReverter(test_util.ConfigTestCase): self.assertRaises( errors.ReverterError, self.reverter.finalize_checkpoint, "Title") - @mock.patch("certbot.reverter.os.rename") + @mock.patch("certbot.reverter.compat.os_rename") def test_finalize_checkpoint_no_rename_directory(self, mock_rename): self.reverter.add_to_checkpoint(self.sets[0], "perm save")