Repository navigation
gh-105250: Fix NEWOBJ handling of custom metaclasses in C pickle #155920
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
10399f7
f22a1a5
25924ec
25eb25a
b4dfe45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Fix a difference in behavior between the C and pure Python implementations of | ||
| :mod:`pickle` when unpickling with the ``NEWOBJ`` and ``NEWOBJ_EX`` opcodes: | ||
| the C implementation now performs an attribute lookup of ``cls.__new__`` when | ||
| the class has a custom metaclass, so that a metaclass ``__getattribute__`` | ||
| hook observes the lookup as documented. Patched by Chaewon Kim |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6328,7 +6328,36 @@ load_newobj(PickleState *state, UnpicklerObject *self, int use_kwargs) | |||||||||||||||||||||||||||||||||||||||||||||
| goto error; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| obj = ((PyTypeObject *)cls)->tp_new((PyTypeObject *)cls, args, kwargs); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (Py_TYPE(cls) == &PyType_Type) { | ||||||||||||||||||||||||||||||||||||||||||||||
| /* Fast path: with the default metaclass, an attribute lookup of | ||||||||||||||||||||||||||||||||||||||||||||||
| cls.__new__ is not observable, so tp_new can be called | ||||||||||||||||||||||||||||||||||||||||||||||
| directly. */ | ||||||||||||||||||||||||||||||||||||||||||||||
| obj = ((PyTypeObject *)cls)->tp_new((PyTypeObject *)cls, args, | ||||||||||||||||||||||||||||||||||||||||||||||
| kwargs); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||||||||
| /* Look __new__ up on the class so that a custom metaclass | ||||||||||||||||||||||||||||||||||||||||||||||
| __getattribute__ observes the lookup, as in the Python | ||||||||||||||||||||||||||||||||||||||||||||||
| implementation. */ | ||||||||||||||||||||||||||||||||||||||||||||||
| PyObject *func = PyObject_GetAttr(cls, &_Py_ID(__new__)); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (func == NULL) { | ||||||||||||||||||||||||||||||||||||||||||||||
| goto error; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| Py_ssize_t nargs = PyTuple_GET_SIZE(args); | ||||||||||||||||||||||||||||||||||||||||||||||
| PyObject *newargs = PyTuple_New(nargs + 1); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (newargs == NULL) { | ||||||||||||||||||||||||||||||||||||||||||||||
| Py_DECREF(func); | ||||||||||||||||||||||||||||||||||||||||||||||
| goto error; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| PyTuple_SET_ITEM(newargs, 0, Py_NewRef(cls)); | ||||||||||||||||||||||||||||||||||||||||||||||
| for (Py_ssize_t i = 0; i < nargs; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||
| PyTuple_SET_ITEM(newargs, i + 1, | ||||||||||||||||||||||||||||||||||||||||||||||
| Py_NewRef(PyTuple_GET_ITEM(args, i))); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| obj = PyObject_Call(func, newargs, kwargs); | ||||||||||||||||||||||||||||||||||||||||||||||
| Py_DECREF(newargs); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can you benchmark this one too?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I applied it(b4dfe45), and re-ran the benchmark to see whether the small-stack call path helps: |
||||||||||||||||||||||||||||||||||||||||||||||
| Py_DECREF(func); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| if (obj == NULL) { | ||||||||||||||||||||||||||||||||||||||||||||||
| goto error; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to move look up count as class varaible
Something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f22a1a5 — moved the count to a class variable on the metaclass.