Opened 11 years ago

Closed 10 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 11 years ago.
19036-test.diff (1.5 KB ) - added by Claude Paroz 11 years ago.
Test case as a patch
19036-1.diff (2.5 KB ) - added by Claude Paroz 11 years ago.
Decoding base64 by multiple of 4
19036-2.diff (2.5 KB ) - added by johannesl 11 years ago.
Corrections for stable/1.5.x branch

Download all attachments as: .zip

Change History (14)

by anonymous, 11 years ago

Attachment: big_base64_upload added

comment:1 by Claude Paroz, 11 years ago

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)

by Claude Paroz, 11 years ago

Attachment: 19036-test.diff added

Test case as a patch

comment:2 by Sir Anthony, 11 years ago

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.

by Claude Paroz, 11 years ago

Attachment: 19036-1.diff added

Decoding base64 by multiple of 4

comment:3 by Claude Paroz, 11 years ago

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 by Claude Paroz, 11 years ago

Severity: NormalRelease blocker

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

by johannesl, 11 years ago

Attachment: 19036-2.diff added

Corrections for stable/1.5.x branch

comment:5 by johannesl, 11 years ago

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 by Jannis Leidel, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Claude Paroz <claude@…>, 11 years ago

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 by Claude Paroz <claude@…>, 11 years ago

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 by m.zak@…, 10 years ago

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 by Tim Graham, 10 years ago

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