Skip to content

Conversation

@johnzhou721
Copy link
Contributor

@johnzhou721 johnzhou721 commented May 29, 2025

A DOS by Quadratic complexity issue is fixed in idlelib. Part of (but does not fix) #134873.

@terryjreedy terryjreedy moved this to In Progress in IDLE Issues May 29, 2025
@terryjreedy terryjreedy self-assigned this May 29, 2025
@terryjreedy
Copy link
Member

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 vevent name should have either no <>s or 2 of each, with maybe the latter for back compatibility (I will test). But I will may stick with the more general code to not break buggy extensions.

Copy link
Member

@ZeroIntensity ZeroIntensity left a 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.

@ZeroIntensity ZeroIntensity added type-security A security issue needs backport to 3.9 needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels May 29, 2025
@johnzhou721
Copy link
Contributor Author

johnzhou721 commented May 29, 2025 via email

@johnzhou721
Copy link
Contributor Author

johnzhou721 commented May 29, 2025 via email

@kexinoh
Copy link

kexinoh commented May 30, 2025

@johnzhou721
I would greatly appreciate it if you could kindly address the issue located at

cpython/Lib/idlelib/editor.py

Lines 1373 to 1378 in 5ab66a8

while True:
chars = chars[:-1]
ncharsdeleted = ncharsdeleted + 1
have = len(chars.expandtabs(tabwidth))
if have <= want or chars[-1] not in " \t":
break
. I sincerely apologize for overlooking this in my previous message.

As an example, I successfully utilized Gemini 2.5 Pro to generate a reasonable fix for this problem. Could you give it a try?

@johnzhou721
Copy link
Contributor Author

@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)

@johnzhou721
Copy link
Contributor Author

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

@johnzhou721
Copy link
Contributor Author

Assuming this is the fix that we go with, let's add a test case.

Where? How? For what? Thanks! @ZeroIntensity

@ZeroIntensity
Copy link
Member

Where? How? For what?

We need a test case in test_idlelib that results in DOS/extreme slowness off main. Basically, just do something to prove that this PR fixes it (probably just testing with large amounts of data).

@serhiy-storchaka
Copy link
Member

Sorry for my unclear comment. I see now that it can be be interpreted incorrectly.

I questioned not the change (which LGTM), but necessary of the NEWS entry for it.

@johnzhou721
Copy link
Contributor Author

I have made the requested changes; please review again

FYI: I credited @terryjreedy as well since they came up with the approach for combining it into all one line.

(@terryjreedy: not sure about your pronouns, sorry for using they)

@bedevere-app
Copy link

bedevere-app bot commented Jun 29, 2025

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from terryjreedy June 29, 2025 13:08
@zware
Copy link
Member

zware commented Jun 30, 2025

@zware Do you have any idea what is wrong? Is the fact that johnzhou forked from somewhere else than python/cpython relevant?

My suspicion would be that the push to https:// is rejected; I seem to remember something about GitHub no longer accepting HTTPS pushes at all anymore, but I don't have a reference for that assertion. I'd try git push [email protected]:johnzhou721/cpython pr_134874:idledos instead.

Co-authored-by: Zachary Ware <[email protected]>
@johnzhou721
Copy link
Contributor Author

Wow. Apparently I forgot to resolve a conversation.

Since this is finalized, merging main to retrigger this.

@terryjreedy Have you had time to rereview this yet? Thanks!

@johnzhou721
Copy link
Contributor Author

Hmm... apparently the labels in the sidebar is triggering the issue, not the Resolve Conversation on GitHub. Sorry for merging main again!

@johnzhou721
Copy link
Contributor Author

@terryjreedy Is there anything else I need to do to get this merged, or are you just busy? Thanks!

@johnzhou721
Copy link
Contributor Author

#136556 tracks the refactoring for the backspace part... I don't know if any information will need to be included there. The issue in its curretn form is quite vague and I am clueless about what to do.

@johnzhou721
Copy link
Contributor Author

@terryjreedy Is there any chance that you'd have time to look at this again? Thank you!

@@ -1,9 +1,12 @@
What's New in IDLE 3.14.0
(since 3.13.0)
Released on 2025-10-07
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right file or right section to add to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mentioned by @terryjreedy that this is the file to add to. Resolving.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK but this is not for version 3.14 released on october 7, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merwok I'm not entirely sure how backports will be handled--but it seems that there's no other news for IDLE 3.15 yet, so I added the section header. Thanks for letting me know, and also -- I've renamed the PR.

@merwok
Copy link
Member

merwok commented Nov 4, 2025

Can the PR title be changed to be more useful? Every PR is a change. Good titles say «Add thing to spam» or «Fix eggs with ham»

@johnzhou721 johnzhou721 changed the title gh-136061: IDLE - update code in editor.Editor.load_extension gh-136061: IDLE - modernize idlelib code in editor.Editor.load_extension methodname processing Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

8 participants