Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#7635 closed (fixed)

File uploads sometimes incorrectly detect an infinite loop.

Reported by: lakin.wecker@… Owned by: axiak
Component: HTTP handling Version: master
Severity: Keywords: 2070-fix
Cc: axiak, brosner, christian 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@… 7 years ago.
backtrace_from_error.txt
second_patch.diff (6.8 KB) - added by axiak 7 years ago.
Increase counter from 500 to 64000.
increase_limit.diff (641 bytes) - added by axiak 7 years ago.
Increase counter from 500 to 64000.
multipartparser_recusion_check.diff (3.5 KB) - added by axiak 7 years ago.
Different way to check recursion

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by lakin.wecker@…

backtrace_from_error.txt

comment:1 Changed 7 years ago by mtredinnick

  • Cc axiak added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

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

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

comment:4 Changed 7 years ago by 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 7 years ago by axiak

Increase counter from 500 to 64000.

Changed 7 years ago by axiak

Increase counter from 500 to 64000.

comment:5 Changed 7 years ago by 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 7 years ago by mtredinnick

  • Has patch set
  • Summary changed from r7814 introduces bug into the admin interface. to File uploads sometimes incorrectly detect an infinite loop.
  • Triage Stage changed from Unreviewed to Accepted

(Changed description to something approaching the real problem.)

comment:7 Changed 7 years ago by anonymous

  • Cc brosner added

comment:8 Changed 7 years ago by axiak

  • Component changed from Uncategorized to HTTP handling

I actually use the component...

comment:9 Changed 7 years ago by jacob

  • milestone set to 1.0 beta

comment:10 Changed 7 years ago by christian

  • Cc christian added

comment:11 Changed 7 years ago by axiak

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

Changed 7 years ago by axiak

Different way to check recursion

comment:12 Changed 7 years ago by axiak

  • Keywords 2070-fix added
  • Owner changed from nobody to axiak
  • Status changed from new to assigned

Adding keywords...

comment:13 Changed 7 years ago by axiak

  • Triage Stage changed from Accepted to Ready for checkin

I think we can be happy with this fix.

comment:14 Changed 7 years ago by jacob

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

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

comment:15 Changed 4 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

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