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-95196: Fixed instances of types.ClassMethodDescriptorType not being pickled correctly #96162

Conversation

UltimateLobster
Copy link

@UltimateLobster UltimateLobster commented Aug 21, 2022

NOTE: I want to apologize for anything that is not up to par in this PR. This is my first time writing to cpython (and writing in python's C-API in general), I would love to learn from any mistakes done here and improve for the next time. I tried my best to look at other examples and guides and derive the correct conventions and workflow from there.

Previously, instances of types.ClassMethodDescriptorType were reduced
into a wrong set of instructions. Instead of the instructions retrieving
the descriptor itself (using direct access to the object's __dict__),
the instructions retrieved the underlying classmethod (using the normal getattr)
which resulted in different object upon unpickling.
This change has replaced the implementation of __reduce__ for such
instances so it will directly access the owner's __dict__.

Since there is no publicly available builtin function (That I could
find) to get an attribute directly from the object's __dict__, and in
order for the implmentation to work with older versions of python, I
unfortunately could not avoid the usage of the "eval" builtin in the
implementation.

correctly

Previously, instances of types.ClassMethodDescriptorType were reduced
into a wrong set of instructions. Instead of instructions to retreive
the descriptor itself (using direct access to the owner's __dict__),
the instructions retrieved the underlying classmethod (using getattr).
This change has replaced the implementation of __reduce__ for these
instances so it will directly access the owner's __dict__.

Since there is no publicly available builtin function (That I could
find) to get an attribute directly from the object's __dict__, and in
order for the implmentation to work with older versions of python, I
unfortunately could not avoid the usage of the "eval" builtin in the
implementation.
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 21, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@Fidget-Spinner
Copy link
Member

Sorry for missing this. I'll try to review before the end of the week.

@UltimateLobster
Copy link
Author

Sorry for missing this. I'll try to review before the end of the week.

Thank you :D

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

@serhiy-storchaka can you please help verify if this is the correct way to solve this problem?

…unction for instances of types.ClassMethodDescriptorType in order to use it
@UltimateLobster UltimateLobster requested review from Fidget-Spinner and removed request for serhiy-storchaka August 28, 2022 18:31
@serhiy-storchaka
Copy link
Member

Using eval() here looks clever, but perhaps too clever. One of drawbacks of eval() is that it is much slower than simple attribute resolution and dict lookup. Other is that it looks suspicious (even if it is completely safe in this case) and can trigger alarms.

In any case, I think that it is better to just disable pickling of types.ClassMethodDescriptorType, for compatibility with Python implemented classmethod descriptors and with C and Python implemented staticmethod descriptors, and because there are no use common cases for this (otherwise we would get such report years ago).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect pickling behavior in the c implemented classmethod descriptors
6 participants