Fixed #29027 -- Fixed file_move_safe() crash when moving files with SELinux.

Thanks Florian Apolloner for the review.
This commit is contained in:
Yuri Konotopov 2022-09-14 16:24:43 +04:00 committed by Mariusz Felisiak
parent 08c5a78726
commit 64e5ef1f17
2 changed files with 44 additions and 16 deletions

View File

@ -5,9 +5,8 @@ Move a file in the safest way possible::
>>> file_move_safe("/tmp/old_file", "/tmp/new_file") >>> file_move_safe("/tmp/old_file", "/tmp/new_file")
""" """
import errno
import os import os
from shutil import copystat from shutil import copymode, copystat
from django.core.files import locks from django.core.files import locks
@ -82,12 +81,15 @@ def file_move_safe(
try: try:
copystat(old_file_name, new_file_name) copystat(old_file_name, new_file_name)
except PermissionError as e: except PermissionError:
# Certain filesystems (e.g. CIFS) fail to copy the file's metadata if # 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 # the type of the destination filesystem isn't the same as the source
# filesystem; ignore that. # filesystem. This also happens with some SELinux-enabled systems.
if e.errno != errno.EPERM: # Ignore that, but try to set basic permissions.
raise try:
copymode(old_file_name, new_file_name)
except PermissionError:
pass
try: try:
os.remove(old_file_name) os.remove(old_file_name)

View File

@ -419,35 +419,61 @@ class FileMoveSafeTests(unittest.TestCase):
os.close(handle_a) os.close(handle_a)
os.close(handle_b) os.close(handle_b)
def test_file_move_copystat_cifs(self): def test_file_move_permissionerror(self):
""" """
file_move_safe() ignores a copystat() EPERM PermissionError. This file_move_safe() ignores PermissionError thrown by copystat() and
happens when the destination filesystem is CIFS, for example. copymode().
For example, PermissionError can be raised when the destination
filesystem is CIFS, or when relabel is disabled by SELinux across
filesystems.
""" """
copystat_EACCES_error = PermissionError(errno.EACCES, "msg") permission_error = PermissionError(errno.EPERM, "msg")
copystat_EPERM_error = PermissionError(errno.EPERM, "msg") os_error = OSError("msg")
handle_a, self.file_a = tempfile.mkstemp() handle_a, self.file_a = tempfile.mkstemp()
handle_b, self.file_b = tempfile.mkstemp() handle_b, self.file_b = tempfile.mkstemp()
handle_c, self.file_c = tempfile.mkstemp()
try: try:
# This exception is required to reach the copystat() call in # This exception is required to reach the copystat() call in
# file_safe_move(). # file_safe_move().
with mock.patch("django.core.files.move.os.rename", side_effect=OSError()): with mock.patch("django.core.files.move.os.rename", side_effect=OSError()):
# An error besides EPERM isn't ignored. # An error besides PermissionError isn't ignored.
with mock.patch( with mock.patch(
"django.core.files.move.copystat", side_effect=copystat_EACCES_error "django.core.files.move.copystat", side_effect=os_error
): ):
with self.assertRaises(PermissionError): with self.assertRaises(OSError):
file_move_safe(self.file_a, self.file_b, allow_overwrite=True) file_move_safe(self.file_a, self.file_b, allow_overwrite=True)
# EPERM is ignored. # When copystat() throws PermissionError, copymode() error besides
# PermissionError isn't ignored.
with mock.patch( with mock.patch(
"django.core.files.move.copystat", side_effect=copystat_EPERM_error "django.core.files.move.copystat", side_effect=permission_error
):
with mock.patch(
"django.core.files.move.copymode", side_effect=os_error
):
with self.assertRaises(OSError):
file_move_safe(
self.file_a, self.file_b, allow_overwrite=True
)
# PermissionError raised by copystat() is ignored.
with mock.patch(
"django.core.files.move.copystat", side_effect=permission_error
): ):
self.assertIsNone( self.assertIsNone(
file_move_safe(self.file_a, self.file_b, allow_overwrite=True) file_move_safe(self.file_a, self.file_b, allow_overwrite=True)
) )
# PermissionError raised by copymode() is ignored too.
with mock.patch(
"django.core.files.move.copymode", side_effect=permission_error
):
self.assertIsNone(
file_move_safe(
self.file_c, self.file_b, allow_overwrite=True
)
)
finally: finally:
os.close(handle_a) os.close(handle_a)
os.close(handle_b) os.close(handle_b)
os.close(handle_c)
class SpooledTempTests(unittest.TestCase): class SpooledTempTests(unittest.TestCase):