Opened 10 years ago

Closed 10 years ago

Last modified 7 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 10 years ago.
Patch for #7683
7683.2.diff (698 bytes) - added by Andrii Kurinnyi 10 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 10 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 10 years ago.
Catches the specific OSError and passes the rest.
7683.5.diff (1.9 KB) - added by spaetz 10 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 10 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 10 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 10 years ago by emperorcezar

Cc: emperorcezar@… added

Changed 10 years ago by gorman

Attachment: 7683.diff added

Patch for #7683

comment:4 Changed 10 years ago by gorman

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

Added a patch.

comment:5 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: closedreopened

comment:6 Changed 10 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 10 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 10 years ago by Ramiro Morales

Keywords: 2070-fix added

See also #7658

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

#7766 reported close before move as a problem also.

Changed 10 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 10 years ago by screeley

Cc: screeley@… added

comment:10 Changed 10 years ago by chr

7683.3.dif works on mac osx 10.5, apache2 modwsgi

comment:11 Changed 10 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 10 years ago by osm@…

Cc: sebastian@… added

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

comment:13 Changed 10 years ago by chr

Cc: django@… added

comment:14 Changed 10 years ago by masneyb@…

This patch works correctly for me under Debian/Lenny.

comment:15 Changed 10 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 10 years ago by screeley

Attachment: 7683.4.diff added

Catches the specific OSError and passes the rest.

Changed 10 years ago by spaetz

Attachment: 7683.5.diff added

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

comment:16 Changed 10 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 10 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 10 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 10 years ago by anonymous

Cc: dcwatson@… added

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

#8021 is another report of this, I believe.

comment:21 in reply to:  16 Changed 10 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 10 years ago by donspaulding

Cc: donspauldingii@… added

Does this patch still need improvement?

comment:23 Changed 10 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 10 years ago by Alex Gaynor

Patch needs improvement: unset

comment:25 Changed 10 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 7 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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