Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134873: Fix a DOS issue in idlelib#134874
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
I believe that the 6 lines from 1205 to 1210 can be replaced by 2 lines -- an re.match and an f-string. I will submit an alternate proposal later. I believe that the input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Assuming this is the fix that we go with, let's add a test case.
Misc/NEWS.d/next/Security/2025-05-29-03-24-18.gh-issue-134873.dziqkQ.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@terryjreedy so should I leave the code for now, or should I go aheadand replace with the re.match thing you are going to propose?@ZeroIntensity so do you mean that active voice is preferred inrelease notes? I can replace this specific case with the change thatyou are suggesting, but I'm asking for advice in this aspect forfuture News. |
Uh oh!
There was an error while loading.Please reload this page.
Yes, I think it can be. Will fix. … Message ID: ***@***.***> |
kexinoh commentedMay 30, 2025
@johnzhou721 Lines 1373 to 1378 in5ab66a8
As an example, I successfully utilized Gemini 2.5 Pro to generate a reasonable fix for this problem. Could you give it a try? |
@kexinoh Yes, I would give it a try once I have time; however, I am working on something else right now -- is it acceptable if I delay this by about a day or so? (if anyone else has a fix ready before I get to this, feel free to make a pr onto the branch of my pr and I'll merge it into my PR) |
…dziqkQ.rstCo-authored-by: Peter Bierma <zintensitydev@gmail.com>
@kexinoh I have a small amount of time not enough to work on anything else before I end my day so I attempted the issue you pointed out -- but can't test though. |
Where? How? For what? Thanks!@ZeroIntensity |
We need a test case in |
Lib/idlelib/editor.py Outdated
| have = len(chars.expandtabs(tabwidth)) | ||
| for i in range(len(chars) - 1, -1, -1): | ||
| if have <= want or chars[i] not in " \t": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
have <= want is now a constant condition buthave previoulsy was recomputed at every iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just pushed another change. Not sure if I got the logic right here.
| if have <= want or chars[-1] not in " \t": | ||
| have = len(chars.expandtabs(tabwidth)) | ||
| for i in range(len(chars) - 1, -1, -1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not usingrfind(' ') andrfind('\t') to locate the rightmost ' \t'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sorry for being so inexperienced but could you please elaborate on your approach using rfind? I personally do not see how it could simplify matters, but if you could, I'd like for you to take some time to explain it. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably got somehting wrong in the latest commit...
Lib/idlelib/editor.py Outdated
| have = len(chars.expandtabs(tabwidth)) | ||
| for i in range(len(chars) - 1, -1, -1): | ||
| if have <= want or chars[i] not in " \t": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just pushed another change. Not sure if I got the logic right here.
| if have <= want or chars[-1] not in " \t": | ||
| have = len(chars.expandtabs(tabwidth)) | ||
| for i in range(len(chars) - 1, -1, -1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sorry for being so inexperienced but could you please elaborate on your approach using rfind? I personally do not see how it could simplify matters, but if you could, I'd like for you to take some time to explain it. Thank you!
Uh oh!
There was an error while loading.Please reload this page.
A DOS by Quadratic complexity issue is fixed in idlelib. Part of (but does not fix)#134873.