Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7635 closed (fixed)

File uploads sometimes incorrectly detect an infinite loop.

Reported by: lakin.wecker@… Owned by: Michael Axiak
Component: HTTP handling Version: dev
Severity: Keywords: 2070-fix
Cc: Michael Axiak, Brian Rosner, Christian Schilling 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

just updated my django trunk install and am not getting a http://dpaste.com/60810/ that wasn't there before http://code.djangoproject.com/changeset/7814 ... now I realize that this presents a backwards incompatible change, but the error is coming from the admin interface (one that does have a file field) but I'm not uploading a file

Will upload the traceback from the paste on this ticket as well.

The models used:

from django.db import models 
from django.conf import settings


#-------------------------------------------------------------------------------
class File(models.Model):
    """
    """
    path = models.CharField(max_length=255, unique=True)

    folder = 'protected_files'
    file = models.FileField(upload_to=folder,)
    number_of_codes_to_create = models.PositiveIntegerField(
        help_text = "Use this to generate new codes on save."
    )
    max_uses_for_created_codes =  models.PositiveIntegerField(
        default=2,
        help_text = "New codes will have this many uses on generation."
    )

    def __unicode__(self) :
        return self.filename()

    class Admin:
        pass



#-------------------------------------------------------------------------------
class FileCode(models.Model):
    """
    """

    file = models.ForeignKey(
        File,
        edit_inline=models.TABULAR,
        related_name='codes'
    )
    code = models.CharField(max_length=10, core=True)

    uses = models.IntegerField(default=0)
    max_uses = models.PositiveIntegerField(default=2)


    class Admin:
        pass

Attachments (4)

r7814_backtrace.txt (2.5 KB ) - added by lakin.wecker@… 16 years ago.
backtrace_from_error.txt
second_patch.diff (6.8 KB ) - added by Michael Axiak 16 years ago.
Increase counter from 500 to 64000.
increase_limit.diff (641 bytes ) - added by Michael Axiak 16 years ago.
Increase counter from 500 to 64000.
multipartparser_recusion_check.diff (3.5 KB ) - added by Michael Axiak 16 years ago.
Different way to check recursion

Download all attachments as: .zip

Change History (19)

by lakin.wecker@…, 16 years ago

Attachment: r7814_backtrace.txt added

backtrace_from_error.txt

comment:1 by Malcolm Tredinnick, 16 years ago

Cc: Michael Axiak added

@axiak: This looks like your department. Any ideas?

comment:2 by Michael Axiak, 16 years ago

Hello,

Can you post more information? Specifically, the following would be extremely helpful:

  1. What browser were you using?
  2. How were you deploying Django (test server, mod_python, mod_wsgi, fastcgi, ...)?

I'm trying to replicate this now.

comment:3 by lakin.wecker@…, 16 years ago

Firefox 3. It happened with both the dev server and apache/mod_python deployment.

comment:4 by Michael Axiak, 16 years ago

Okay, after working with lakin online for about an hour I found the issue.

The issue is that in order to prevent us from entering an infinite loop while parsing, we have a counter that essentially counts how many times we enter a segment of code.

We set the limit to something that is "high", like 500. Problem is, this counter gets incremented whenever there is an additional piece of form data.

In your example, you had a file field, so the form encoding was multipart/form-data, and you had 815 form fields because of the edit inline.

I'm working on coming up with a better way to detect an infinite loop. If there is no better place, then could we make the limit something incredibly high? (100000 comes to mind) or remove it altogether?

The reason for it is that in early development we had cases where we struck an infinite loop, and it was a bad failure mode. We figured this error would be better than that failure.

by Michael Axiak, 16 years ago

Attachment: second_patch.diff added

Increase counter from 500 to 64000.

by Michael Axiak, 16 years ago

Attachment: increase_limit.diff added

Increase counter from 500 to 64000.

comment:5 by Michael Axiak, 16 years ago

Disregard second_patch.diff :-).

I'm opting for increasing the limit to 64,000 for now. I think we should have some more battle testing before we remove this check outright.

comment:6 by Malcolm Tredinnick, 16 years ago

Has patch: set
Summary: r7814 introduces bug into the admin interface.File uploads sometimes incorrectly detect an infinite loop.
Triage Stage: UnreviewedAccepted

(Changed description to something approaching the real problem.)

comment:7 by anonymous, 16 years ago

Cc: Brian Rosner added

comment:8 by Michael Axiak, 16 years ago

Component: UncategorizedHTTP handling

I actually use the component...

comment:9 by Jacob, 16 years ago

milestone: 1.0 beta

comment:10 by Christian Schilling, 16 years ago

Cc: Christian Schilling added

comment:11 by Michael Axiak, 16 years ago

I'm uploading an attempt at a different recursion metric that should work much better.

by Michael Axiak, 16 years ago

Different way to check recursion

comment:12 by Michael Axiak, 16 years ago

Keywords: 2070-fix added
Owner: changed from nobody to Michael Axiak
Status: newassigned

Adding keywords...

comment:13 by Michael Axiak, 16 years ago

Triage Stage: AcceptedReady for checkin

I think we can be happy with this fix.

comment:14 by Jacob, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [7905]) Fixed #7635: do a better job checking for infinite loops in multi-part MIME parsing. Thanks, Mike Axiak.

comment:15 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