Code

Opened 6 years ago

Closed 6 years ago

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

Download all attachments as: .zip

Change History (31)

comment:1 follow-up: Changed 6 years ago by axiak

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

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

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

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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

  • Cc emperorcezar@… added

Changed 6 years ago by gorman

Patch for #7683

comment:4 Changed 6 years ago by gorman

  • Component changed from Uncategorized to Core framework
  • Has patch set
  • Resolution set to fixed
  • Status changed from reopened to closed

Added a patch.

comment:5 Changed 6 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:6 Changed 6 years ago by andrewsk

  • 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 6 years ago by andrewsk

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

comment:7 Changed 6 years ago by ramiro

  • Keywords 2070-fix added

See also #7658

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

#7766 reported close before move as a problem also.

Changed 6 years ago by screeley

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

  • Cc screeley@… added

comment:10 Changed 6 years ago by chr

7683.3.dif works on mac osx 10.5, apache2 modwsgi

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

  • Cc sebastian@… added

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

comment:13 Changed 6 years ago by chr

  • Cc django@… added

comment:14 Changed 6 years ago by masneyb@…

This patch works correctly for me under Debian/Lenny.

comment:15 Changed 6 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

Catches the specific OSError and passes the rest.

Changed 6 years ago by spaetz

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

comment:16 follow-ups: Changed 6 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 6 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 6 years ago by mtredinnick

  • Triage Stage changed from Accepted to Ready 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 6 years ago by anonymous

  • Cc dcwatson@… added

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

#8021 is another report of this, I believe.

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

  • milestone set to 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 6 years ago by donspaulding

  • Cc donspauldingii@… added

Does this patch still need improvement?

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

  • Patch needs improvement unset

comment:25 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.