Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-37764: fix hang case in get_unstructured #15239

Open
wants to merge 12 commits into
base: master
from

Conversation

@epicfaace
Copy link
Contributor

commented Aug 12, 2019

Fixes a case in which email._header_value_parser.get_unstructured hangs the system for some invalid headers.

https://bugs.python.org/issue37764

This fix should also be backported to 3.7 and 3.8

https://bugs.python.org/issue37764

@epicfaace epicfaace requested a review from python/email-team as a code owner Aug 12, 2019

@maxking
Copy link
Contributor

left a comment

Thanks for this PR.
I've made inline comments. Please also see my comments on BPO.

Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/test/test_email/test__header_value_parser.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 16, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

epicfaace added some commits Aug 19, 2019

@epicfaace

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 19, 2019

Thanks for making the requested changes!

@maxking: please review the changes made to this pull request.

Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
@@ -1039,7 +1039,7 @@ def get_encoded_word(value):
raise errors.HeaderParseError(
"expected encoded word but found {}".format(value))
remstr = ''.join(remainder)
if len(remstr) > 1 and remstr[0] in hexdigits and remstr[1] in hexdigits:
if len(remstr) > 1 and remstr[0] in hexdigits and remstr[1] in hexdigits and tok.count('?') < 2:

This comment has been minimized.

Copy link
@maxking

maxking Aug 21, 2019

Contributor

Probably needs to be split on multiple line for pep8.

This comment has been minimized.

Copy link
@epicfaace

epicfaace Aug 23, 2019

Author Contributor

Done.

Is there a way to check for this automatically? What's the standard I should usually apply? (for example, this file doesn't have two blank lines between each class, which also violates pep8)

Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 21, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@DeltaF1

This comment has been minimized.

Copy link

commented Aug 22, 2019

I applied this patch to a local install of python 3.7, and the problem seemed to persist with my test case. The attached file has the email I parsed, with some content redacted for privacy. It appears to be very mangled, but even so the library should probably not fall into an infinite loop if it receives mangled data.

Hopefully you can reproduce the error, otherwise it might just be an artefact of my patching method.

mangled_message.txt

@epicfaace

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I applied this patch to a local install of python 3.7, and the problem seemed to persist with my test case. The attached file has the email I parsed, with some content redacted for privacy. It appears to be very mangled, but even so the library should probably not fall into an infinite loop if it receives mangled data.

Hopefully you can reproduce the error, otherwise it might just be an artefact of my patching method.

mangled_message.txt

I did add a test in this PR with that mangled message, but it doesn't seem to hang. Either it's an artefact with your patching method, or a different issue with 3.7.

@epicfaace

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I have made the requested changes; please review again.

@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 23, 2019

Thanks for making the requested changes!

@maxking: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.