Permalink
Browse files

Fixed #22680 -- I/O operation on closed file.

This patch is two-fold; first it ensure that Django does close everything in
request.FILES at the end of the request and secondly the storage system should
no longer close any files during save, it's up to the caller to handle that --
or let Django close the files at the end of the request.
  • Loading branch information...
1 parent a1c6cd6 commit e2efc8965edf684aaf48621680ef54b84e116576 @apollo13 apollo13 committed May 25, 2014
@@ -211,7 +211,6 @@ def _save(self, name, content):
# 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:
@@ -230,7 +229,6 @@ def _save(self, name, content):
_file = os.fdopen(fd, mode)
_file.write(chunk)
finally:
- content.close()
locks.unlock(fd)
if _file is not None:
_file.close()
@@ -96,9 +96,6 @@ def __init__(self, file, field_name, name, content_type, size, charset, content_
def open(self, mode=None):
self.file.seek(0)
- def close(self):
- pass
-
def chunks(self, chunk_size=None):
self.file.seek(0)
yield self.read()
@@ -219,6 +219,8 @@ def get_response(self, request):
signals.got_request_exception.send(sender=self.__class__, request=request)
response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
+ response._closable_objects.append(request)
+
return response
def handle_uncaught_exception(self, request, resolver, exc_info):
@@ -228,6 +228,7 @@ def parse(self):
break
except SkipFile:
+ self._close_files()
# Just use up the rest of this file...
exhaust(field_stream)
else:
@@ -237,6 +238,7 @@ def parse(self):
# If this is neither a FIELD or a FILE, just exhaust the stream.
exhaust(stream)
except StopUpload as e:
+ self._close_files()
if not e.connection_reset:
exhaust(self._input_data)
else:
@@ -268,6 +270,14 @@ def IE_sanitize(self, filename):
"""Cleanup filename from Internet Explorer full paths."""
return filename and filename[filename.rfind("\\") + 1:].strip()
+ def _close_files(self):
+ # Free up all file handles.
+ # FIXME: this currently assumes that upload handlers store the file as 'file'
+ # We should document that... (Maybe add handler.free_file to complement new_file)
+ for handler in self._upload_handlers:
+ if hasattr(handler, 'file'):
+ handler.file.close()
+
class LazyStream(six.Iterator):
"""
View
@@ -5,6 +5,7 @@
import re
import sys
from io import BytesIO
+from itertools import chain
from pprint import pformat
from django.conf import settings
@@ -256,6 +257,11 @@ def _load_post_and_files(self):
else:
self._post, self._files = QueryDict('', encoding=self._encoding), MultiValueDict()
+ def close(self):
+ if hasattr(self, '_files'):
+ for f in chain.from_iterable(l[1] for l in self._files.lists()):
+ f.close()
+
# File-like and iterator interface.
#
# Expects self._stream to be set to an appropriate source of bytes by
View
@@ -504,6 +504,10 @@ File Uploads
attribute is now optional. If it is omitted or given ``None`` or an empty
string, a subdirectory won't be used for storing the uploaded files.
+* Uploaded files are now explicitly closed before the response is delivered to
+ the client. Partially uploaded files are also closed as long as they are
+ named ``file`` in the upload handler.
+
Forms
^^^^^
@@ -19,7 +19,8 @@
from django.core.exceptions import SuspiciousOperation
from django.core.files.base import File, ContentFile
from django.core.files.storage import FileSystemStorage, get_storage_class
-from django.core.files.uploadedfile import SimpleUploadedFile
+from django.core.files.uploadedfile import (InMemoryUploadedFile, SimpleUploadedFile,
+ TemporaryUploadedFile)
from django.test import LiveServerTestCase, SimpleTestCase
from django.test import override_settings
from django.utils import six
@@ -206,6 +207,23 @@ def test_file_save_with_path(self):
self.storage.delete('path/to/test.file')
+ def test_save_doesnt_close(self):
+ with TemporaryUploadedFile('test', 'text/plain', 1, 'utf8') as file:
+ file.write(b'1')
+ file.seek(0)
+ self.assertFalse(file.closed)
+ self.storage.save('path/to/test.file', file)
+ self.assertFalse(file.closed)
+ self.assertFalse(file.file.closed)
+
+ file = InMemoryUploadedFile(six.StringIO('1'), '', 'test',
+ 'text/plain', 1, 'utf8')
+ with file:
+ self.assertFalse(file.closed)
+ self.storage.save('path/to/test.file', file)
+ self.assertFalse(file.closed)
+ self.assertFalse(file.file.closed)
+
def test_file_path(self):
"""
File storage returns the full path of a file
@@ -327,6 +327,37 @@ def test_fileupload_getlist(self):
self.assertEqual(got.get('file1'), 1)
self.assertEqual(got.get('file2'), 2)
+ def test_fileuploads_closed_at_request_end(self):
+ file = tempfile.NamedTemporaryFile
+ with file() as f1, file() as f2a, file() as f2b:
+ response = self.client.post('/fd_closing/t/', {
+ 'file': f1,
+ 'file2': (f2a, f2b),
+ })
+
+ request = response.wsgi_request
+ # Check that the files got actually parsed.
+ self.assertTrue(hasattr(request, '_files'))
+
+ file = request._files['file']
+ self.assertTrue(file.closed)
+
+ files = request._files.getlist('file2')
+ self.assertTrue(files[0].closed)
+ self.assertTrue(files[1].closed)
+
+ def test_no_parsing_triggered_by_fd_closing(self):
+ file = tempfile.NamedTemporaryFile
+ with file() as f1, file() as f2a, file() as f2b:
+ response = self.client.post('/fd_closing/f/', {
+ 'file': f1,
+ 'file2': (f2a, f2b),
+ })
+
+ request = response.wsgi_request
+ # Check that the fd closing logic doesn't trigger parsing of the stream
+ self.assertFalse(hasattr(request, '_files'))
+
def test_file_error_blocking(self):
"""
The server should not block when there are upload errors (bug #8622).
@@ -15,4 +15,5 @@
url(r'^getlist_count/$', views.file_upload_getlist_count),
url(r'^upload_errors/$', views.file_upload_errors),
url(r'^filename_case/$', views.file_upload_filename_case_view),
+ url(r'^fd_closing/(?P<access>t|f)/$', views.file_upload_fd_closing),
]
@@ -157,3 +157,9 @@ def file_upload_content_type_extra(request):
(k, smart_str(v)) for k, v in uploadedfile.content_type_extra.items()
])
return HttpResponse(json.dumps(params))
+
+
+def file_upload_fd_closing(request, access):
+ if access == 't':
+ request.FILES # Trigger file parsing.
+ return HttpResponse('')

0 comments on commit e2efc89

Please sign in to comment.