Skip to content

gh-128636: Fix crash in PyREPL when os.environ is overwritten with an invalid value for mac #138089

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

gh-128636: Fix crash in PyREPL when os.environ is overwritten with an invalid value for mac #138089

wants to merge 15 commits into from
+31 −9

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented Aug 23, 2025 •
edited by bedevere-app bot
Loading

for mac if environ is valid and it will retrigger all of it

so I checked the code base in repr the os is from init
so make it in__init__ is better and can avoid

and that is not enough, we also need fix it the same way in

_can_colorize theme

cc @pablogsal

in my env it fixed the issue, and I wonder it maybe a litter faster

@yihong0618
fix: close issue python#128636 for mac make is_mac at init
1b8a228 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
This was referenced Aug 23, 2025
Open
Merged
@yihong0618
Copy link
Contributor Author

the news is the same so maybe we can ignore it?

@yihong0618
fix: mypy check issue
ae9d55f 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@picnixz picnixz changed the title [3.13] gh-128636: Fix crash in PyREPL when os.environ is overwritten with an invalid value for mac(GH-128653) gh-128636: Fix crash in PyREPL when os.environ is overwritten with an invalid value for mac Aug 23, 2025
@yihong0618
Copy link
Contributor Author

Why only 3.13?

only up to 3.13 new repr has this issue follow this patch title #129186

@picnixz
Copy link
Member

picnixz commented Aug 23, 2025

the news is the same so maybe we can ignore it?

No, because we have already shipped a few releases since then, so we need a fresh one.

picnixz
picnixz reviewed Aug 23, 2025
View reviewed changes
@yihong0618
Copy link
Contributor Author

the news is the same so maybe we can ignore it?

No, because we have already shipped a few releases since then, so we need a fresh one.

learned that, thank you very much

@picnixz
Copy link
Member

picnixz commented Aug 23, 2025

only up to 3.13 new repr has this issue follow this patch title #129186

That's the backport. All PRs must first be done against main and we backport them when needed. We only use [3.x] when the fix is specific to the version.

@yihong0618
Copy link
Contributor Author

only up to 3.13 new repr has this issue follow this patch title #129186

That's the backport. All PRs must first be done against main and we backport them when needed. We only use [3.x] when the fix is specific to the version.

copy that, sorry for my wrong understand at first

@yihong0618
fix: delete copilot tab useless comments
b838c04 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
picnixz
picnixz reviewed Aug 23, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure but caching THEME() seems to go against the comment in THEME().

@yihong0618
fix: address comments
738d41e 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

yihong0618 commented Aug 23, 2025 •
edited
Loading

I'm not entirely sure but caching THEME() seems to go against the comment in THEME().

addressed thank you

and add tests

@yihong0618
fix: add tests for the change
a0a5cf3 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
fix: test is wrong
2e9faf7 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Merge branch 'main' into hy/close_issue_128636_for_mac
3a25033
@yihong0618 yihong0618 requested a review from picnixz August 24, 2025 08:12
picnixz
picnixz previously requested changes Aug 24, 2025
View reviewed changes
@bedevere-app
Copy link

bedevere-app bot commented Aug 24, 2025

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

@yihong0618
fix: address review comments
c7cbd22 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Merge branch 'hy/close_issue_128636_for_mac' of https://github.com/yi…
53aa617 
…hong0618/cpython into hy/close_issue_128636_for_mac
picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
Lib/test/test_pyrepl/test_unix_console.py Outdated
@@ -303,3 +303,15 @@ def test_getheightwidth_with_invalid_environ(self, _os_write):
self.assertIsInstance(console.getheightwidth(), tuple)
os.environ = []
self.assertIsInstance(console.getheightwidth(), tuple)

def test_mac_with_invalid_environ(self, _os_write):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_mac_with_invalid_environ(self, _os_write):
def test_is_mac_with_invalid_environ(self, _os_write):

picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
@picnixz
Copy link
Member

picnixz commented Aug 24, 2025 •
edited
Loading

The change with is_mac affects restore(). Please test this (if it's possible). Otherwise, the test might not be necessary. More generally, we should test the functions or methods that may call os.environ.* during the interactive session.

@yihong0618
Copy link
Contributor Author

The change with is_mac affects restore(). Please test this (if it's possible). Otherwise, the test might not be necessary. More generally, we should test the functions or methods that may call os.environ.* during the interactive session.

copy will try this way but seems not simple for testing let me try

@yihong0618
fix: address comments better test for it
02e8f9d 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

The change with is_mac affects restore(). Please test this (if it's possible). Otherwise, the test might not be necessary. More generally, we should test the functions or methods that may call os.environ.* during the interactive session.

@picnixz try a better one, learned something from this, thank you again

without this patch(on main)

image

with this patch

image

picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
picnixz
picnixz reviewed Aug 24, 2025
View reviewed changes
@yihong0618 @picnixz
fix: better way for already colorized
e4acbd0 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@yihong0618 yihong0618 requested a review from hugovk as a code owner August 24, 2025 13:49
@yihong0618
Copy link
Contributor Author

yihong0618 commented Aug 24, 2025 •
edited
Loading

addressed and add co-author thanks for the kindly help

and FYI, the new way is better but not so simple.

tested about the theme change, the code is right, but already_colorize maybe can change to a better name

@yihong0618
fix: mypy check
aa37996 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
fix: useless code check
72867e0 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618 yihong0618 requested a review from picnixz August 25, 2025 00:32
@ambv
Copy link
Contributor

ambv commented Aug 25, 2025

While I appreciate the willingness to fix this, I don't really like an argument to can_colorize that amounts to "please return True". It's bad form.

ambv
ambv reviewed Aug 25, 2025
View reviewed changes
@yihong0618
Copy link
Contributor Author

agreed but the two can_color is already there one is function another is a boolean, Can we refactor theme? Or there's other way to fix this

@picnixz picnixz removed their request for review August 25, 2025 12:47
@picnixz picnixz dismissed their stale review August 25, 2025 12:48

I'm letting Łukasz shepherd this as I'm not a REPL expert

ambv
ambv reviewed Aug 25, 2025
View reviewed changes
@yihong0618
fix: address commnets better way with safe_get
fe34228 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

fixed thank you for the help

@yihong0618 yihong0618 requested a review from ambv August 25, 2025 13:49
hugovk
hugovk reviewed Aug 25, 2025
View reviewed changes
@yihong0618
fix: address comments again
3177b0a 
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618 yihong0618 requested a review from hugovk August 25, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@ambv ambv Awaiting requested review from ambv ambv is a code owner

@hugovk hugovk Awaiting requested review from hugovk hugovk is a code owner

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yihong0618 @picnixz @ambv @hugovk