Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16082 closed Bug (fixed)

Directory creation race condition in FileSystemStorage

Reported by: Piet Delport Owned by: nobody
Component: File uploads/storage Version: master
Severity: Normal Keywords: FileSystemStorage directory creation race condition
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

My company recently encountered the following problem on a production server:

Traceback (most recent call last):

[...]

 File "[...]", line 191, in process_view
   storage.save(path, File([...]))

 File "[...]/python2.6/site-packages/django/core/files/storage.py", line 47, in save
   name = self._save(name, content)

 File "[...]/python2.6/site-packages/django/core/files/storage.py", line 146, in _save
   os.makedirs(directory)

 File "[...]/python2.6/os.py", line 157, in makedirs
   mkdir(name, mode)

OSError: [Errno 17] File exists: '[MEDIA_ROOT]/avatars/foo'

I tracked this down to a race condition in FileSystemStorage._save. When there is enough load, two concurrent processes can attempt to create the same intermediate directory, causing the second one to fail entirely.

To handle this robustly, FileSystemStorage should catch the EEXIST from os.makedirs and continue normally: I implemented this along with some tests, in the attached patch.

Note: For comparison, Python 3.2 added a keyword argument to os.makedirs to do this: see Issue 9299: os.makedirs(): Add a keyword argument to suppress "File exists" exception.

Attachments (1)

django-file_storage-makedirs-race.patch (3.8 KB) - added by Piet Delport 5 years ago.

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by Piet Delport

comment:1 Changed 5 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 5 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

The patch looks good and contains extensive tests. It applies cleanly and the test suite passes. The implementation doesn't add much overhead. It's a fix for an edge case, so it does not deserve a mention in the release notes.

comment:3 Changed 5 years ago by Piet Delport

Thank you. :)

comment:4 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16280]:

Fixed #16082 -- Fixed race condition in the FileSystemStorage backend with regard to creating directories. Thanks, pjdelport.

comment:5 Changed 5 years ago by Aymeric Augustin

#16108 describes a similar race condition in the file removal code.

comment:6 Changed 5 years ago by Jannis Leidel

In [16287]:

Fixed #16108 -- Fixed another race condition in the FileSystemStorage backend with regard to deleting a file. Refs #16082, too. Thanks, Aymeric Augustin.

Note: See TracTickets for help on using tickets.
Back to Top