Repository navigation
gh-138223: Fix Infinite loop in email._header_value_parser._fold_mime_parameters when parameter names are too long - #138231
ChenyangLi4288 wants to merge 16 commits into
Conversation
… parameter keys The infinite loop occurred in _fold_mime_parameters() when processing MIME parameters with very long keys (64 characters) during RFC 2231 encoding. The issue was in two locations: 1. In email._header_value_parser._fold_mime_parameters(): - Replace infinite 'while True:' loop with 'while splitpoint > 1:' - Ensure splitpoint is always at least 1 to prevent getting stuck - Add fallback logic to force minimal splits when values cannot fit 2. In email.header._ValueFormatter._append_chunk(): - Add safety check for extremely long strings that cannot be split - Force line breaks when no suitable split points are found - Prevent infinite loops in header folding for edge cases This fixes GitHub issue python#138223 where add_attachment() with long parameter keys would cause as_string() to hang indefinitely during MIME parameter folding and header processing.
… parameter keys The infinite loop occurred in _fold_mime_parameters() when processing MIME parameters with very long keys (64 characters) during RFC 2231 encoding. Changes made: 1. In email._header_value_parser._fold_mime_parameters(): - Replace infinite 'while True:' loop with 'while splitpoint > 1:' - Ensure splitpoint is always at least 1 to prevent getting stuck - Add fallback logic to force minimal splits when values cannot fit 2. In email.header._append_chunk(): - Add comment explaining handling of extremely long strings that can't be split This fixes GitHub issue python#138223 where add_attachment() with long parameter keys would cause as_string() to hang indefinitely during MIME parameter folding and header processing.
|
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 |
…pythonGH-138223) - Fix a variable scope issue with encoded_value in _fold_mime_parameters - Add test case to reproduce and verify the fix - Update NEWS entry for the security fix
|
RFC 2231 specifying that: |
They do, it's 126 reserved characters (it's in the EBNF)
In this case, I think forcing the writing is fine. OTOH, does the RFC allow splitting the parameter names in a good way? (and how) |
|
Could you pinpoint the location of the RFC where it's stated, so that we also add it at the source code level as a comment? TiA. |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
The tests are incomplete and incorrect as the length limit is dynamic and depends on the length of the parameter's values, and please:
- don't create a new file,
- don't catch unexpected exceptions,
- verify that
mainhangs with the test inputs. The bad values depend on the values.
|
I'm converting it into a draft until comments are addressed. |
…d_mime_parameters Fix infinite loop that occurred when processing MIME parameters with very long parameter names during RFC 2231 encoding. The issue was in the while loop that tried to find split points for long parameter values. Changes made: - In email._header_value_parser._fold_mime_parameters(): Replace infinite 'while True:' loop with 'while splitpoint > 1:' and ensure splitpoint is always at least 1 to prevent getting stuck - Add fallback logic to force minimal splits when values cannot fit Testing: - Added test_mime_parameter_folding_no_infinite_loop to TestMIMEPart class - Test creates scenario where maxchars = 1 (edge case that caused infinite loop) - Verifies as_string() completes successfully instead of hanging - Test passes, confirming the fix prevents infinite loops This fixes GitHub issue python#138223 where add_attachment() with long parameter keys would cause as_string() to hang indefinitely during MIME parameter folding and header processing.
merge from the remote repo
|
It doesn't EXPLICITLY state that parameter names cannot be split. All examples in section 7 show splitting values across multiple complete parameter names, never splitting the parameter name itself. So the prohibition against splitting parameter names is inferred from the RFC's design and purpose rather than explicitly stated. |
As there is no mechanism to split them and since the EBNF doesn't allow partial names, I think we should consider this to be the norm. @bitdancer any recommendation here? |
|
Please, stop merging main into your branch unless there is a conflict to solve. It wastes resources. |
|
Sorry for that, will stop doing that. |
|
any updates on that? |
|
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 |
Usually, reviews can be requested after a month or so if the reviewers didn't reply since then. We don't always have the necessary bandwidth. |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Sorry for not addressing this review! I hope it will help but I think we need an email's maintainer for that (since we force a split...).
| # have that, we'd be stuck, so in that case fall back to | ||
| # the RFC standard width. | ||
| maxlen = 78 | ||
| splitpoint = maxchars = maxlen - chrome_len - 2 | ||
| while True: | ||
| maxchars = maxlen - chrome_len - 2 |
There was a problem hiding this comment.
Correct me if I've misunderstood something, but I believe the simpler fix here is the following:
- # have that, we'd be stuck, so in that case fall back to
- # the RFC standard width.
- maxlen = 78
+ # the RFC standard width, or the width of the chrome plus
+ # an arbitrary 10 if the parameter name is long.
+ maxlen = max(78, chrome_len + 10
With this change applied instead of yours, your test passes, at least. (I haven't reviewed the test yet; thought I'd post this first). My logic is that if we are already violating the user's request for maxwidth (and we already have a case where we're doing that), we might as well just make the new value something semi-reasonable, rather than "just enough" (one character more than the chrome).
If this isn't enough to protect against an infinite loop (and I haven't tried to reason through that yet, either), then we are going to need a better test.
There was a problem hiding this comment.
Thanks for the suggestion! I tried maxlen = max(78, chrome_len + 10) in place of my change. It fixes the ASCII cases, but some non-ASCII values still hang, as @devdavidejesus also found:
import email.message
m = email.message.EmailMessage()
m.set_content('hi')
m.set_param('x' * 62, '\U0001F600')
m.as_string() # never returnsHere the fallback isn't taken (maxlen > chrome_len + 3), but maxchars is only a few characters, and a single percent-encoded astral character needs 12 (%F0%9F%98%80). The inner loop ends at splitpoint == 0, writes an empty section, and value never gets shorter. Because chrome_len grows once the section number reaches 10, longer values hit this with even more names. In a sweep of names of 40–120 characters and 8 ASCII/non-ASCII values, main hangs in 62 of 112 cases, the max(78, chrome_len + 10) change in 8 (non-ASCII values, names of 60–63 characters), and this PR in none. Every value survives a round trip through message_from_string().
So I kept the forced one-character split. The side effect is that when even one encoded character doesn't fit, that section is written on its own line past maxlen (86 columns for the example above). Since the fallback already exceeds the requested width in this situation, that seemed acceptable to me, but I'm happy to combine both approaches if you prefer.
I've also rewritten the test. It now checks the exact folded output for inputs that hang on main (one of them also hangs with the maxlen change alone), and it checks that a range of long names and non-ASCII values survives a round trip.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
This PR is stale because it has been open for 30 days with no activity. |
|
I tested this on current main, with this PR's version of With this PR, About whether import email.message
m = email.message.EmailMessage()
m.set_content('hi')
m.set_param('x' * 62, '\U0001F600')
m.as_string() # never returnsThe fallback is not taken there, but The tests in this PR only use ASCII values. A case like the one above, which also hangs on main, would cover this. |
github-actions
Bot
removed
the
stale
|
@devdavidejesus Thank you for the review and suggestions! Will work on that! |
…act folding Apply picnixz's suggestion for clamping maxchars/splitpoint and explain why the forced one-character split is needed. Replace the regression test: check the exact folded header for two inputs that hang on main (one also hangs with maxlen = max(78, chrome_len + 10)), and check that a grid of long names and ASCII/non-ASCII values survives a round trip through the parser.
Documentation build overview
451 files changed ·
|
|
Will need to merge main to my branch to fix the doc CI failure. |
The infinite loop occurred in _fold_mime_parameters() when processing MIME parameters
with very long keys (64 characters) during RFC 2231 encoding.
Changes made:
In email._header_value_parser._fold_mime_parameters():
In email.header._append_chunk():
This fixes GitHub issue #138223 where add_attachment() with long parameter
keys would cause as_string() to hang indefinitely during MIME parameter
folding and header processing.
email._header_value_parser._fold_mime_parameterswhen parameter names are too long #138223