Infinite loop with short maximum line lengths in EmailPolicy

[pganssle]

BPO	36564
Nosy	@warsaw, @msapiro, @ned-deily, @bitdancer, @maxking, @pganssle, @miss-islington, @tirkarthi
PRs	bpo-36564: Fix infinite loop in email folding logic #12732 [3.8] bpo-36564: Fix infinite loop in email folding logic (GH-12732) #14797 [3.7] bpo-36564: Fix infinite loop in email folding logic (GH-12732) #14798 [3.6] bpo-36564: Fix infinite loop in email folding logic (GH-12732) #14799

^{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 2019-07-21.14:03:58.094>
created_at = <Date 2019-04-08.16:45:34.459>
labels = ['3.7', '3.8', '3.9']
title = 'Infinite loop with short maximum line lengths in EmailPolicy'
updated_at = <Date 2019-07-21.14:03:58.082>
user = 'https://github.com/pganssle'

bugs.python.org fields:

activity = <Date 2019-07-21.14:03:58.082>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2019-07-21.14:03:58.094>
closer = 'ned.deily'
components = []
creation = <Date 2019-04-08.16:45:34.459>
creator = 'p-ganssle'
dependencies = []
files = []
hgrepos = []
issue_num = 36564
keywords = ['patch', 'security_issue']
message_count = 12.0
messages = ['339660', '342686', '342715', '342716', '342717', '342721', '342722', '342729', '348036', '348037', '348038', '348244']
nosy_count = 8.0
nosy_names = ['barry', 'msapiro', 'ned.deily', 'r.david.murray', 'maxking', 'p-ganssle', 'miss-islington', 'xtreak']
pr_nums = ['12732', '14797', '14798', '14799']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue36564'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9']

[pganssle]

When reviewing PR 12020 fixing an infinite loop in the e-mail module, I noticed that a *different* infinite loop is documented with a "# XXX" comment on line 2724:

cpython/Lib/email/_header_value_parser.py

Line 2724 in 58721a9

# XXX We'll get an infinite loop here if maxlen is <= 7

This is triggered when the policy's max_line_length is set to be shorter than minimum line length required by the "RFC 2047 chrome". It can be reproduced with:

    from email.policy import default

    policy = default.clone(max_line_length=7) # max_line_length = 78
    policy.fold("Subject", "12345678")

I could not find an entry on the tracker for this bug, but it is documented in the source code itself, so maybe I just didn't try hard enough.

Related but distinct bugs: bpo-33529, bpo-33524

I will submit a patch to fix this.

[maxking]

Moving the conversation here from #12732 for David.

I previously suggested HeaderParseError because of the fact that we could fail to parse a value into a Header Object in the above scenario.

@r.david.murray Would it be more appropriate to have something like a "PolicyError" in case where the policy's max_line_length isn't long enough to fit the shortest encoded word?

[bitdancer]

Can you demonstrate the parsing error? maxlen should have no effect during parsing.

[bitdancer]

As for the other, I don't see the need for a custom error. It's a ValueError in my view. I wouldn't object to it strongly, but note that this error is content dependent. If there's nothing to encode, you can "get away with" a shorter maxlen. Though why you would want to is beyond me, and that's another reason I don't think this warrants a custom error class.

[pganssle]

Responding to a comment on the PR:

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.

So I think in an ideal world this would be consistent between the two, but I *also* think that the right solution is to raise an exception rather than silently coercing, and changing this in _fold_mime_parameters would be a backwards-incompatible change.

I think that if you agree that an exception is better, maybe the path forward is to raise an exception in this case and switch the _fold_mime_parameters case over to raising a warning, which will be turned into an exception in a later release (3.10 maybe).

It is so sad that I never came back to fix that XXX comment I left myself :(

I'm glad that the XXX comment was at least there! Otherwise I wouldn't have notice that there was anything to fix!

[bitdancer]

Good point about the backward compatibility. Yes I agree, I think raising the error is probably better. A deprecation warning seems like a good path forward...I will be very surprised if anyone encounters it, though :)

[maxking]

I was wrong about the parsing error, it looks like length from the policy isn't used when parsing.

>>> from email.policy import default
>>> from email import message_from_string
>>> p = default.clone(max_line_length=10)
>>> msg = message_from_string("""\
... From: Hello@example.com
... To: Hello@example.com
... Subject: WelcomeToThisLongSubject
... 
... Thanks""", policy=p)
>>> msg
<email.message.EmailMessage object at 0x7f448f70d860>
>>> msg['Subject']
'WelcomeToThisLongSubject'

This works just fine. Thanks David. +1 for ValueError then.

[bitdancer]

Right, one of the fundamental principles of the email library is that when parsing input we do not ever raise an error. We may note defects, but whatever we get we *must* parse and turn in to *something*.

[warsaw]

New changeset f69d5c6 by Barry Warsaw (Paul Ganssle) in branch 'master':
Fix infinite loop in email folding logic (GH-12732)
f69d5c6

[miss-islington]

New changeset 6a2aec0 by Miss Islington (bot) in branch '3.8':
Fix infinite loop in email folding logic (GH-12732)
6a2aec0

[miss-islington]

New changeset e7bec26 by Miss Islington (bot) in branch '3.7':
Fix infinite loop in email folding logic (GH-12732)
e7bec26

[ned-deily]

New changeset 79a47e2 by Ned Deily (Miss Islington (bot)) in branch '3.6':
Fix infinite loop in email folding logic (GH-12732) (GH-14799)
79a47e2