Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#7683 closed (fixed)

Streaming Upload file deleted before move

Reported by: emperorcezar Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: 2070-fix
Cc: emperorcezar@…, prigun@…, screeley@…, sebastian@…, django@…, dcwatson@…, donspauldingii@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In /django/db/models/base.py on line 532 it's calling raw_field.close() before it tries moving the file and that's causing the following traceback.

raw_field.close() is deleting the temp file before it gets a chance to be moved.

Environment:

Request Method: POST
Request URL: http://localhost:8000/essayupload/
Django Version: 0.97-pre-SVN-7870
Python Version: 2.5.2
Installed Applications:
['appid.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'appid.apply',
 'django.contrib.admin',
 'registration',
 'extensions']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'appid.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware',
 'django.middleware.locale.LocaleMiddleware')


Traceback:
File "/usr/lib/python2.5/site-packages/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "/usr/lib/python2.5/site-packages/django/contrib/auth/decorators.py" in __call__
  67.             return self.view_func(request, *args, **kwargs)
File "/home/cezar/code/appid/apply/views.py" in essayUpload
  316.             object = form.save(commit=False)
File "/usr/lib/python2.5/site-packages/django/newforms/models.py" in save
  273.         return save_instance(self, self.instance, self._meta.fields, fail_message, commit)
File "/usr/lib/python2.5/site-packages/django/newforms/models.py" in save_instance
  44.         f.save_form_data(instance, cleaned_data[f.name])
File "/usr/lib/python2.5/site-packages/django/db/models/fields/__init__.py" in save_form_data
  865.             getattr(instance, "save_%s_file" % self.name)(data.name, data, save=False)
File "/usr/lib/python2.5/site-packages/django/db/models/fields/__init__.py" in <lambda>
  817.         setattr(cls, 'save_%s_file' % self.name, lambda instance, filename, raw_field, save=True: instance._save_FIELD_file(self, filename, raw_field, save))
File "/usr/lib/python2.5/site-packages/django/db/models/base.py" in _save_FIELD_file
  533.             file_move_safe(raw_field.temporary_file_path(), full_filename)
File "/usr/lib/python2.5/site-packages/django/core/files/move.py" in file_move_safe
  40.         file_move(old_file_name, new_file_name)
File "/usr/lib/python2.5/shutil.py" in move
  199.             copy2(src,dst)
File "/usr/lib/python2.5/shutil.py" in copy2
  91.     copyfile(src, dst)
File "/usr/lib/python2.5/shutil.py" in copyfile
  46.         fsrc = open(src, 'rb')

Exception Type: IOError at /essayupload/
Exception Value: [Errno 2] No such file or directory: u'02-heaven_on_their_minds_192_ogg_cbr_ex.ogg'

Attachments (5)

7683.diff (687 bytes) - added by gorman 8 years ago.
Patch for #7683
7683.2.diff (698 bytes) - added by Andrii Kurinnyi 8 years ago.
Close method on the NamedTemporaryFile removes temporary file before it can be moved somewhere.
7683.3.diff (1.8 KB) - added by screeley 8 years ago.
I think the close should still be called to handle self._file.file.close() as well as setting self._file.close_called. raw_field.close() should be moved after the file_move_safe and the exception should be caught within the close. Tested on Linux.
7683.4.diff (1.8 KB) - added by screeley 8 years ago.
Catches the specific OSError and passes the rest.
7683.5.diff (1.9 KB) - added by spaetz 8 years ago.
same as patch 7683.3, but just catching errno ==2 (missing file)

Download all attachments as: .zip

Change History (31)

comment:1 Changed 8 years ago by Michael Axiak

Resolution: duplicate
Status: newclosed

This is actually not the problem. The problem is #7675.

comment:2 in reply to:  1 Changed 8 years ago by emperorcezar

Resolution: duplicate
Status: closedreopened

Replying to axiak:

This is actually not the problem. The problem is #7675.

I applied the patch from #7675. Now getting the following traceback.

Environment:

Request Method: POST
Request URL: http://localhost:8000/essayupload/
Django Version: 0.97-pre-SVN-7871
Python Version: 2.5.2
Installed Applications:
['appid.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'appid.apply',
 'django.contrib.admin',
 'registration',
 'extensions']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'appid.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware',
 'django.middleware.locale.LocaleMiddleware')


Traceback:
File "/usr/lib/python2.5/site-packages/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "/usr/lib/python2.5/site-packages/django/contrib/auth/decorators.py" in __call__
  67.             return self.view_func(request, *args, **kwargs)
File "/home/cezar/code/appid/apply/views.py" in essayUpload
  316.             object = form.save(commit=False)
File "/usr/lib/python2.5/site-packages/django/newforms/models.py" in save
  273.         return save_instance(self, self.instance, self._meta.fields, fail_message, commit)
File "/usr/lib/python2.5/site-packages/django/newforms/models.py" in save_instance
  44.         f.save_form_data(instance, cleaned_data[f.name])
File "/usr/lib/python2.5/site-packages/django/db/models/fields/__init__.py" in save_form_data
  865.             getattr(instance, "save_%s_file" % self.name)(data.name, data, save=False)
File "/usr/lib/python2.5/site-packages/django/db/models/fields/__init__.py" in <lambda>
  817.         setattr(cls, 'save_%s_file' % self.name, lambda instance, filename, raw_field, save=True: instance._save_FIELD_file(self, filename, raw_field, save))
File "/usr/lib/python2.5/site-packages/django/db/models/base.py" in _save_FIELD_file
  533.             raw_field.close()
File "/usr/lib/python2.5/site-packages/django/core/files/uploadedfile.py" in close
  204.     def close(self):                return self._file.close()
File "/usr/lib/python2.5/tempfile.py" in close
  411.                 self.unlink(self.name)

Exception Type: OSError at /essayupload/
Exception Value: [Errno 2] No such file or directory: '/tmp/tmpVwM5Fd.upload'

comment:3 Changed 8 years ago by emperorcezar

Cc: emperorcezar@… added

Changed 8 years ago by gorman

Attachment: 7683.diff added

Patch for #7683

comment:4 Changed 8 years ago by gorman

Component: UncategorizedCore framework
Has patch: set
Resolution: fixed
Status: reopenedclosed

Added a patch.

comment:5 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: closedreopened

comment:6 Changed 8 years ago by Andrii Kurinnyi

Cc: prigun@… added

Attached patch doesn't fix the problem either. It looks like raw_field.close() tries to delete the file, and if file was moved first, it throws an exception. I'm wondering if we need to call close for NamedTemporaryFile objects at all? Original patch 2070 didn't override this method and everything worked.

Changed 8 years ago by Andrii Kurinnyi

Attachment: 7683.2.diff added

Close method on the NamedTemporaryFile removes temporary file before it can be moved somewhere.

comment:7 Changed 8 years ago by Ramiro Morales

Keywords: 2070-fix added

See also #7658

comment:8 Changed 8 years ago by Karen Tracey <kmtracey@…>

#7766 reported close before move as a problem also.

Changed 8 years ago by screeley

Attachment: 7683.3.diff added

I think the close should still be called to handle self._file.file.close() as well as setting self._file.close_called. raw_field.close() should be moved after the file_move_safe and the exception should be caught within the close. Tested on Linux.

comment:9 Changed 8 years ago by screeley

Cc: screeley@… added

comment:10 Changed 8 years ago by chr

7683.3.dif works on mac osx 10.5, apache2 modwsgi

comment:11 Changed 8 years ago by mafix

7683.3.diff works on ubuntu linux - hardy - apache2 mod-wsgi

some one shut apply it to the subversion..

thanks for the patch

comment:12 Changed 8 years ago by osm@…

Cc: sebastian@… added

patch .3.diff works like a charm. CC'ing myself

comment:13 Changed 8 years ago by chr

Cc: django@… added

comment:14 Changed 8 years ago by masneyb@…

This patch works correctly for me under Debian/Lenny.

comment:15 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

On the grounds that bare except catching is bad, the change to the close() method should try to catch as specific an exception as possible (and possibly even check the errno value).

Handling a specific error is good. Accidentally disguising all errors as if they are okay is bad.

Changed 8 years ago by screeley

Attachment: 7683.4.diff added

Catches the specific OSError and passes the rest.

Changed 8 years ago by spaetz

Attachment: 7683.5.diff added

same as patch 7683.3, but just catching errno ==2 (missing file)

comment:16 Changed 8 years ago by spaetz

Depending on how specific you prefer to catch the error, patch 7683.5 will just catch OSError with errno==2 (missing file) and pass on all other exceptions. Pick you choice (I think just catching all OSError as patch 7683.4 did should be fine).

comment:17 in reply to:  16 Changed 8 years ago by spaetz

Replying to spaetz:

patch 7683.5 will just catch OSError with errno==2 (missing file)

Bear with my spamming. patch 7683.5 is based on 7683.4 and not on 7683.3 as accidentally written in the description.

comment:18 Changed 8 years ago by Malcolm Tredinnick

Triage Stage: AcceptedReady for checkin

Catching errno=2 is the right thing here, so thanks for the final patch. There are plenty of other filesystem errors that could occur that are unrelated and serious enough to want to know about.

Whoever checks this in should give it another read over for sanity, but this should be pretty much ready now.

comment:19 Changed 8 years ago by anonymous

Cc: dcwatson@… added

comment:20 Changed 8 years ago by Karen Tracey <kmtracey@…>

#8021 is another report of this, I believe.

comment:21 in reply to:  16 Changed 8 years ago by anonymous

milestone: 1.0 beta

Replying to spaetz:

Depending on how specific you prefer to catch the error, patch 7683.5 will just catch OSError with errno==2 (missing file) and pass on all other exceptions. Pick you choice (I think just catching all OSError as patch 7683.4 did should be fine).

It's good work in ubuntu 8.04 and django 1.0a(#8156)

comment:22 Changed 8 years ago by donspaulding

Cc: donspauldingii@… added

Does this patch still need improvement?

comment:23 Changed 8 years ago by spaetz

I don't think so. If you agree, please reset the "patch needs improvement" flag. Please note mtredinnick:
"Whoever checks this in should give it another read over for sanity, but this should be pretty much ready now."

comment:24 Changed 8 years ago by Alex Gaynor

Patch needs improvement: unset

comment:25 Changed 8 years ago by Jacob

Resolution: fixed
Status: reopenedclosed

(In [8217]) Fixed #7683: Try not to delete uploaded files before moving them, and don't wig out of someone else does. Patch from screeley and spaetz.

comment:26 Changed 5 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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