Browse Source

Fixed #28170 -- Fixed file_move_safe() crash when moving files to a CIFS mount.

Derrick Jackson 8 years ago
parent
commit
789c290150
3 changed files with 38 additions and 1 deletions
  1. 10 1
      django/core/files/move.py
  2. 3 0
      docs/releases/1.11.2.txt
  3. 25 0
      tests/files/tests.py

+ 10 - 1
django/core/files/move.py

@@ -5,6 +5,7 @@ Move a file in the safest way possible::
     >>> file_move_safe("/tmp/old_file", "/tmp/new_file")
 """
 
+import errno
 import os
 from shutil import copystat
 
@@ -66,7 +67,15 @@ def file_move_safe(old_file_name, new_file_name, chunk_size=1024 * 64, allow_ove
         finally:
             locks.unlock(fd)
             os.close(fd)
-    copystat(old_file_name, new_file_name)
+
+    try:
+        copystat(old_file_name, new_file_name)
+    except PermissionError as e:
+        # Certain filesystems (e.g. CIFS) fail to copy the file's metadata if
+        # the type of the destination filesystem isn't the same as the source
+        # filesystem; ignore that.
+        if e.errno != errno.EPERM:
+            raise
 
     try:
         os.remove(old_file_name)

+ 3 - 0
docs/releases/1.11.2.txt

@@ -54,3 +54,6 @@ Bugfixes
 
 * Made date-based generic views return a 404 rather than crash when given an
   out of range date (:ticket:`28209`).
+
+* Fixed a regression where ``file_move_safe()`` crashed when moving files to a
+  CIFS mount (:ticket:`28170`).

+ 25 - 0
tests/files/tests.py

@@ -1,3 +1,4 @@
+import errno
 import gzip
 import os
 import struct
@@ -340,6 +341,30 @@ class FileMoveSafeTests(unittest.TestCase):
         os.close(handle_a)
         os.close(handle_b)
 
+    def test_file_move_copystat_cifs(self):
+        """
+        file_move_safe() ignores a copystat() EPERM PermissionError. This
+        happens when the destination filesystem is CIFS, for example.
+        """
+        copystat_EACCES_error = PermissionError(errno.EACCES, 'msg')
+        copystat_EPERM_error = PermissionError(errno.EPERM, 'msg')
+        handle_a, self.file_a = tempfile.mkstemp()
+        handle_b, self.file_b = tempfile.mkstemp()
+        try:
+            # This exception is required to reach the copystat() call in
+            # file_safe_move().
+            with mock.patch('django.core.files.move.os.rename', side_effect=OSError()):
+                # An error besides EPERM isn't ignored.
+                with mock.patch('django.core.files.move.copystat', side_effect=copystat_EACCES_error):
+                    with self.assertRaises(PermissionError):
+                        file_move_safe(self.file_a, self.file_b, allow_overwrite=True)
+                # EPERM is ignored.
+                with mock.patch('django.core.files.move.copystat', side_effect=copystat_EPERM_error):
+                    self.assertIsNone(file_move_safe(self.file_a, self.file_b, allow_overwrite=True))
+        finally:
+            os.close(handle_a)
+            os.close(handle_b)
+
 
 class SpooledTempTests(unittest.TestCase):
     def test_in_memory_spooled_temp(self):