From 6790f48f849d7c57711711f73833182f9188c7c8 Mon Sep 17 00:00:00 2001 From: Nir Schulman Date: Sun, 21 Aug 2022 23:36:28 +0300 Subject: [PATCH 1/2] Fixed instances of types.ClassMethodDescriptorType not being pickled 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. --- Doc/whatsnew/3.12.rst | 3 ++ Lib/test/pickletester.py | 4 ++- ...2-08-21-22-00-51.gh-issue-95196.MwXQ1l.rst | 1 + Objects/descrobject.c | 31 ++++++++++++++++++- 4 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-08-21-22-00-51.gh-issue-95196.MwXQ1l.rst diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index f9fa8ac3123198..56caa2c943d2c7 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -83,6 +83,9 @@ Other Language Changes mapping is hashable. (Contributed by Serhiy Storchaka in :gh:`87995`.) +* :class:`types.ClassMethodDescriptorType` instances are now pickled correctly. + (Contributed by Nir Schulman in :gh:`95196`.) + New Modules =========== diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 21419e11c87497..5e91dfba0eaf5f 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -2795,8 +2795,10 @@ class Nested(str): ({1, 2}.__contains__, (2,)), # unbound "coexist" method (set.__contains__, ({1, 2}, 2)), - # built-in class method + # built-in bound class method (dict.fromkeys, (("a", 1), ("b", 2))), + # built-in unbound class method + (dict.__dict__["fromkeys"], (dict, ("a", 1), ("b", 2))), # built-in static method (bytearray.maketrans, (b"abc", b"xyz")), # subclass methods diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-08-21-22-00-51.gh-issue-95196.MwXQ1l.rst b/Misc/NEWS.d/next/Core and Builtins/2022-08-21-22-00-51.gh-issue-95196.MwXQ1l.rst new file mode 100644 index 00000000000000..248a7e0772aa27 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-08-21-22-00-51.gh-issue-95196.MwXQ1l.rst @@ -0,0 +1 @@ +Fixed :class:`types.ClassMethodDescriptorType` being pickled incorrectly. diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 82570e085143ed..2c9a7e52936976 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -633,11 +633,40 @@ descr_reduce(PyDescrObject *descr, PyObject *Py_UNUSED(ignored)) PyDescr_TYPE(descr), PyDescr_NAME(descr)); } + +static PyObject* +classmethoddescr_reduce(PyDescrObject* descr, PyObject* Py_UNUSED(ignored)) +{ + /* Ideally, we would want to use a different callable than eval in + order to get the descriptor. However, we need to ensure the pickled + object will not cause errors upon unpickling in older versions. */ + PyObject* evalFunctionName = PyUnicode_FromString("eval"); + if (evalFunctionName == NULL) { + return NULL; + } + PyObject* eval = _PyEval_GetBuiltin(evalFunctionName); + Py_DECREF(evalFunctionName); + + if (eval == NULL) { + return NULL; + } + + return Py_BuildValue("N(s, N, {s:O, s:O})", + eval, "cls.__dict__[name]", Py_None, + "cls", PyDescr_TYPE(descr), "name", PyDescr_NAME(descr) + ); +} + static PyMethodDef descr_methods[] = { {"__reduce__", (PyCFunction)descr_reduce, METH_NOARGS, NULL}, {NULL, NULL} }; +static PyMethodDef classmethoddescr_methods[] = { + {"__reduce__", (PyCFunction)classmethoddescr_reduce, METH_NOARGS, NULL}, + {NULL, NULL} +}; + static PyMemberDef descr_members[] = { {"__objclass__", T_OBJECT, offsetof(PyDescrObject, d_type), READONLY}, {"__name__", T_OBJECT, offsetof(PyDescrObject, d_name), READONLY}, @@ -776,7 +805,7 @@ PyTypeObject PyClassMethodDescr_Type = { 0, /* tp_weaklistoffset */ 0, /* tp_iter */ 0, /* tp_iternext */ - descr_methods, /* tp_methods */ + classmethoddescr_methods, /* tp_methods */ descr_members, /* tp_members */ method_getset, /* tp_getset */ 0, /* tp_base */ From 9d625accc5d2f3cce335371c7c9a54c187cb40cf Mon Sep 17 00:00:00 2001 From: Nir Schulman Date: Sun, 28 Aug 2022 21:27:44 +0300 Subject: [PATCH 2/2] Added _Py_ID() for the builtin eval function and changed the reduce function for instances of types.ClassMethodDescriptorType in order to use it --- Include/internal/pycore_global_strings.h | 1 + Include/internal/pycore_runtime_init_generated.h | 7 +++++++ Objects/descrobject.c | 13 +------------ 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h index aada220395023d..13c80732ef81f5 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -341,6 +341,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(endpos) STRUCT_FOR_ID(env) STRUCT_FOR_ID(errors) + STRUCT_FOR_ID(eval) STRUCT_FOR_ID(event) STRUCT_FOR_ID(eventmask) STRUCT_FOR_ID(exc_type) diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h index 09890cd812015b..5032b5528ff3e5 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -850,6 +850,7 @@ extern "C" { INIT_ID(endpos), \ INIT_ID(env), \ INIT_ID(errors), \ + INIT_ID(eval), \ INIT_ID(event), \ INIT_ID(eventmask), \ INIT_ID(exc_type), \ @@ -2002,6 +2003,8 @@ _PyUnicode_InitStaticStrings(void) { PyUnicode_InternInPlace(&string); string = &_Py_ID(errors); PyUnicode_InternInPlace(&string); + string = &_Py_ID(eval); + PyUnicode_InternInPlace(&string); string = &_Py_ID(event); PyUnicode_InternInPlace(&string); string = &_Py_ID(eventmask); @@ -5927,6 +5930,10 @@ _PyStaticObjects_CheckRefcnt(void) { _PyObject_Dump((PyObject *)&_Py_ID(errors)); Py_FatalError("immortal object has less refcnt than expected _PyObject_IMMORTAL_REFCNT"); }; + if (Py_REFCNT((PyObject *)&_Py_ID(eval)) < _PyObject_IMMORTAL_REFCNT) { + _PyObject_Dump((PyObject *)&_Py_ID(eval)); + Py_FatalError("immortal object has less refcnt than expected _PyObject_IMMORTAL_REFCNT"); + }; if (Py_REFCNT((PyObject *)&_Py_ID(event)) < _PyObject_IMMORTAL_REFCNT) { _PyObject_Dump((PyObject *)&_Py_ID(event)); Py_FatalError("immortal object has less refcnt than expected _PyObject_IMMORTAL_REFCNT"); diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 2c9a7e52936976..0362490f40e4a4 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -640,19 +640,8 @@ classmethoddescr_reduce(PyDescrObject* descr, PyObject* Py_UNUSED(ignored)) /* Ideally, we would want to use a different callable than eval in order to get the descriptor. However, we need to ensure the pickled object will not cause errors upon unpickling in older versions. */ - PyObject* evalFunctionName = PyUnicode_FromString("eval"); - if (evalFunctionName == NULL) { - return NULL; - } - PyObject* eval = _PyEval_GetBuiltin(evalFunctionName); - Py_DECREF(evalFunctionName); - - if (eval == NULL) { - return NULL; - } - return Py_BuildValue("N(s, N, {s:O, s:O})", - eval, "cls.__dict__[name]", Py_None, + _PyEval_GetBuiltin(&_Py_ID(eval)), "cls.__dict__[name]", Py_None, "cls", PyDescr_TYPE(descr), "name", PyDescr_NAME(descr) ); }