From 1ff11304dcebbabdede8ef3d659ca0e54055e2fd Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Sun, 25 May 2014 22:52:47 +0200 Subject: [PATCH] [1.7.x] 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. Backport of e2efc8965edf684aaf48621680ef54b84e116576 from master. --- django/core/files/storage.py | 2 -- django/core/files/uploadedfile.py | 3 --- django/core/handlers/base.py | 2 ++ django/http/multipartparser.py | 10 ++++++++++ django/http/request.py | 6 ++++++ docs/releases/1.7.txt | 4 ++++ tests/file_storage/tests.py | 20 +++++++++++++++++++- tests/file_uploads/tests.py | 31 +++++++++++++++++++++++++++++++ tests/file_uploads/urls.py | 1 + tests/file_uploads/views.py | 6 ++++++ 10 files changed, 79 insertions(+), 6 deletions(-) diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 98864f7f22..58c623b883 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -209,7 +209,6 @@ class FileSystemStorage(Storage): # 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: @@ -228,7 +227,6 @@ class FileSystemStorage(Storage): _file = os.fdopen(fd, mode) _file.write(chunk) finally: - content.close() locks.unlock(fd) if _file is not None: _file.close() diff --git a/django/core/files/uploadedfile.py b/django/core/files/uploadedfile.py index 75f25405b9..9a94894424 100644 --- a/django/core/files/uploadedfile.py +++ b/django/core/files/uploadedfile.py @@ -96,9 +96,6 @@ class InMemoryUploadedFile(UploadedFile): 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() diff --git a/django/core/handlers/base.py b/django/core/handlers/base.py index ffc48a076a..887898a33c 100644 --- a/django/core/handlers/base.py +++ b/django/core/handlers/base.py @@ -207,6 +207,8 @@ class BaseHandler(object): 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): diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index 20232cb7c6..1bcace94cd 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -228,6 +228,7 @@ class MultiPartParser(object): break except SkipFile: + self._close_files() # Just use up the rest of this file... exhaust(field_stream) else: @@ -237,6 +238,7 @@ class MultiPartParser(object): # 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 @@ class MultiPartParser(object): """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): """ diff --git a/django/http/request.py b/django/http/request.py index ded4744a87..ecad639c82 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -5,6 +5,7 @@ import os import re import sys from io import BytesIO +from itertools import chain from pprint import pformat from django.conf import settings @@ -245,6 +246,11 @@ class HttpRequest(object): 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 diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index d70c40a057..e8f25a45e1 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -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 ^^^^^ diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index a0e2e2a6cc..b7e4bcdf6d 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -19,7 +19,8 @@ from django.core.cache import cache 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 @@ class FileStorageTests(unittest.TestCase): 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 diff --git a/tests/file_uploads/tests.py b/tests/file_uploads/tests.py index cb2a6dd92f..c933d38236 100644 --- a/tests/file_uploads/tests.py +++ b/tests/file_uploads/tests.py @@ -327,6 +327,37 @@ class FileUploadTests(TestCase): 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). diff --git a/tests/file_uploads/urls.py b/tests/file_uploads/urls.py index a6f446bf55..b5f90c88c8 100644 --- a/tests/file_uploads/urls.py +++ b/tests/file_uploads/urls.py @@ -15,4 +15,5 @@ urlpatterns = patterns('', (r'^getlist_count/$', views.file_upload_getlist_count), (r'^upload_errors/$', views.file_upload_errors), (r'^filename_case/$', views.file_upload_filename_case_view), + (r'^fd_closing/(?Pt|f)/$', views.file_upload_fd_closing), ) diff --git a/tests/file_uploads/views.py b/tests/file_uploads/views.py index a29391af6e..de195f4d39 100644 --- a/tests/file_uploads/views.py +++ b/tests/file_uploads/views.py @@ -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('')