Opened 8 years ago

Closed 8 years ago

Last modified 5 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: master
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: UI/UX:

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@… 8 years ago.
backtrace_from_error.txt
second_patch.diff (6.8 KB) - added by Michael Axiak 8 years ago.
Increase counter from 500 to 64000.
increase_limit.diff (641 bytes) - added by Michael Axiak 8 years ago.
Increase counter from 500 to 64000.
multipartparser_recusion_check.diff (3.5 KB) - added by Michael Axiak 8 years ago.
Different way to check recursion

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by lakin.wecker@…

Attachment: r7814_backtrace.txt added

backtrace_from_error.txt

comment:1 Changed 8 years ago by Malcolm Tredinnick

Cc: Michael Axiak added

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

comment:2 Changed 8 years ago by Michael Axiak

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 Changed 8 years ago by lakin.wecker@…

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

comment:4 Changed 8 years ago by Michael Axiak

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.

Changed 8 years ago by Michael Axiak

Attachment: second_patch.diff added

Increase counter from 500 to 64000.

Changed 8 years ago by Michael Axiak

Attachment: increase_limit.diff added

Increase counter from 500 to 64000.

comment:5 Changed 8 years ago by Michael Axiak

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 Changed 8 years ago by Malcolm Tredinnick

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 Changed 8 years ago by anonymous

Cc: Brian Rosner added

comment:8 Changed 8 years ago by Michael Axiak

Component: UncategorizedHTTP handling

I actually use the component...

comment:9 Changed 8 years ago by Jacob

milestone: 1.0 beta

comment:10 Changed 8 years ago by Christian Schilling

Cc: Christian Schilling added

comment:11 Changed 8 years ago by Michael Axiak

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

Changed 8 years ago by Michael Axiak

Different way to check recursion

comment:12 Changed 8 years ago by Michael Axiak

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

Adding keywords...

comment:13 Changed 8 years ago by Michael Axiak

Triage Stage: AcceptedReady for checkin

I think we can be happy with this fix.

comment:14 Changed 8 years ago by Jacob

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 Changed 5 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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