bpo-36564: Fix infinite loop in email folding logic by pganssle · Pull Request #12732 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation28 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

pganssle

@pganssle

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

pganssle

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thanks @pganssle !

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing period.

warsaw

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inlined comments.

@bedevere-bot

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

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

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

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

I have made the requested changes; please review again

@bedevere-bot

Thanks for making the requested changes!

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

@pganssle

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

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 :)

warsaw

@bedevere-bot

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

warsaw

@miss-islington

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 16, 2019

@pganssle @miss-islington

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 16, 2019

@pganssle @miss-islington

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

@bedevere-bot

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 16, 2019

@pganssle @miss-islington

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

miss-islington added a commit that referenced this pull request

Jul 16, 2019

@miss-islington @pganssle

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

miss-islington added a commit that referenced this pull request

Jul 16, 2019

@miss-islington @pganssle

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

ned-deily pushed a commit that referenced this pull request

Jul 21, 2019

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

lisroach pushed a commit to lisroach/cpython that referenced this pull request

Sep 10, 2019

@pganssle @lisroach

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

DinoV pushed a commit to DinoV/cpython that referenced this pull request

Jan 14, 2020

@pganssle @DinoV

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