[Django] #19036: Big base64-encoded files uploading
Django
--------------------------------------+--------------------
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
I'm trying to upload image file using ajax & browser
FileReader.readAsDataURL API. In most cases it fails with
`MultiPartParserError: Could not decode base64 data: Error('Incorrect
padding',).`
When django cut of header from 'multipart/form-data' part with encoded
file data, it does not check length of the remaining data and pass it to
decoder. Length of passed data is `(FileUploadHandler chunk size) -
(removed header length)` which is not divides by 4 in most cases.
It is reproduced by testcase I attached. It is based on standard Django
test_base64_upload test but reads 512Kb from /dev/urandom instead of short
string.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Confirmed. Do you have any idea about the cleaner way to solve this?
Some ideas:
1. force the stream to be a multiple of 4 by "ungetting" some bytes from
the stream after we stripped off the headers
2. only decode the base64-encoded file at the end of the upload (using the
`file_complete` upload handler method)
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:1>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sir Anthony):
Not sure. I tried to make a patch by modifying `MultiPartParser`, where it
must be processed in my opinion (variant 1), but was confused a little
with streams there. I did not spent much time with this part of Django
previously and do not realize full request processing routine in details
yet. But I think, second variant is less acceptable because it will be
returning to data processing after finishing and will require doubling of
code (using low-level calls in higher-level abstractions) what will
complicate the structure of handlers.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:2>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
Comment:
Just attached a patch that seems to work (testing welcome). My only worry
is about memory management when doing `chunk += over_chunk`. Hopefully the
Python implementation is smart and do not copy too much stuff in memory.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:3>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* severity: Normal => Release blocker
Comment:
Added blocker flag, as currently 3 of 4 base64 file uploads are doomed to
fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:4>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by johannesl):
Regarding memory usage, this should be good enough for release. After 1.5,
it might be possible to use base64.encode together with StringIO, or even
adjusting read-sizes in streaming code.. I doubt the complexity it adds
will be worth the performance gains though.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:5>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jezdez):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:6>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2a67374b51c5705d5c64a5d7117ad8552e98b4bb"]:
{{{
#!CommitTicketReference repository=""
revision="2a67374b51c5705d5c64a5d7117ad8552e98b4bb"
Fixed #19036 -- Fixed base64 uploads decoding
Thanks anthony at adsorbtion.org for the report, and johannesl for
bringing the patch up-to-date.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:7>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"fc379b48655a365f0dd51880a1f68a15811f903a"]:
{{{
#!CommitTicketReference repository=""
revision="fc379b48655a365f0dd51880a1f68a15811f903a"
[1.5.x] Fixed #19036 -- Fixed base64 uploads decoding
Thanks anthony at adsorbtion.org for the report, and johannesl for
bringing the patch up-to-date.
Backport of 2a67374b5 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:8>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by m.zak@…):
* status: closed => new
* resolution: fixed =>
Comment:
This bug is present also in Django 1.4.13 which is mentioned on Django
webpage as still supported.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:9>
Django
Reporter: anthony@… | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: new => closed
* resolution: => fixed
Comment:
The 1.4 branch is only receiving security fixes at this time.
--
Ticket URL: <https://code.djangoproject.com/ticket/19036#comment:10>