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

Infinite loop with short maximum line lengths in EmailPolicy #80745

Closed
pganssle opened this issue Apr 8, 2019 · 12 comments
Closed

Infinite loop with short maximum line lengths in EmailPolicy #80745

pganssle opened this issue Apr 8, 2019 · 12 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes

Comments

@pganssle
Copy link
Member

pganssle commented Apr 8, 2019

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
    Copy link
    Member Author

    pganssle commented Apr 8, 2019

    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:

    # 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.

    @pganssle pganssle added 3.7 (EOL) end of life 3.8 only security fixes labels Apr 8, 2019
    @maxking
    Copy link
    Contributor

    maxking commented May 17, 2019

    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
    Copy link
    Member

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

    @bitdancer
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    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
    Copy link
    Contributor

    maxking commented May 17, 2019

    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
    Copy link
    Member

    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
    Copy link
    Member

    warsaw commented Jul 16, 2019

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

    @miss-islington
    Copy link
    Contributor

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

    @miss-islington
    Copy link
    Contributor

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

    @ned-deily
    Copy link
    Member

    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

    @ned-deily ned-deily added the 3.9 only security fixes label Jul 21, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants