KEMBAR78
gh-128632: fix segfault on nested __classdict__ type param by tom-pytel · Pull Request #128744 · python/cpython · GitHub
Skip to content

gh-128632: fix segfault on nested __classdict__ type param #128744

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
Merged
merged 15 commits into from
Apr 4, 2025
Merged

gh-128632: fix segfault on nested __classdict__ type param #128744

merged 15 commits into from
Apr 4, 2025

Conversation

@tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Jan 11, 2025 •
edited
Loading

Add reserved name check in symtable.c:symtable_visit_type_param_bound_or_default() for __class__ and __classdict__ to prevent segfault when __classdict__ is used.

@bedevere-app
Copy link

bedevere-app bot commented Jan 11, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request Jan 11, 2025
Closed
JelleZijlstra
JelleZijlstra reviewed Jan 11, 2025
View reviewed changes
Lib/test/test_syntax.py

@support.cpython_only
def test_disallowed_type_param_names(self):
# See gh-128632
Copy link
Member

Choose a reason for hiding this comment

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

Could you also test the following related cases:

  • Using a generic function or type alias instead of a class with these names for the type parameters
  • Using the names __classcell__ and __classdictcell__, which appear internally in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added func and alias tests. Didn't add __classcell__ or __classdictcell__ tests as those are not excluded and don't cause problems currently. Do you want to add them to the forbidden list?

>>> class A:
...     class B[__classcell__]: print(type(__classcell__))
...     
<class 'typing.TypeVar'>
>>> class A:
...     class B[__classdictcell__]: print(type(__classdictcell__))
...     
<class 'typing.TypeVar'>

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the tests! I want the *cell* names to also be tested, because they're also somewhat likely to cause problems, even if they don't currently crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I interpret 'somewhat likely to cause problems' to mean yes disallow these names (and test them).

Copy link
Member

Choose a reason for hiding this comment

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

Since they work fine now we should not disallow them, especially because we'd want to backport this change. However, we should have a test to make sure future changes don't introduce crashes with these names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names re-allowed and tests added. Also re-allowed __class__ as that doesn't crash either even though it is special.

JelleZijlstra
JelleZijlstra reviewed Jan 11, 2025
View reviewed changes
@tom-pytel
Copy link
Contributor Author

Just a reminder, according to @ZeroIntensity this needs backporting to 3.13 and 3.12.

@ZeroIntensity
Copy link
Member

I'm going to yield to Jelle. This seems complex enough that backporting could be too risky.

iritkatriel
iritkatriel reviewed Jan 11, 2025
View reviewed changes
@tom-pytel
Copy link
Contributor Author

I'm going to yield to Jelle. This seems complex enough that backporting could be too risky.

Had a look and doesn't look too bad, same functions are present to fix.

@JelleZijlstra JelleZijlstra self-requested a review January 17, 2025 04:40
@JelleZijlstra JelleZijlstra added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Mar 1, 2025
@JelleZijlstra JelleZijlstra merged commit 891c61c into python:main Apr 4, 2025
42 checks passed
@miss-islington-app
Copy link

Thanks @tom-pytel for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @tom-pytel and @JelleZijlstra, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 891c61c1fa480928dd60cce8bbc8764630c95025 3.13

@miss-islington-app
Copy link

Sorry, @tom-pytel and @JelleZijlstra, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 891c61c1fa480928dd60cce8bbc8764630c95025 3.12

@JelleZijlstra
Copy link
Member

@tom-pytel are you interested in doing the backports? You'd need to run the cherry_picker commands from above in your local checkout, then fix the conflicts manually.

@tom-pytel
Copy link
Contributor Author

@tom-pytel are you interested in doing the backports? You'd need to run the cherry_picker commands from above in your local checkout, then fix the conflicts manually.

Sure

tom-pytel added a commit to tom-pytel/cpython that referenced this pull request Apr 4, 2025
@tom-pytel
pythongh-128632: fix segfault on nested __classdict__ type param (pyt…
4b391b1 
…hon#128744)

(cherry picked from commit 891c61c)
tom-pytel added a commit to tom-pytel/cpython that referenced this pull request Apr 4, 2025
@tom-pytel
[3.13] pythongh-128632: fix segfault on nested __classdict__ type par…
617d4e6 
…am (pythonGH-128744)

(cherry picked from commit 891c61c)

Co-authored-by: Tomasz Pytel <tompytel@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Apr 4, 2025

GH-132085 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Apr 4, 2025
@bedevere-app
Copy link

bedevere-app bot commented Apr 4, 2025

GH-132090 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Apr 4, 2025
@tom-pytel
Copy link
Contributor Author

Ping @JelleZijlstra, backports complete.

JelleZijlstra pushed a commit that referenced this pull request Apr 4, 2025
@tom-pytel
[3.13] gh-128632: fix segfault on nested __classdict__ type param (GH…
900dc2b 
…-128744) (#132085)

(cherry picked from commit 891c61c)

Co-authored-by: Tomasz Pytel <tompytel@gmail.com>
@JelleZijlstra
Copy link
Member

Thanks! Merged the 3.13 one and left a comment on the 3.12 one.

seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
@tom-pytel @seehwan80
pythongh-128632: fix segfault on nested __classdict__ type param (pyt…
c5cc52d 
…hon#128744)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

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

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

Assignees

@JelleZijlstra JelleZijlstra

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tom-pytel @ZeroIntensity @JelleZijlstra @iritkatriel