Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-136061: IDLE - update code in editor.Editor.load_extension#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

Open
johnzhou721 wants to merge34 commits intopython:main
base:main
Choose a base branch
Loading
fromjohnzhou721:idledos

Conversation

johnzhou721
Copy link
Contributor

@johnzhou721johnzhou721 commentedMay 29, 2025•
edited by bedevere-appbot
Loading

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

@terryjreedyterryjreedy moved this toIn Progress inIDLE IssuesMay 29, 2025
@terryjreedyterryjreedy self-assigned thisMay 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 inputvevent 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

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

@ZeroIntensityZeroIntensity added type-securityA security issue needs backport to 3.9only security fixes needs backport to 3.10only security fixes needs backport to 3.11only security fixes needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes needs backport to 3.14bugs and security fixes labelsMay 29, 2025
@johnzhou721
Copy link
ContributorAuthor

johnzhou721 commentedMay 29, 2025 via email

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

@johnzhou721
Copy link
ContributorAuthor

johnzhou721 commentedMay 29, 2025 via email

Yes, I think it can be. Will fix.
…
Message ID: ***@***.***>

@kexinoh
Copy link

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

whileTrue:
chars=chars[:-1]
ncharsdeleted=ncharsdeleted+1
have=len(chars.expandtabs(tabwidth))
ifhave<=wantorchars[-1]notin"\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
ContributorAuthor

@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>
@johnzhou721
Copy link
ContributorAuthor

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

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 intest_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).

@terryjreedyterryjreedy changed the titlegh-134873: Avoid quadratic complexities in idlelibgh-134873: IDLE - update code in editor.Editor.load_extensionJun 28, 2025
@terryjreedy
Copy link
Member

terryjreedy commentedJun 28, 2025•
edited
Loading

Still need to delete Security blurb and add IDLE blurb and news items. However

f:\dev\3x>git pushhttps://github.com/johnzhou721/cpython pr_134874:idledos

remote: Permission to johnzhou721/cpython.git denied to terryjreedy.
fatal: unable to access 'https://github.com/johnzhou721/cpython/': The requested URL returned error: 403

This is my standard pr revision push template. with pr#,johnzhou721idledos taken from top of this page.

@zware Do you have any idea what is wrong? Is the fact that johnzhou forked from somewhere else than python/cpython relevant?
@johnzhou721 Did you uncheck [] allow others to edit pr? The fact that I edited online suggests not.

Edit: deleted blurb on View files page. Unfortunately, that triggers retest, which will fail because no blurb.

Blurb-it failed also due to lack of access to PR.

@serhiy-storchakaserhiy-storchaka changed the titlegh-134873: IDLE - update code in editor.Editor.load_extensiongh-136061: IDLE - update code in editor.Editor.load_extensionJun 28, 2025
@johnzhou721
Copy link
ContributorAuthor

@terryjreedy Thanks for looking into this issue and resolving my suspicions. I will be blurbing it.

Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

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

Please make a change and an addition. Change the blurb to say
Update code in editor.Editor.load_extension. Patch by johnzhou and Zachary Ware.

Then add the same + the prefexgh-134873: to 'idlelib/News3.txt'. The result should be that the top 10 lines of the file looks like the following, with 2 blank lines above and 1 below the addition. (These blank lines matter as they enable clean backports!)

What's New in IDLE 3.14.0(since 3.13.0)Released on 2025-10-07=========================gh-134873: Update code in editor.Editor.load_extension.Patch by johnzhou and Zachary Ware.gh-112936: IDLE - Include Shell menu in single-process mode,

@bedevere-app
Copy link

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 phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@serhiy-storchaka
Copy link
Member

Is it needed? This change is mainly cosmetic, it has almost no effect on users.

Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

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

After further review, I realize that we can replace 6 lines with one line to convert stringvevent to stringmethodname in one expression that serves as a 'template' of the changes needed. See comment.

@serhiy-storchaka I don't think this change is needed as a security feature. To me, it makes the code a bit easier to read and understand.

If you veto merging the change (and close the issue as "won't fix"), I would instead want to add and merge a comment as to the equivalent expression, for whenever I or someone were to do a more thorough review of the extension functions. (I already noticed that load failure prints "Failed to import extension" to console twice, and aborts IDLE startup. Since essential IDLE functions were converted from 'extensions' to normal features years ago, stopping is no longer appropriate.)

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

zware and johnzhou721 reacted with thumbs up emoji

@johnzhou721
Copy link
ContributorAuthor

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

Thanks for making the requested changes!

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

@zware
Copy link
Member

@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 tohttps:// 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 trygit push git@github.com:johnzhou721/cpython pr_134874:idledos instead.

Co-authored-by: Zachary Ware <zachary.ware@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@zwarezwarezware left review comments

@picnixzpicnixzpicnixz left review comments

@ZeroIntensityZeroIntensityAwaiting requested review from ZeroIntensity

@terryjreedyterryjreedyAwaiting requested review from terryjreedyterryjreedy is a code owner

Assignees

@terryjreedyterryjreedy

Labels
Projects
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@johnzhou721@terryjreedy@kexinoh@ZeroIntensity@picnixz@zware@serhiy-storchaka
[8]ページ先頭
©2009-2025 Movatter.jp