[C API] Make the PyDescrObject structure opaque: PyDescr_NAME() and PyDescr_TYPE() #90696

Activity

1. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   While working on the PEP-674 implementation, I noticed that PyDescr_NAME() and PyDescr_TYPE() macros are used as l-value by two projects: M2Crypto and mecab-python3. Both are code generated by SWIG.

   M2Crypto-0.38.0/src/SWIG/_m2crypto_wrap.c and mecab-python3-1.0.4/src/MeCab/MeCab_wrap.cpp contain the function:

   SWIGINTERN PyGetSetDescrObject *
   SwigPyStaticVar_new_getset(PyTypeObject *type, PyGetSetDef *getset) {

     PyGetSetDescrObject *descr;
     descr = (PyGetSetDescrObject *)PyType_GenericAlloc(SwigPyStaticVar_Type(), 0);
     assert(descr);
     Py_XINCREF(type);
     PyDescr_TYPE(descr) = type;
     PyDescr_NAME(descr) = PyString_InternFromString(getset->name);
     descr->d_getset = getset;
     if (PyDescr_NAME(descr) == NULL) {
       Py_DECREF(descr);
       descr = NULL;
     }
     return descr;
   }

   ref: https://bugs.python.org/issue45476#msg407410

   SwigPyStaticVar_new_getset() is more of less an outdated copy of Python 2.7 descr_copy() of Python Objects/descrobject.c.

   I tried to prevent using the PyDescr_NAME() and PyDescr_TYPE() macros as l-value in two ways:

   • Add PyDescr_SET_NAME() and PyDescr_SET_Type()
   • Add PyDescr_New()

   The problem of the PyDescr_SET_NAME() and PyDescr_SET_Type() approach is that SWIG code is outdated: it doesn't initialized the PyDescrObject.d_qualname member added to Python 3.3 (bpo-13577, commit 9d57481).

   So I implemented PyDescr_New() function: in CPython, it means basically to rename descr_new() to PyDescr_New(). Problem: implementating this function for older Python versions is non trivial. Code that I wrote for the pythoncapi_compat project:

   // bpo-45476 added PyDescr_New() to Python 3.11.0a5
   #if PY_VERSION_HEX < 0x030B00A5
   PyDescrObject *
   PyDescr_New(PyTypeObject *descr_type, PyTypeObject *type, const char *name)
   {
       assert(descr_type != NULL);
       assert(type != NULL);
       assert(name != NULL);
   
       PyObject *name_obj;
   #if PY_MAJOR_VERSION >= 3
       name_obj = PyUnicode_InternFromString(name);
   #else
       name_obj = PyString_InternFromString(name);
   #endif
       if (name_obj == NULL) {
           return NULL;
       }
   
       PyDescrObject *descr = (PyDescrObject *)PyType_GenericAlloc(descr_type, 0);
       if (descr == NULL) {
           Py_DECREF(name_obj);
           return NULL;
       }
   
       
       descr->d_type = (PyTypeObject*)Py_NewRef(type);
       descr->d_name = name_obj;
       // PyDescrObject.d_qualname was added to Python 3.3.0a1
   #if PY_VERSION_HEX >= 0x030300A1
       descr->d_qualname = NULL;
   #endif
       return descr;
   }
   #endif

   The even more complicated part is to write unit tests for this function in pythoncapi_compat. It requires to implement a whole type with tp_traverse tp_dealloc functions.

   In Python, this function must be at least documented.

   At the end, IMO it's not really worth it to add so much complexity to Python and pythoncapi_compat just for 2 projects using SWIG. I created this issue to keep a track of my analysis, if someone else tries to do something similar tomorrow.

   I attached my WIP patches to keep a copy of this work, in case if we need to revisit this idea later.

   For the PEP-674, IMO it's better to leave the two PyDescr_NAME() and PyDescr_TYPE() macros unchanged. There is no need in the short term to make PyDescrObject structure and structures using it opaque in the short term. Well, SWIG code looks incorrect (it doesn't initialize d_qualname), but a fix just for SWIG should be enough.
2. vstinner

   added

   3.11only security fixesonly security fixes

   topic-C-API

   on Jan 26, 2022
3. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   pydescr_set_type.patch: Python patch to add PyDescr_SET_TYPE() and PyDescr_SET_NAME() functions.
4. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   pydescr_new.patch: Python patch adding the PyDescr_New() function.
5. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   pythoncapi_compat_pydescr_new.patch: pythoncapi_compat patch adding PyDescr_New() function with tests.
6. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   swig_pydescr_new.patch: SWIG patch adding PyDescr_New() and using it.
7. vstinner commented on Jan 26, 2022

   

   vstinner

   on Jan 26, 2022

   MemberAuthor

   More actions

   Since making PyDescrObject opaque is not really worth it, I close this issue.

   I mean, it's worth it, but there are more important structures like PyObject, PyTypeObject or PyListObject.
8. vstinner

   closed this as completedon Jan 26, 2022
9. vstinner

   closed this as completedon Jan 26, 2022
10. vstinner commented on Jan 26, 2022

    

    vstinner

    on Jan 26, 2022

    MemberAuthor

    More actions

    The problem of the PyDescr_SET_NAME() and PyDescr_SET_Type() approach is that SWIG code is outdated: it doesn't initialized the PyDescrObject.d_qualname member added to Python 3.3 (bpo-13577, commit 9d57481).

    I checked: Oh, in practice, it's not an issue since PyType_GenericAlloc() initializes the memory to zero. So the code "just works".
11. vstinner

    changed the title [-][C API] Make the PyDescrObject structure opaque[/-] [+][C API] Make the PyDescrObject structure opaque: PyDescr_NAME() and PyDescr_TYPE()[/+] on Jan 26, 2022
12. vstinner

    changed the title [-][C API] Make the PyDescrObject structure opaque[/-] [+][C API] Make the PyDescrObject structure opaque: PyDescr_NAME() and PyDescr_TYPE()[/+] on Jan 26, 2022
13. ezio-melotti

    transferred this issue fromon Apr 10, 2022