Repository navigation
set_payload does not handle binary payloads correctly #62524
Description
| BPO | 18324 |
|---|---|
| Nosy | @warsaw, @bitdancer, @vajrasky |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2013-08-22.01:18:56.039>
created_at = <Date 2013-06-28.19:16:22.263>
labels = ['easy', 'type-bug', 'expert-email']
title = 'set_payload does not handle binary payloads correctly'
updated_at = <Date 2013-08-22.01:18:56.037>
user = 'https://github.com/bitdancer'bugs.python.org fields:
activity = <Date 2013-08-22.01:18:56.037>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2013-08-22.01:18:56.039>
closer = 'r.david.murray'
components = ['email']
creation = <Date 2013-06-28.19:16:22.263>
creator = 'r.david.murray'
dependencies = []
files = ['30726', '30884', '30998', '31000', '31049']
hgrepos = []
issue_num = 18324
keywords = ['patch', 'easy']
message_count = 13.0
messages = ['192012', '192823', '192826', '192828', '192829', '193454', '193455', '193456', '193490', '193493', '193771', '195850', '195853']
nosy_count = 4.0
nosy_names = ['barry', 'r.david.murray', 'python-dev', 'vajrasky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18324'
versions = ['Python 3.3', 'Python 3.4']
Activity
bitdancer commentedon Jun 28, 2013
In order to maintain model consistency without exposing the need for 'surrogateescape' to library users, it should be possible to pass binary data to set_payload and have it do the correct conversion to the expected storage format for the model. Currently, this does not work. The attached patch provides one example test out of a class of tests that should be written and made to pass.
vajrasky commentedon Jul 10, 2013
Here is the preliminary patch for email module to pass the test.
bitdancer commentedon Jul 10, 2013
Thanks, but the patch is incorrect. The model consistently stores its data as surrogateescaped strings, and this assumption is baked in to other parts of the code. So the correct fix is to do the surrogateescape encoding at the time the payload is set.
It might in fact be better to store a binary payload as binary, but making that kind of change to the model requires much more extensive review and testing.
vajrasky commentedon Jul 10, 2013
I see. Thanks for the explanation. I'll do this patch if nobody is interested.
bitdancer commentedon Jul 10, 2013
If you want to work on it that would be great. Note that one of the things that is needed is a bunch more tests of setting various *kinds* of binary payload, including ones containing non-ascii data, and making sure the right thing happens when the payload is later fetched/serialized.
vajrasky commentedon Jul 21, 2013
Here is the patch for this ticket.
David Murray, am I on the right path? If yes, I'll put more robust tests, such as the ones with Asian encodings and unusual encodings.
vajrasky commentedon Jul 21, 2013
Sorry, got typo for the last patch.
bitdancer commentedon Jul 21, 2013
It looks like you are still patching get_payload. This should be a really simple patch against set_payload.
It occurs to me that there could be a backward compatibility concern if passing binary to set_payload currently actually works in some cases, so we definitely needs a bunch of test that do that to make sure they all fail before we fix the bug :)
vajrasky commentedon Jul 22, 2013
"It looks like you are still patching get_payload. This should be a really simple patch against set_payload."
Okay, do I get it right at this time?
About your second point, I need more time to think about it.
bitdancer commentedon Jul 22, 2013
Yes, that's what I had in mind.
vajrasky commentedon Jul 27, 2013
Here is the third version of the patch.
I am not sure what to do with the invalid data for base64 and uuencode. I decided to raise exception instead of converting it to None silently.
python-dev commentedon Aug 22, 2013
New changeset 64e004737837 by R David Murray in branch '3.3':
bpo-18324: set_payload now correctly handles binary input.
http://hg.python.org/cpython/rev/64e004737837
New changeset a4afcf93ef7b by R David Murray in branch 'default':
Merge bpo-18324: set_payload now correctly handles binary input.
http://hg.python.org/cpython/rev/a4afcf93ef7b
bitdancer commentedon Aug 22, 2013
Thanks, Vajrasky. The v2 patch was almost correct. What you couldn't know without being as deeply enmeshed in this code as I am is that the test failures from the encoders module were actually invalid. We'd previously "fixed" them, but the fixes were incorrectly compensating for this bug in set_payload. Once this bug was fixed, those "fixes" just needed to be backed out, a 'decode=True' added to their get_payload calls, and all the tests pass.
I *think* this is the last inconsistency in the model. I hope.