Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #4948, a race condition in file saving. Thanks to Martin von Lö…

…wis, who diagnosed the problem and pointed the way to a fix.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@8306 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 58cd4902a71a3695dd6c21dc957f59c333db364c 1 parent ab1a442
@jacobian jacobian authored
View
12 django/core/files/locks.py
@@ -40,20 +40,24 @@
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
View
23 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)
View
52 django/core/files/storage.py
@@ -39,9 +39,9 @@ def save(self, name, content):
# 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 @@ def _save(self, name, content):
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)
View
35 tests/regressiontests/file_storage/tests.py
@@ -64,3 +64,38 @@
>>> 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_')
+

0 comments on commit 58cd490

Please sign in to comment.
Something went wrong with that request. Please try again.