Opened 10 years ago

Closed 9 years ago

#19036 closed Bug (fixed)

Big base64-encoded files uploading

Reported by: anthony@… 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)

big_base64_upload (970 bytes) - added by anonymous 10 years ago.
19036-test.diff (1.5 KB) - added by Claude Paroz 10 years ago.
Test case as a patch
19036-1.diff (2.5 KB) - added by Claude Paroz 10 years ago.
Decoding base64 by multiple of 4
19036-2.diff (2.5 KB) - added by johannesl 10 years ago.
Corrections for stable/1.5.x branch

Download all attachments as: .zip

Change History (14)

Changed 10 years ago by anonymous

Attachment: big_base64_upload added

comment:1 Changed 10 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

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)

Changed 10 years ago by Claude Paroz

Attachment: 19036-test.diff added

Test case as a patch

comment:2 Changed 10 years ago 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.

Changed 10 years ago by Claude Paroz

Attachment: 19036-1.diff added

Decoding base64 by multiple of 4

comment:3 Changed 10 years ago by Claude Paroz

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 Claude Paroz

Severity: NormalRelease blocker

Added blocker flag, as currently 3 of 4 base64 file uploads are doomed to fail.

Changed 10 years ago by johannesl

Attachment: 19036-2.diff added

Corrections for stable/1.5.x branch

comment:5 Changed 10 years ago 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.

comment:6 Changed 10 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:7 Changed 10 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 2a67374b51c5705d5c64a5d7117ad8552e98b4bb:

Fixed #19036 -- Fixed base64 uploads decoding

Thanks anthony at adsorbtion.org for the report, and johannesl for
bringing the patch up-to-date.

comment:8 Changed 10 years ago by Claude Paroz <claude@…>

In 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.

comment:9 Changed 9 years ago by m.zak@…

Resolution: fixed
Status: closednew

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 Tim Graham

Resolution: fixed
Status: newclosed

The 1.4 branch is only receiving security fixes at this time.

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