Code

Opened 6 years ago

Last modified 6 years ago

#9115 new Bug

Check for presence of os.unlink in temp.py

Reported by: michaelhart Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords: temp.py windows os.unlink
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, loading Django in Google App Engine on Windows fails with an AttributeError on line 34 of django/core/files/temp.py because os.unlink does not exist.

I posted a ticket for it on one of the Google App Engine example apps (http://code.google.com/p/rietveld/issues/detail?id=44), and Guido suggested that I post a ticket here as you guys may want to run on a non-writable filesystem anyway.

I've attached a patch that checks if os.unlink is available before creating the Windows TemporaryFile hack. Note I'm a Python newbie so it might not be the right way to go about it, but you'll get the idea.

Also, FWIW, the bug reporting and patch submission guideline links in the "Create New Ticket" instructions don't seem to work.

Attachments (1)

check_for_unlink_in_temp.patch (466 bytes) - added by michaelhart 6 years ago.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by michaelhart

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I don't think this is really the solution, since Guido's point is really the root cause.

It sounds like we need a setting for the file-size management that says "never, ever save to disk" and then if you've set that and somebody uploads something that eats all your memory, that's your problem. Either we can write to disk (in which case os.unlink has to exist so we can manage the files properly) or we can't, in which case we should allow the user to specify that. Transparently moving into that "don't write to disk mode" would be dangerous, however, since it really does run the risk of a small denial of service style attack so the person setting up the applications must agree to that explicitly.

comment:2 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by michaelhart

One option might be to defer the importing of any write-specific module initialisations, or at least import these conditionally.

For example, the user should only specify to use the MemoryFileUploadHandler and not the TemporaryFileUploadHandler in their settings (something that's not currently done in the rietveld project):

FILE_UPLOAD_HANDLERS = (
    'django.core.files.uploadhandler.MemoryFileUploadHandler',
    )

And then in core/files/uploadedfile.py, delay the importing until the TemporaryUploadedFile constructor:

class TemporaryUploadedFile(UploadedFile):
    """
    A file uploaded to a temporary location (i.e. stream-to-disk).
    """
    def __init__(self, name, content_type, size, charset):
        super(TemporaryUploadedFile, self).__init__(name, content_type, size, charset)
        # Moved this from top of file
        from django.core.files import temp as tempfile
        if settings.FILE_UPLOAD_TEMP_DIR:
            self._file = tempfile.NamedTemporaryFile(suffix='.upload', dir=settings.FILE_UPLOAD_TEMP_DIR)
        else:
            self._file = tempfile.NamedTemporaryFile(suffix='.upload')

Or, only import temp if TemporaryFileUploadHandler is in settings.FILE_UPLOAD_HANDLERS:

from django.conf import settings
from django.core.files.base import File
from django.utils.encoding import smart_str

if 'django.core.files.uploadhandler.TemporaryFileUploadHandler' in settings.FILE_UPLOAD_HANDLERS:
    from django.core.files import temp as tempfile

Both of these methods work for me (I can attach patches for either/both if you like, however I suspect they're still a little hacky).

comment:4 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.