bpo-36564: Fix infinite loop in email folding logic #12732

pganssle · 2019-04-08T16:55:47Z

As far as I can tell, this infinite loop would be triggered if:

The value being folded contains a single word (no spaces) longer than
max_line_length
The max_line_length is shorter than the encoding's name + 9
characters.

bpo-36564: https://bugs.python.org/issue36564

https://bugs.python.org/issue36564

pganssle · 2019-04-08T17:01:38Z

I believe this will cause merge conflicts with #12020. I have subscribed to that PR and will rebase if it is merged first.

pganssle · 2019-04-08T17:03:41Z

Lib/email/_header_value_parser.py

+    chrome_len = len(encode_as) + 7
+
+    if (chrome_len + 1) >= maxlen:
+        raise ValueError("Maximum length too short to fit any values")


I picked ValueError here, but if there's a better error type let me know, I'm not super familiar with the email module.

We should probably use email.errors.HeaderParseError instead of ValueError.

It would be nice if you could use single quotes for consistency in the module.

Also, the error message seems a bit vague given that it is not too short to fit any value, but too short to hold encoded word. An ascii value would be just fine with the line length). And, Maximum length is a hard to make sense when coming from an exception message. max_line_length is more greppable to me.
What do you think about this error?

max_line_length is too small to fit an encoded word.

+1 for HeaderParseError but @bitdancer has more up-to-date perspective than I might have.

No, a ValueError would be more appropriate here, I think, because it is an error in the value of maxlen. This isn't a parsing error, it's a rendering error.

Now, that said you might want to consider the fact that in _fold_mime_parameters I deal with this issue by bumping maxlen to 78 rather than raising an error. I'm not sure that was the right choice, but whatever we do, it should probably be made consistent between the two cases. It is so sad that I never came back to fix that XXX comment I left myself :(

Note: I unfortunately am not too likely to see github comments, I get too much noise from that source...I just caught this one by chance. If you need my attention please post on the issue.

Also, the error message seems a bit vague given that it is not too short to fit any value, but too short to hold encoded word. An ascii value would be just fine with the line length). And, Maximum length is a hard to make sense when coming from an exception message. max_line_length is more greppable to me.

I agree that "Maximum length" is not going to be a good error, switching to max_line_length is a good idea.

Regarding the "encoded word" situation, I guess when I wrote this I had assumed that the RFC chrome was added to every line regardless, but only realized that that wasn't the case through testing. I'm still not entirely clear why the encoding chrome is unnecessary for shorter words, but I think the suggested comments at least explain when the chrome becomes necessary.

pganssle · 2019-05-16T02:06:59Z

@maxking @msapiro @warsaw I have rebased this after #12020 was merged, and it is ready for review. I figured I'd tag Abhilash and Mark on this, as potential new email maintainers.

maxking

Overall, the changes look good to me. I have added some inline comments for error message and slight change to a comment.

Thanks @pganssle !

maxking · 2019-05-16T18:38:17Z

Lib/email/_header_value_parser.py

+    chrome_len = len(encode_as) + 7
+
+    if (chrome_len + 1) >= maxlen:
+        raise ValueError("Maximum length too short to fit any values")


We should probably use email.errors.HeaderParseError instead of ValueError.

It would be nice if you could use single quotes for consistency in the module.

Also, the error message seems a bit vague given that it is not too short to fit any value, but too short to hold encoded word. An ascii value would be just fine with the line length). And, Maximum length is a hard to make sense when coming from an exception message. max_line_length is more greppable to me.
What do you think about this error?

max_line_length is too small to fit an encoded word.

maxking · 2019-05-16T18:47:34Z

Lib/test/test_email/test_policy.py

+        # required length for a line
+
+        # Note: This is only triggered when there is a single word longer than
+        # maxlen, hence the 1234567890 at the end of this whimsical subject


Slight change to the comment to make it a bit more clear why this bug is trigerred when there is a single world longer than maxlen.

# Note: This is only triggered when there is a single word longer than # max_line_length. This is because when we encounter a word longer than max_line_length, # it is broken down into encoded words to fit max_line_length. If the max_line_length isn't # large enough to even contain RFC 2047 chrome (`?=<charset>?q??=`), we fail. # Hence the 1234567890 at the end of this whimsical subject.

Just to make it clear that Header can have words longer than max line length, as long as max_line_length > 7 + len(charset) + 1.

(There's an extra space in your comment: "even contain"

HeaderParseError seems right to me too, but maybe @bitdancer has a different opinion?

Actually, the phrase "even contain RFC 2047 chrome (?=<charset>?q??=)" should probably be "even contain RFC 2047 chrome plus one character(?=<charset>?q?x?=)"

warsaw · 2019-05-16T21:26:55Z

Lib/test/test_email/test_policy.py

+    def test_short_maxlen_error(self):
+        # RFC 2047 chrome takes up 7 characters, plus the length of the charset
+        # name, so folding should fail if maxlen is lower than the minimum
+        # required length for a line


Missing period.

warsaw

See inlined comments.

bedevere-bot · 2019-05-16T21:27:35Z

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.

msapiro · 2019-05-16T21:33:32Z

I agree with @maxking that this is a good change - much better than leaving the looping possibility open with just a comment in the code. I also agree with all of @maxking review comments with one minor note inline and the thanks to @pganssle for developing this.

maxking · 2019-05-17T02:15:38Z

Also @pganssle you may want to change the description, since the first point (The value being folded contains a single word (no spaces) longer than max_line_length) isn't true.

pganssle · 2019-05-29T15:55:41Z

Thanks for the reviews everyone. Just as an update, I am planning on making the requested changes soon, but I've just started a new job and I'm still working out some of the IP issues so I've been holding off on any non-trivial OSS work until that's sorted out. Shouldn't be too much longer.

pganssle · 2019-07-14T11:12:26Z

I have made the requested changes; please review again

bedevere-bot · 2019-07-14T11:12:29Z

Thanks for making the requested changes!

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

As far as I can tell, this infinite loop would be triggered if: 1. The value being folded contains a single word (no spaces) longer than max_line_length 2. The max_line_length is shorter than the encoding's name + 9 characters. bpo-36564: https://bugs.python.org/issue36564

maxking · 2019-07-16T17:32:11Z

LGTM!

@pganssle It is usually better to not force push and just keep adding more commits as your make changes your existing PR. It makes it a bit easy to remember what changes were made after the previous review, especially if it has been a while since last review :)

Lib/email/_header_value_parser.py

bedevere-bot · 2019-07-16T17:44:54Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

miss-islington · 2019-07-16T17:50:04Z

Thanks @pganssle for the PR, and @warsaw for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-07-16T17:50:17Z

GH-14797 is a backport of this pull request to the 3.8 branch.

As far as I can tell, this infinite loop would be triggered if: 1. The value being folded contains a single word (no spaces) longer than max_line_length 2. The max_line_length is shorter than the encoding's name + 9 characters. bpo-36564: https://bugs.python.org/issue36564 (cherry picked from commit f69d5c6) Co-authored-by: Paul Ganssle <pganssle@users.noreply.github.com>

bedevere-bot · 2019-07-16T17:50:24Z

GH-14798 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2019-07-16T17:50:34Z

GH-14799 is a backport of this pull request to the 3.6 branch.

As far as I can tell, this infinite loop would be triggered if: 1. The value being folded contains a single word (no spaces) longer than max_line_length 2. The max_line_length is shorter than the encoding's name + 9 characters. bpo-36564: https://bugs.python.org/issue36564 (cherry picked from commit f69d5c6) Co-authored-by: Paul Ganssle <pganssle@users.noreply.github.com>

As far as I can tell, this infinite loop would be triggered if: 1. The value being folded contains a single word (no spaces) longer than max_line_length 2. The max_line_length is shorter than the encoding's name + 9 characters. bpo-36564: https://bugs.python.org/issue36564

pganssle requested a review from a team as a code owner April 8, 2019 16:55

the-knights-who-say-ni added the CLA signed label Apr 8, 2019

bedevere-bot added the awaiting review label Apr 8, 2019

pganssle force-pushed the email_infinite_loop branch from dc75480 to f77716c Compare April 8, 2019 17:00

pganssle commented Apr 8, 2019

View reviewed changes

pganssle force-pushed the email_infinite_loop branch from f77716c to 7294e5e Compare April 13, 2019 00:13

pganssle force-pushed the email_infinite_loop branch from 7294e5e to 60b9217 Compare April 20, 2019 18:37

pganssle force-pushed the email_infinite_loop branch from 60b9217 to bf536d1 Compare May 15, 2019 12:48

maxking reviewed May 16, 2019

View reviewed changes

warsaw reviewed May 16, 2019

View reviewed changes

warsaw requested changes May 16, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 16, 2019

pganssle force-pushed the email_infinite_loop branch 2 times, most recently from 184072d to aff66ad Compare July 14, 2019 11:11

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 14, 2019

pganssle force-pushed the email_infinite_loop branch from aff66ad to 01f7fb9 Compare July 14, 2019 14:55

warsaw requested changes Jul 16, 2019

View reviewed changes

Lib/email/_header_value_parser.py Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting change review labels Jul 16, 2019

warsaw approved these changes Jul 16, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jul 16, 2019

warsaw added needs backport to 3.6 type-security A security issue labels Jul 16, 2019

warsaw merged commit f69d5c6 into python:master Jul 16, 2019

bedevere-bot removed awaiting merge needs backport to 3.8 labels Jul 16, 2019

bedevere-bot removed the needs backport to 3.7 label Jul 16, 2019

bedevere-bot removed the needs backport to 3.6 label Jul 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36564: Fix infinite loop in email folding logic #12732

bpo-36564: Fix infinite loop in email folding logic #12732

pganssle commented Apr 8, 2019 •

edited by bedevere-bot

Loading

pganssle commented Apr 8, 2019

pganssle Apr 8, 2019

maxking May 16, 2019

warsaw May 16, 2019

bitdancer May 17, 2019

pganssle May 17, 2019

pganssle commented May 16, 2019

maxking left a comment

maxking May 16, 2019

maxking May 16, 2019

warsaw May 16, 2019

warsaw May 16, 2019

msapiro May 16, 2019

warsaw May 16, 2019

warsaw left a comment

bedevere-bot commented May 16, 2019

msapiro commented May 16, 2019

maxking commented May 17, 2019

pganssle commented May 29, 2019

pganssle commented Jul 14, 2019

bedevere-bot commented Jul 14, 2019

maxking commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

miss-islington commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

bpo-36564: Fix infinite loop in email folding logic #12732

bpo-36564: Fix infinite loop in email folding logic #12732

Conversation

pganssle commented Apr 8, 2019 • edited by bedevere-bot Loading

pganssle commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented May 16, 2019

maxking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warsaw left a comment

Choose a reason for hiding this comment

bedevere-bot commented May 16, 2019

msapiro commented May 16, 2019

maxking commented May 17, 2019

pganssle commented May 29, 2019

pganssle commented Jul 14, 2019

bedevere-bot commented Jul 14, 2019

maxking commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

miss-islington commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

bedevere-bot commented Jul 16, 2019

pganssle commented Apr 8, 2019 •

edited by bedevere-bot

Loading