Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-94822: Don't specialize when metaclasses are involved #94892

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jul 16, 2022

This patch changes LOAD_ATTR_CLASS to fail if the class has a metaclass other than type, or if the named attribute is present on type itself. It also adds a bunch of regression tests for nasty edge-cases when caching class attributes and methods.

Supersedes #94863 (see #94863 (comment)).

@brandtbucher brandtbucher added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) needs backport to 3.11 only security fixes labels Jul 16, 2022
@brandtbucher brandtbucher self-assigned this Jul 16, 2022
@Fidget-Spinner
Copy link
Member

Thanks for the quick fix. I think this LGTM. IIRC speaking to Mark at EuroPython he had other ideas about this though (something about just blocking non-immutable classes IIRC). Let's wait for his reply on Monday.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the extra tests.

Would it make sense to add a function to test.support, so we can replace
for _ in range(1025): can be replaced with for _ in test.support.UntilSpecialized():?

@@ -945,6 +945,10 @@ specialize_class_load_attr(PyObject *owner, _Py_CODEUNIT *instr,
PyObject *name)
{
_PyLoadMethodCache *cache = (_PyLoadMethodCache *)(instr + 1);
if (!PyType_CheckExact(owner) || _PyType_Lookup(Py_TYPE(owner), name)) {
Copy link
Member

Choose a reason for hiding this comment

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

You could safely generalize this to any immutable metaclass.
Py_TYPE(owner)->tp_flags & Py_TPFLAGS_IMMUTABLETYPE

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, you're only referring to the PyType_CheckExact call, right? The _PyType_Lookup call stays?

Wouldn't we also need to add something like Py_TYPE(owner)->tp_getattro == PyType_Type.tp_getattro too to protect against immutable types that define __getattribute__?

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, you're only referring to the PyType_CheckExact call, right?

Yes

Wouldn't we also need to add something like Py_TYPE(owner)->tp_getattro == PyType_Type.tp_getattro too

And probably some other special case I've forgotten.
Let's just stick with the PyType_CheckExact(owner) to be on the safe side.

@brandtbucher brandtbucher merged commit daf68ba into python:main Jul 18, 2022
@miss-islington
Copy link
Contributor

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @brandtbucher, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker daf68ba92f315bfd239a0c993f9f683fb90325fb 3.11

brandtbucher added a commit to brandtbucher/cpython that referenced this pull request Jul 18, 2022
@bedevere-bot
Copy link

GH-94980 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 18, 2022
brandtbucher added a commit that referenced this pull request Jul 18, 2022
@brandtbucher brandtbucher deleted the no-metaclasses branch July 21, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants