Make sure nothing is changed if a rename failed

- rename is implemented via remove/add, a failed add shall revert the remove
- see #643

Author: mrbean-bremen

CHANGES.md

@@ -11,6 +11,9 @@ The released versions correspond to PyPi releases.
 * fixed handling of alternative path separator in `os.path.split`,
   `os.path.splitdrive` and `glob.glob`
   (see [#632](../../issues/632))
+* fixed handling of failed rename due to permission error
+  (see [#643](../../issues/643))
+
 
 ## [Version 4.5.1](https://pypi.python.org/pypi/pyfakefs/4.5.1) (2021-08-29)
 This is a bugfix release.

pyfakefs/fake_filesystem.py

@@ -2257,14 +2257,27 @@ def rename(self, old_file_path: AnyPath,
         if new_dir_object.has_parent_object(old_object):
             self.raise_os_error(errno.EINVAL, new_path)
 
+        self._do_rename(old_dir_object, old_name, new_dir_object, new_name)
+
+    def _do_rename(self, old_dir_object, old_name, new_dir_object, new_name):
         object_to_rename = old_dir_object.get_entry(old_name)
         old_dir_object.remove_entry(old_name, recursive=False)
         object_to_rename.name = new_name
         new_name = new_dir_object._normalized_entryname(new_name)
-        if new_name in new_dir_object.entries:
-            # in case of overwriting remove the old entry first
-            new_dir_object.remove_entry(new_name)
-        new_dir_object.add_entry(object_to_rename)
+        old_entry = (new_dir_object.get_entry(new_name)
+                     if new_name in new_dir_object.entries else None)
+        try:
+            if old_entry:
+                # in case of overwriting remove the old entry first
+                new_dir_object.remove_entry(new_name)
+            new_dir_object.add_entry(object_to_rename)
+        except OSError:
+            # adding failed, roll back the changes before re-raising
+            if old_entry and new_name not in new_dir_object.entries:
+                new_dir_object.add_entry(old_entry)
+            object_to_rename.name = old_name
+            old_dir_object.add_entry(object_to_rename)
+            raise
 
     def _handle_broken_link_with_trailing_sep(self, path: AnyStr) -> None:
         # note that the check for trailing sep has to be done earlier

pyfakefs/tests/fake_filesystem_shutil_test.py

@@ -24,7 +24,7 @@
 import unittest
 
 from pyfakefs import fake_filesystem_unittest
-from pyfakefs.fake_filesystem import is_root
+from pyfakefs.fake_filesystem import is_root, set_uid, USER_ID
 from pyfakefs.tests.test_utils import RealFsTestMixin
 
 is_windows = sys.platform == 'win32'
@@ -38,6 +38,8 @@ def __init__(self, methodName='runTest'):
     def setUp(self):
         RealFsTestMixin.setUp(self)
         self.cwd = os.getcwd()
+        self.uid = USER_ID
+        set_uid(1000)
         if not self.use_real_fs():
             self.setUpPyfakefs()
             self.filesystem = self.fs
@@ -47,6 +49,7 @@ def setUp(self):
             self.fs.set_disk_usage(1000, self.base_path)
 
     def tearDown(self):
+        set_uid(self.uid)
         RealFsTestMixin.tearDown(self)
 
     @property
@@ -57,6 +60,22 @@ def is_windows_fs(self):
 
 
 class FakeShutilModuleTest(RealFsTestCase):
+    @unittest.skipIf(is_windows, 'Posix specific behavior')
+    def test_catch_permission_error(self):
+        root_path = self.make_path('rootpath')
+        self.create_dir(root_path)
+        dir1_path = self.os.path.join(root_path, 'dir1')
+        dir2_path = self.os.path.join(root_path, 'dir2')
+        self.create_dir(dir1_path)
+        self.os.chmod(dir1_path, 0o555)  # remove write permissions
+        self.create_dir(dir2_path)
+        old_file_path = self.os.path.join(dir2_path, 'f1.txt')
+        new_file_path = self.os.path.join(dir1_path, 'f1.txt')
+        self.create_file(old_file_path)
+
+        with self.assertRaises(PermissionError):
+            shutil.move(old_file_path, new_file_path)
+
     def test_rmtree(self):
         directory = self.make_path('xyzzy')
         dir_path = os.path.join(directory, 'subdir')