gh-138223: Fix Infinite loop in email._header_value_parser._fold_mime_parameters when parameter names are too long#138231
gh-138223: Fix Infinite loop in email._header_value_parser._fold_mime_parameters when parameter names are too long#138231ChenyangLi4288 wants to merge 13 commits intopython:mainfrom
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...).
| maxchars = maxlen - chrome_len - 2 | ||
| # Ensure maxchars is at least 1 to prevent negative values | ||
| if maxchars <= 0: | ||
| maxchars = 1 | ||
| splitpoint = maxchars | ||
| splitpoint = max(1, splitpoint) # Ensure splitpoint is always at least 1 |
There was a problem hiding this comment.
| maxchars = maxlen - chrome_len - 2 | |
| # Ensure maxchars is at least 1 to prevent negative values | |
| if maxchars <= 0: | |
| maxchars = 1 | |
| splitpoint = maxchars | |
| splitpoint = max(1, splitpoint) # Ensure splitpoint is always at least 1 | |
| maxchars = maxlen - chrome_len - 2 | |
| splitpoint = maxchars = max(1, maxchars) |
| # 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.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
4 participants
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