Opened 10 years ago
Closed 9 years ago
#19036 closed Bug (fixed)
Big base64-encoded files uploading
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | File uploads/storage | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | 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
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.
Attachments (4)
Change History (14)
Changed 10 years ago by
Attachment: | big_base64_upload added |
---|
comment:1 Changed 10 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 10 years ago by
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.
comment:3 Changed 10 years ago by
Has patch: | set |
---|
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.
comment:4 Changed 10 years ago by
Severity: | Normal → Release blocker |
---|
Added blocker flag, as currently 3 of 4 base64 file uploads are doomed to fail.
comment:5 Changed 10 years ago by
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.
comment:6 Changed 10 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:7 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:9 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → new |
This bug is present also in Django 1.4.13 which is mentioned on Django webpage as still supported.
comment:10 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
The 1.4 branch is only receiving security fixes at this time.
Confirmed. Do you have any idea about the cleaner way to solve this?
Some ideas:
file_complete
upload handler method)