[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type #83754

Closed vstinner closed 3 years ago

BPO

39573

Nosy

@vstinner

PRs

python/cpython#18388

python/cpython#18389

python/cpython#18390

python/cpython#18391

python/cpython#18392

python/cpython#18393

python/cpython#18394

python/cpython#18398

python/cpython#18400

python/cpython#18402

python/cpython#18411

python/cpython#18419

python/cpython#18488

python/cpython#18496

python/cpython#18507

python/cpython#18508

python/cpython#18521

python/cpython#18601

python/cpython#18789

python/cpython#18798

python/cpython#18799

python/cpython#18804

python/cpython#18809

python/cpython#19882

python/cpython#19975

python/cpython#20290

python/cpython#20391

python/cpython#20429

python/cpython#20610

python/cpython#21262

python/cpython#21433

python/cpython#23366

python/cpython#23375

python/cpython#26493

python/cpython#26550

python/cpython#26596

python/cpython#28128

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['expert-C-API', '3.11'] title = '[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type' updated_at = user = 'https://github.com/vstinner' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'none' closed = True closed_date = closer = 'vstinner' components = ['C API'] creation = creator = 'vstinner' dependencies = [] files = [] hgrepos = [] issue_num = 39573 keywords = ['patch'] message_count = 96.0 messages = ['361513', '361514', '361515', '361516', '361517', '361518', '361519', '361522', '361523', '361526', '361527', '361529', '361531', '361540', '361549', '361555', '361557', '361590', '361593', '361607', '361611', '361626', '361631', '361639', '361904', '361960', '361961', '361963', '361964', '361965', '361971', '361977', '361987', '361988', '362033', '362034', '362133', '362134', '362166', '362212', '362216', '362445', '363345', '363494', '363564', '365690', '366473', '366493', '368047', '369896', '369898', '370074', '370303', '370638', '370663', '370665', '370666', '370671', '370729', '370902', '370932', '372308', '373460', '379675', '379679', '379680', '379757', '379759', '381337', '381345', '381365', '381374', '381403', '381404', '382260', '382534', '382539', '382780', '382781', '382783', '394954', '394971', '395018', '395205', '395206', '395287', '395323', '395536', '401365', '401370', '401378', '401395', '401396', '401399', '403252', '410995'] nosy_count = 1.0 nosy_names = ['vstinner'] pr_nums = ['18388', '18389', '18390', '18391', '18392', '18393', '18394', '18398', '18400', '18402', '18411', '18419', '18488', '18496', '18507', '18508', '18521', '18601', '18789', '18798', '18799', '18804', '18809', '19882', '19975', '20290', '20391', '20429', '20610', '21262', '21433', '23366', '23375', '26493', '26550', '26596', '28128'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue39573' versions = ['Python 3.11'] ```

Linked PRs

gh-120599

New changeset 7d847e29d76b178c2db66b180065771b4d90c78f by Dong-hee Na in branch 'master': bpo-39573: Fix buildbot failure for tupleobject.c (GH-20391) https://github.com/python/cpython/commit/7d847e29d76b178c2db66b180065771b4d90c78f

New changeset fe2978b3b940fe2478335e3a2ca5ad22338cdf9c by Victor Stinner in branch 'master': bpo-39573: Convert Py_REFCNT and Py_SIZE to functions (GH-20429) https://github.com/python/cpython/commit/fe2978b3b940fe2478335e3a2ca5ad22338cdf9c

bpo-39573: Convert Py_TYPE() to a static inline function (GH-20290)

This change broke two projects:

Cython: https://github.com/cython/cython/commit/d8e93b332fe7d15459433ea74cd29178c03186bd (FIXED)
immutables: https://github.com/MagicStack/immutables/issues/46

bpo-39573: Convert Py_TYPE() to a static inline function (GH-20290)

This change broke pycurl: https://github.com/pycurl/pycurl/pull/636

Extract of its current code: """ /* Initialize the type of the new type objects here; doing it here

is required for portability to Windows without requiring C++. */ p_Curl_Type = &Curl_Type; p_CurlMulti_Type = &CurlMulti_Type; p_CurlShare_Type = &CurlShare_Type; Py_TYPE(&Curl_Type) = &PyType_Type; Py_TYPE(&CurlMulti_Type) = &PyType_Type; Py_TYPE(&CurlShare_Type) = &PyType_Type; """

To port code to Python 3.10, the following macro can be copied/pasted in your code. It defines Py_SET_SIZE() if it's not defined.

#if PY_VERSION_HEX < 0x030900A4
#  define Py_SET_SIZE(obj, size) do { Py_SIZE(obj) = (size); } while (0)
#endif

Similar macro for Py_SET_TYPE:

#if PY_VERSION_HEX < 0x030900A4
#  define Py_SET_TYPE(obj, size) do { Py_TYPE(obj) = (size); } while (0)
#endif

numpy fix defines Py_SET_TYPE() and Py_SET_SIZE() on old Python versions:

In the whole numpy code base, only 5 lines have to be modified!

I proposed PR 20610 to enhance the documentation explaining how to port existing to code to Py_SET_SIZE() & cie.

New changeset dc24b8a2ac32114313bae519db3ccc21fe45c982 by Victor Stinner in branch 'master': bpo-39573: Porting to Python 3.10: Py_SET_SIZE() macro (GH-20610) https://github.com/python/cpython/commit/dc24b8a2ac32114313bae519db3ccc21fe45c982

Note: numpy was updated to also the use the macros using ", (void)0": https://github.com/numpy/numpy/commit/f1671076c80bd972421751f2d48186ee9ac808aa

See also bpo-40881 "--with-valgrind broken": unicode_release_interned() still used "Py_REFCNT(s) += 1;". It's now fixed by commit c96a61e8163c2d25ed4ac77cf96201fd0bdb945c.

Another idea would be to convert some C extensions of the standard library to the limited C API. It would ensure that the limited C API contains enough functions to be useful, but would also notify us directly if the API is broken.

I created bpo-41111: "Convert a few stdlib extensions to the limited C API".

New changeset 8182cc2e68a3c6ea5d5342fed3f1c76b0521fbc1 by Victor Stinner in branch 'master': bpo-39573: Use the Py_TYPE() macro (GH-21433) https://github.com/python/cpython/commit/8182cc2e68a3c6ea5d5342fed3f1c76b0521fbc1

This also breaks pycurl:

https://github.com/pycurl/pycurl/pull/660

And breezy:

https://bugzilla.redhat.com/show_bug.cgi?id=1890880 (not yet reported upstream)

I don't understand the rationale for this change in depth, but does the benefit outweigh (yet another) backwards incompatibility?

I don't understand the rationale for this change in depth, but does the benefit outweigh (yet another) backwards incompatibility?

I think you can have it both ways. Do you want a C API that is stable over a long period of CPython releases or do you want to continue to be able to look deep (i.e. non opaque PyObject*) into CPython implementation internals?

During the sprint last week, we talked about how to provide a compatible API, similar to what Pypy cpyext does. It would be possible to provide a (nearly) fully compatible API with the approach. It could get quite painful for CPython to maintain such a thing however. E.g. cpyext has proxy objects (to maintain CPython compatible structure layouts) but keeping those proxies in sync with the internal VM object structures is expensive and tricky.

Certainly making PyObject opaque is going to break some 3rd party code. Making it opaque for the non-limited API is not an option IMHO because it breaks too much 3rd party code. Is making it opaque for the limited C API going to break too much code? Maybe, I don't know. Thanks for pointing out pycurl and breezy.

Correction: I think you *cannot* have it both ways.

Miro:

This also breaks pycurl: https://github.com/pycurl/pycurl/pull/660

Right, see my previous comment, another PR was already proposed in May!

me:

This change broke pycurl: https://github.com/pycurl/pycurl/pull/636

Merged pycurl fix: https://github.com/pycurl/pycurl/commit/e633f9a1ac4df5e249e78c218d5fbbd848219042

Miro:

I don't understand the rationale for this change in depth, but does the benefit outweigh (yet another) backwards incompatibility?

I wrote PEP-620 "Hide implementation details from the C API" to explain the rationale: https://www.python.org/dev/peps/pep-0620/

Another batch of broken projects:

Those are just the initial set of packages we have discovered so far. I think there will be more.

I wrote PR 23366 to revert the Py_TYPE() and Py_SIZE() changes: convert them back to macros to allow again "Py_TYPE(obj) = type;" and "Py_SIZE(obj) = size;" syntaxes.

Miro Hrončok:

Another batch of broken projects: PyPAM https://bugzilla.redhat.com/show_bug.cgi?id=1897264

"Py_TYPE="

bitarray https://bugzilla.redhat.com/show_bug.cgi?id=1897536

"Py_TYPE=/Py_SIZE="

boost https://bugzilla.redhat.com/show_bug.cgi?id=1896382

"Py_TYPE="

duplicity https://bugzilla.redhat.com/show_bug.cgi?id=1896684

"Py_TYPE="

gobject-introspection https://bugzilla.redhat.com/show_bug.cgi?id=1893194

"Py_TYPE="

mercurial https://bugzilla.redhat.com/show_bug.cgi?id=1897178

"Py_TYPE=/Py_SIZE="

pybluez https://bugzilla.redhat.com/show_bug.cgi?id=1897256

"Py_TYPE="

pygobject3 https://bugzilla.redhat.com/show_bug.cgi?id=1894522

"Py_TYPE="

pylibacl https://bugzilla.redhat.com/show_bug.cgi?id=1897529

"Py_TYPE="

pyside2 https://bugzilla.redhat.com/show_bug.cgi?id=1898974

"Py_TYPE="

rdiff-backup https://bugzilla.redhat.com/show_bug.cgi?id=1898980

"Py_TYPE="

Those are just the initial set of packages we have discovered so far. I think there will be more.

Well, since the PEP-620 is not accepted, I prefer to revert the Py_TYPE() and the Py_SIZE() changes for now. We should have a wider discussion on how to introduce incompatible changes into the C API before being able to push more incompatible changes which impact a so wide range of C extension modules.

One practical problem is how to estimate the number of broken Python projects to decide if an incompatible change can be introduced or not. When I did early experiment before merging the PR 20290, it seems like only a minority of C extensions rely on "Py_TYPE(obj) = type;" syntax. It's a common pattern to define a type statically. Pseudo-code:

--- PyTypeObject MyType = {...};

PyInit_MyExtension(...)
{
   Py_TYPE(&MyType) = ...;
   PyType_Ready(&MyType);
   ...
}

"Py_TYPE(&MyType) = ...;" is required since some C compilers don't support setting ob_type directly in the MyType static declaration. The type must be set at runtime.

Also I considered that the change is trivial enough to be accepable. Well, I was wrong, and that's why I'm not proposing to revert thes changes.

About the rationale for introducing C API incompatible changes, see the PEP-620.

New changeset 0e2ac21dd4960574e89561243763eabba685296a by Victor Stinner in branch 'master': bpo-39573: Convert Py_TYPE() and Py_SIZE() back to macros (GH-23366) https://github.com/python/cpython/commit/0e2ac21dd4960574e89561243763eabba685296a

New changeset e0251787d85950538cf2490c2c73cc680b153940 by Miro Hrončok in branch 'master': bpo-39573: Remove What's new entry for Py_SIZE() (GH-23375) https://github.com/python/cpython/commit/e0251787d85950538cf2490c2c73cc680b153940

And breezy: https://bugzilla.redhat.com/show_bug.cgi?id=1890880 (not yet reported upstream)

Oh, I didn't notice that this project is broken by the Py_REFCNT() change. I expected it to be broken by the Py_TYPE() change as others.

Should we revert the Py_REFCNT() change as well? So far, breezy is the only impacted project, and the fix should be simple, no?

breezy uses "Py_REFCNT(self) -= 1;" instead of "Py_DECREF(self);" in its StaticTuple_Intern() function:

static StaticTuple *
StaticTuple_Intern(StaticTuple *self)
{
    PyObject *canonical_tuple = NULL;

    if (_interned_tuples == NULL || _StaticTuple_is_interned(self)) {
        Py_INCREF(self);
        return self;
    }
    /* SimpleSet_Add returns whatever object is present at self
     * or the new object if it needs to add it.
     */
    canonical_tuple = SimpleSet_Add(_interned_tuples, (PyObject *)self);
    if (!canonical_tuple) {
        // Some sort of exception, propogate it.
        return NULL;
    }
    if (canonical_tuple != (PyObject *)self) {
        // There was already a tuple with that value
        return (StaticTuple *)canonical_tuple;
    }
    self->flags |= STATIC_TUPLE_INTERNED_FLAG;
    // The two references in the dict do not count, so that the StaticTuple
    // object does not become immortal just because it was interned.
    Py_REFCNT(self) -= 1;
    return self;
}

But it also uses "Py_REFCNT(self) = 2;" to "revive dead object temporarily for Discard".

static void
StaticTuple_dealloc(StaticTuple *self)
{
    int i, len;

    if (_StaticTuple_is_interned(self)) {
        /* revive dead object temporarily for Discard */
        Py_REFCNT(self) = 2;
        if (SimpleSet_Discard(_interned_tuples, (PyObject*)self) != 1)
            Py_FatalError("deletion of interned StaticTuple failed");
        self->flags &= ~STATIC_TUPLE_INTERNED_FLAG;
    }
    len = self->size;
    for (i = 0; i < len; ++i) {
        Py_XDECREF(self->items[i]);
    }
    Py_TYPE(self)->tp_free((PyObject *)self);
}

It sounds like an optimization using a set of "interned" tuples. Maybe to reduce the memory footprint.

And breezy: https://bugzilla.redhat.com/show_bug.cgi?id=1890880 (not yet reported upstream)

I reported the issue to breezy upstream: https://bugs.launchpad.net/brz/+bug/1904868

I created the https://github.com/pythoncapi/upgrade_pythoncapi project which updates automatically a C extension to newer C API. For example, it uses Py_TYPE(), Py_SIZE(), Py_REFCNT(), Py_SET_SIZE(), Py_SET_SIZE() and Py_SET_REFCNT() functions.

Status:

Py_SET_REFCNT(), Py_SET_TYPE() and Py_SET_SIZE() functions added to Python 3.9.
Python and Cython have been modified to use Py_TYPE(), Py_SET_REFCNT(), Py_IS_TYPE(), etc.
pythoncapi_compat.h header file has been created to provide new functions to Python 3.6: https://github.com/pythoncapi/pythoncapi_compat
Script has been created to upgrade C extensions to add support for Python 3.10 without losing support for old Python versions: https://github.com/pythoncapi/pythoncapi_compat
PEP-620 "Hide implementation details from the C API" written
Py_TYPE() and Py_SIZE() were converted to a static inline function to deny "Py_TYPE(obj) = type;" syntax, but this change has been reverted during Python 3.10 development cycle since it broke too many C extension modules. (msg381337)

TODO:

Maybe add a new formatter for type names (msg361523)
Avoid sizeof(PyObject) in PyType_FromSpec() (msg366473)

The purpose of this issue is only to fix the API part. Replacing static inline functions with opaque function calls (stable ABI) is not in the scope of this issue.

immutables: https://github.com/MagicStack/immutables/issues/46

I proposed a fix: https://github.com/MagicStack/immutables/pull/52

mercurial https://bugzilla.redhat.com/show_bug.cgi?id=1897178

I proposed a fix: https://bz.mercurial-scm.org/show_bug.cgi?id=6451

pyside2 https://bugzilla.redhat.com/show_bug.cgi?id=1898974

I proposed a fix upstream: https://bugreports.qt.io/browse/PYSIDE-1436 I also wrote a fix in Fedora: https://src.fedoraproject.org/rpms/python-pyside2/pull-request/7 The issue is now tracked in Fedora as: https://bugzilla.redhat.com/show_bug.cgi?id=1902618

bitarray https://bugzilla.redhat.com/show_bug.cgi?id=1897536

I created https://github.com/ilanschnell/bitarray/pull/109

boost https://bugzilla.redhat.com/show_bug.cgi?id=1896382

I proposed https://github.com/boostorg/python/pull/330 fix.

See also https://github.com/boostorg/python/pull/329

Most projects broken by Py_TYPE and Py_SIZE changes have been fixed since last year. I succeeded to use my new pythoncapi_compat project on multiple C extensions. So I propose again to convert Py_TYPE and Py_SIZE macros to static inline functions: https://github.com/python/cpython/pull/26493

I also proposed to promote the pythoncapi_compat project in the "C API: Porting to Python 3.10" section of "What's New In Python 3.10?" on python-dev: https://mail.python.org/archives/list/python-dev@python.org/thread/KHDZGCNOYEDUTSPAATUDP55ZSSQM5RRC/

I already announced the pythoncapi_compat project on the capi-sig last December: https://mail.python.org/archives/list/capi-sig@python.org/thread/LFLXFMKMZ77UCDUFD5EQCONSAFFWJWOZ/

So I propose again to convert Py_TYPE and Py_SIZE macros to static inline functions

+1

New changeset f3fa63ec75fdbb4a08a10957a5c631bf0c4a5970 by Victor Stinner in branch 'main': bpo-39573: Py_TYPE becomes a static inline function (GH-26493) https://github.com/python/cpython/commit/f3fa63ec75fdbb4a08a10957a5c631bf0c4a5970

@victor, git bisect tells me the change f3fa63ec75fdbb4a08a10957a5c631bf0c4a5970 caused test_exceptions.ExceptionTests.test_recursion_in_except_handler to stack overflow only on windows debug builds. 3 windows buildbots using python debug mode is affected. Python compiled with release mode is *not* affected and passes the test. Here's an example error on one of the buildbots:

https://buildbot.python.org/all/#/builders/596/builds/354/steps/4/logs/stdio

I can also reproduce this locally. I tracked this issue down after a recursion in AST also caused a stack overflow, see my message here: https://bugs.python.org/msg395172

TLDR: Windows builds seems to set stack size to 2MB, on *nix it's probably higher (usually 8MB). I suspect the static inline functions are not being inlined in windows debug builds, so every function call adds to the stack. In that message I proposed to increase the stack size on windows but there are some concerns (see msg395177). What do you think?

MSVC by default disables method inlining (/Ob0) when '/Od' is specified on the command line while the optimization options specify '/Ob2'.

Ken Jin: Please open a separated issue for test_exceptions.test_recursion_in_except_handler(). It's not directly related to marking PyObject opaque, as William Pickard explained.

See my notes on the stack size and stack overflow on a recursion error on Windows: https://pythondev.readthedocs.io/unstable_tests.html#unlimited-recursion

New changeset 6d518bb3a11f9b16098f45b21a13ebe8f537f045 by Pablo Galindo in branch 'main': bpo-44348: Revert "bpo-39573: Py_TYPE becomes a static inline function (GH-26493)" (GH-26596) https://github.com/python/cpython/commit/6d518bb3a11f9b16098f45b21a13ebe8f537f045

See also bpo-44378: "Py_IS_TYPE(): cast discards ‘const’ qualifier from pointer target type".

If Py_TYPE() is converted again to a static inline function which takes a "const PyObject*" type, Py_IS_TYPE() can be modified again at the same time to use Py_TYPE().

New changeset cb15afcccffc6c42cbfb7456ce8db89cd2f77512 by Victor Stinner in branch 'main': bpo-39573: Py_TYPE becomes a static inline function (GH-28128) https://github.com/python/cpython/commit/cb15afcccffc6c42cbfb7456ce8db89cd2f77512

boost https://bugzilla.redhat.com/show_bug.cgi?id=1896382

Fixed by: https://github.com/boostorg/python/commit/500194edb7833d0627ce7a2595fec49d0aae2484

I checked again the list of broken projects listed previously.

Fixed:

Fix proposed:

pybluez: https://github.com/pybluez/pybluez/pull/410

Broken:

PyPAM
pygobject3
pylibacl
rdiff-backup

At commit cb15afcccffc6c42cbfb7456ce8db89cd2f77512, I am able to rename PyObject members (to make sure that the structure is not accessed directly), I only had to modify header files:

Py_REFCNT(), Py_SET_REFCNT()
Py_INCREF(), Py_DECREF()
Py_TYPE(), Py_SET_TYPE()
Py_IS_TYPE()

And just two more C files which corner cases:

1 line in Python/specialize.c
1 line in Modules/_testcapimodule.c: check_pyobject_forbidden_bytes_is_freed()

--

I did the same with PyVarObject, rename the ob_size member. I had to modify header files:

Py_SIZE(), Py_SET_SIZE()

But I had to modify the following function of the array module:

static int
array_buffer_getbuf(arrayobject *self, Py_buffer *view, int flags)
{
    ...
    if ((flags & PyBUF_ND)==PyBUF_ND) {
        view->shape = &((PyVarObject*)self)->ob_size;
    }
    ...
    return 0;
}

I'm not sure how to patch this function.

--

This experience doesn't check usage of sizeof(PyObject) and sizeof(PyVarObject) which would break if these structures become opaque. sizeof() issues are listed in previous comments.

Oh and obviously, it's not possible possible to define structures which *include* PyObject or PyVarObject if PyObject and PyVarObject become opaque. Example:

typedef struct {
    PyObject ob_base;
    Py_ssize_t ob_size; /* Number of items in variable part */
} PyVarObject;

This C code requires the PyObject structure to be fully defined (not being opaque).

A new C API and ABI where structures *don't include PyObject or PyVarObject should be designed to allocate their members "before" the PyObject pointer value. Something like the current PyGC_Head structure which is excluded from PyObject and stored *before the "PyObject" pointer.

Simplified code which allocates memory for an object implementin the GC protocol:

static PyObject *
_PyObject_GC_Malloc(size_t basicsize)
{
    ...
    size_t size = sizeof(PyGC_Head) + basicsize;
    ...
    PyGC_Head *g = (PyGC_Head *)PyObject_Malloc(size);
    ...
    PyObject *op = (PyObject *)(g + 1);
    return op;
}

I changed the issue title to restrict its scope: "[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type".

Making PyObject and PyVarObject structures opaque is a broader topic which should be splited into sub-issues.

"Py_TYPE(obj)=type;" is now disallowed. I consider that the work of this issue is now completed and I close the issue.

Thanks everyone who help to fix these tedious issues!

You can continue to use this issue if you need my help to adapt your C extensions to Py_SET_TYPE()/Py_SET_SIZE().

See also the upgrade_pythoncapi.py script of the pythoncapi_compat project which helps to port your C extensions without losing support for old Python versions: https://github.com/pythoncapi/pythoncapi_compat

See also the Py_TYPE() change announcement on the capi-sig list: https://mail.python.org/archives/list/capi-sig@python.org/thread/WGRLTHTHC32DQTACPPX36TPR2GLJAFRB/

I wrote an article about these changes: https://vstinner.github.io/c-api-abstract-pyobject.html

It elaborates the rationale for making these changes.

@victor, git bisect tells me the change f3fa63ec75fdbb4a08a10957a5c631bf0c4a5970 caused test_exceptions.ExceptionTests.test_recursion_in_except_handler to stack overflow only on windows debug builds.

FYI this regression was handled last year in bpo-44348 "test_exceptions.ExceptionTests.test_recursion_in_except_handler stack overflow on Windows debug builds" and fixed at 2021-09-07 by using the trashcan mecanism in the BaseException deallocator function:

New changeset fb305092a5d7894b41f122c1a1117b3abf4c567e by Victor Stinner in branch 'main': bpo-44348: BaseException deallocator uses trashcan (GH-28190) https://github.com/python/cpython/commit/fb305092a5d7894b41f122c1a1117b3abf4c567e

python / cpython

[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type #83754

Linked PRs