diff --git a/django/core/files/locks.py b/django/core/files/locks.py index 212b51a73d..98a11551a7 100644 --- a/django/core/files/locks.py +++ b/django/core/files/locks.py @@ -40,20 +40,24 @@ try: except (ImportError, AttributeError): pass +def fd(f): + """Get a filedescriptor from something which could be a file or an fd.""" + return hasattr(f, 'fileno') and f.fileno() or f + if system_type == 'nt': def lock(file, flags): - hfile = win32file._get_osfhandle(file.fileno()) + hfile = win32file._get_osfhandle(fd(file)) win32file.LockFileEx(hfile, flags, 0, -0x10000, __overlapped) def unlock(file): - hfile = win32file._get_osfhandle(file.fileno()) + hfile = win32file._get_osfhandle(fd(file)) win32file.UnlockFileEx(hfile, 0, -0x10000, __overlapped) elif system_type == 'posix': def lock(file, flags): - fcntl.flock(file.fileno(), flags) + fcntl.flock(fd(file), flags) def unlock(file): - fcntl.flock(file.fileno(), fcntl.LOCK_UN) + fcntl.flock(fd(file), fcntl.LOCK_UN) else: # File locking is not supported. LOCK_EX = LOCK_SH = LOCK_NB = None diff --git a/django/core/files/move.py b/django/core/files/move.py index 66873d450c..5ae776c3dd 100644 --- a/django/core/files/move.py +++ b/django/core/files/move.py @@ -44,16 +44,17 @@ def file_move_safe(old_file_name, new_file_name, chunk_size = 1024*64, allow_ove pass # If the built-in didn't work, do it the hard way. - new_file = open(new_file_name, 'wb') - locks.lock(new_file, locks.LOCK_EX) - old_file = open(old_file_name, 'rb') - current_chunk = None - - while current_chunk != '': - current_chunk = old_file.read(chunk_size) - new_file.write(current_chunk) - - new_file.close() - old_file.close() + fd = os.open(new_file_name, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0)) + try: + locks.lock(fd, locks.LOCK_EX) + old_file = open(old_file_name, 'rb') + current_chunk = None + while current_chunk != '': + current_chunk = old_file.read(chunk_size) + os.write(fd, current_chunk) + finally: + locks.unlock(fd) + os.close(fd) + old_file.close() os.remove(old_file_name) diff --git a/django/core/files/storage.py b/django/core/files/storage.py index fcad86fda0..30d9be9f00 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -39,9 +39,9 @@ class Storage(object): # Get the proper name for the file, as it will actually be saved. if name is None: name = content.name + name = self.get_available_name(name) - - self._save(name, content) + name = self._save(name, content) # Store filenames with forward slashes, even on Windows return force_unicode(name.replace('\\', '/')) @@ -135,19 +135,41 @@ class FileSystemStorage(Storage): os.makedirs(directory) elif not os.path.isdir(directory): raise IOError("%s exists and is not a directory." % directory) - - if hasattr(content, 'temporary_file_path'): - # This file has a file path that we can move. - file_move_safe(content.temporary_file_path(), full_path) - content.close() - else: - # This is a normal uploadedfile that we can stream. - fp = open(full_path, 'wb') - locks.lock(fp, locks.LOCK_EX) - for chunk in content.chunks(): - fp.write(chunk) - locks.unlock(fp) - fp.close() + + # There's a potential race condition between get_available_name and + # saving the file; it's possible that two threads might return the + # same name, at which point all sorts of fun happens. So we need to + # try to create the file, but if it already exists we have to go back + # to get_available_name() and try again. + + while True: + try: + # This file has a file path that we can move. + if hasattr(content, 'temporary_file_path'): + file_move_safe(content.temporary_file_path(), full_path) + content.close() + + # This is a normal uploadedfile that we can stream. + else: + # This fun binary flag incantation makes os.open throw an + # OSError if the file already exists before we open it. + fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0)) + try: + locks.lock(fd, locks.LOCK_EX) + for chunk in content.chunks(): + os.write(fd, chunk) + finally: + locks.unlock(fd) + os.close(fd) + except OSError: + # Ooops, we need a new file name. + name = self.get_available_name(name) + full_path = self.path(name) + else: + # OK, the file save worked. Break out of the loop. + break + + return name def delete(self, name): name = self.path(name) diff --git a/tests/regressiontests/file_storage/tests.py b/tests/regressiontests/file_storage/tests.py index a4503d9805..d9b0fe4f7f 100644 --- a/tests/regressiontests/file_storage/tests.py +++ b/tests/regressiontests/file_storage/tests.py @@ -64,3 +64,38 @@ u'custom_storage.2' >>> custom_storage.delete(first) >>> custom_storage.delete(second) """ + +# Tests for a race condition on file saving (#4948). +# This is written in such a way that it'll always pass on platforms +# without threading. + +import time +from unittest import TestCase +from django.core.files.base import ContentFile +from models import temp_storage +try: + import threading +except ImportError: + import dummy_threading as threading + +class SlowFile(ContentFile): + def chunks(self): + time.sleep(1) + return super(ContentFile, self).chunks() + +class FileSaveRaceConditionTest(TestCase): + def setUp(self): + self.thread = threading.Thread(target=self.save_file, args=['conflict']) + + def save_file(self, name): + name = temp_storage.save(name, SlowFile("Data")) + + def test_race_condition(self): + self.thread.start() + name = self.save_file('conflict') + self.thread.join() + self.assert_(temp_storage.exists('conflict')) + self.assert_(temp_storage.exists('conflict_')) + temp_storage.delete('conflict') + temp_storage.delete('conflict_') +