Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7683 closed (fixed)

Streaming Upload file deleted before move

Reported by: emperorcezar Owned by: nobody
Component: Core (Other) Version: dev
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: no UI/UX: no

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 16 years ago.
Patch for #7683
7683.2.diff (698 bytes ) - added by Andrii Kurinnyi 16 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 16 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 16 years ago.
Catches the specific OSError and passes the rest.
7683.5.diff (1.9 KB ) - added by spaetz 16 years ago.
same as patch 7683.3, but just catching errno ==2 (missing file)

Download all attachments as: .zip

Change History (31)

comment:1 by Michael Axiak, 16 years ago

Resolution: duplicate
Status: newclosed

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

in reply to:  1 comment:2 by emperorcezar, 16 years ago

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 by emperorcezar, 16 years ago

Cc: emperorcezar@… added

by gorman, 16 years ago

Attachment: 7683.diff added

Patch for #7683

comment:4 by gorman, 16 years ago

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

Added a patch.

comment:5 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: closedreopened

comment:6 by Andrii Kurinnyi, 16 years ago

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.

by Andrii Kurinnyi, 16 years ago

Attachment: 7683.2.diff added

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

comment:7 by Ramiro Morales, 16 years ago

Keywords: 2070-fix added

See also #7658

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

#7766 reported close before move as a problem also.

by screeley, 16 years ago

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 by screeley, 16 years ago

Cc: screeley@… added

comment:10 by chr, 16 years ago

7683.3.dif works on mac osx 10.5, apache2 modwsgi

comment:11 by mafix, 16 years ago

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 by osm@…, 16 years ago

Cc: sebastian@… added

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

comment:13 by chr, 16 years ago

Cc: django@… added

comment:14 by masneyb@…, 16 years ago

This patch works correctly for me under Debian/Lenny.

comment:15 by Malcolm Tredinnick, 16 years ago

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.

by screeley, 16 years ago

Attachment: 7683.4.diff added

Catches the specific OSError and passes the rest.

by spaetz, 16 years ago

Attachment: 7683.5.diff added

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

comment:16 by spaetz, 16 years ago

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).

in reply to:  16 comment:17 by spaetz, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by anonymous, 16 years ago

Cc: dcwatson@… added

comment:20 by Karen Tracey <kmtracey@…>, 16 years ago

#8021 is another report of this, I believe.

in reply to:  16 comment:21 by anonymous, 16 years ago

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 by donspaulding, 16 years ago

Cc: donspauldingii@… added

Does this patch still need improvement?

comment:23 by spaetz, 16 years ago

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 by Alex Gaynor, 16 years ago

Patch needs improvement: unset

comment:25 by Jacob, 16 years ago

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 by Jacob, 12 years ago

milestone: 1.0 beta

Milestone 1.0 beta deleted

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