Disallow creation of incomplete/inconsistent objects

[vstinner]

The C API has many ways to create incomplete and/or inconsistent Python objects. We should disallow that.

One big example is that PyTuple_New() API which creates an uninitialized Python tuple object and immediately tracks it in the Garbage Collector. This issue was discussed from 2012 to 2021. At the end, because this API is too widely used, it was decided to not fix this issue: python/cpython#59313 was closed as "not a bug" (!).

Python objects must only be tracked by the GC once they are fully initialized. At least, when calling their "traverse" function will not crash. I added _PyType_AllocNoTrack() function and used it in a few types to fix this issue in modified types: delay PyObject_GC_Track() call only after the object is initialized.

PyUnicode_New() is another example: it creates a Python str object with uninitialized characters.

These API are written for performance: create a "scratch" object, populate it, and then expose it in Python. Since data is written directly into the object, there is no memory copy or conversion. The problem is that Python introspection gives access to these incomplete/inconsistent objects. It's common that exploring gc.get_objects() lead to crashes. In 2016, an optimization for function call in property_descr_get() leaded to crash: issue #70998. There were a bunch of similar bugs, so it motivated me to write the FASTCALL calling convention to avoids the need to create a tuple object to call a C function: issue #71001.

Over the years, I also added many CheckConsistency() functions, used in assertions, to make sure that objects are consistent. Examples: _PyObject_CheckConsistency(), _PyUnicode_CheckConsistency(), _PyType_CheckConsistency(), _PyDict_CheckConsistency().

---

For example, unsafe PyTuple_New() can be replaced with PyTuple_Pack(): the created tuple is only tracked by the GC once it's fully initialized and consistent. PyTuple_Pack() is a good example to follow.

[davidhewitt]

I just took a look at replacing PyTuple_New() with PyTuple_Pack() in PyO3. One case where this doesn't seem to be possible is when the length is not known until runtime. If we wanted to avoid PyTuple_New() in these cases, I think we'd need to add a PyTuple_AllocNoTrack() function equivalent to the _PyType_AllocNoTrack() you describe above?

[vstinner]

If the tuple is short, you can use an array of strong references and allocate it on the heap. Problem: there is no public API to create a tuple from an array and... Steal references (it's bad, boooh!).

[cavokz]

[OT] At Pygolo (a bright new Python-Go API) I'm also thinking of dropping PyTuple_New and PyTuple_SetItem, I guess there is no use case for PyTuple_SetItem besides initializing a tuple created with PyTuple_New, right?

[davidhewitt]

If the tuple is short, you can use an array of strong references and allocate it on the heap.

Would you please share a code example of this approach?

Problem: there is no public API to create a tuple from an array and... Steal references (it's bad, boooh!).

Yes I noticed this when comparing PyTuple_New to PyTuple_Pack that I'm paying for an additional incref / decref cycle. Do you think this problem is widespread enough that people would be willing for PyTuple_PackOwned? (Or PyTuple_PackGive or whatever other naming convention comes out of #11.)

[vstinner]

Py_BuildValue("NNN", ref1, ref2, ref3) can be used to build a tuple of 3 items and steal references.

[vstinner]

PyTuple_SetItem() should not exist: it modify an... immutable object! The problem is that Python introspection make obejcts visible while being initialized, whereas most developers are not aware of that.

The Python str implementation uses heuristics to check if maybe an object was already accessed (visible in Python): check the reference count, check if the hash was computed, etc. Python immutable str objects can also be modified :-(

Maybe an approach for that would be to provide an API to build an object in an efficient way. There is for example the _PyUnicodeWriter API to produce a Python str object. We could have a similar C API to build a Python of an unknown size.

[vstinner]

See also python/cpython#84323 "Modify _PyObject_GC_TRACK() to ensure that newly tracked object is valid".

Extract:

PyType_GenericAlloc() allocates memory for a type allocated on the heap... and then immediately track it in the GC. Problem: this type is not initialized yet, all fields are set to zero. Calling type_traverse() at this point fails with an assertion error: (...)

[erlend-aasland]

PyTuple_SetItem() should not exist: it modify an... immutable object! The problem is that Python introspection make obejcts visible while being initialized, whereas most developers are not aware of that.

FTR, Sergey Fedoseev added an internal API for safe and fast tuple creation some years ago: python/cpython#80211

[vstinner]

FTR, Sergey Fedoseev added an internal API for safe and fast tuple creation some years ago: python/cpython#80211

From the issue, it's non-obvious to identify which function you're talking about. So it's the new _PyTuple_FromArray() function which copies objects by creating new strong references, and only track the tuple once all items are fully initialized. The code is quite short and fit here:

PyObject *
_PyTuple_FromArray(PyObject *const *src, Py_ssize_t n)
{
    if (n == 0) {
        return tuple_get_empty();
    }

    PyTupleObject *tuple = tuple_alloc(n);
    if (tuple == NULL) {
        return NULL; 
    }
    PyObject **dst = tuple->ob_item;
    for (Py_ssize_t i = 0; i < n; i++) {
        PyObject *item = src[i];
        dst[i] = Py_NewRef(item);
    }
    _PyObject_GC_TRACK(tuple);  // <==== the nice part
    return (PyObject *)tuple;
}

[vstinner]

I proposed a new internal C API to build a tuple: python/cpython#107139

[vstinner]

I guess there is no use case for PyTuple_SetItem besides initializing a tuple created with PyTuple_New, right?

Correct. PyTuple_SetItem(), PyTuple_SET_ITEM() and _PyTuple_Resize() are only needed for tuples created by PyTuple_New(). These tuples should not be tracked by the GC, but currently PyTuple_New() tracks the tuple by the GC as soon as the tuple is created.

[vstinner]

Examples of crashes related to PyTuple_New():

• 2003: tuple(generator) + gc.get_objects(): gc.get_referrers() is inherently dangerous python/cpython#39117
• 2016: property_descr_get() uses an internal tuple to optimize PyObject_Call(), tuple is tracked but sometimes invalid: segfault due to null pointer in tuple python/cpython#70998 -- see also issue #68464 (related to issue #58581)

Before I wrote the METH_FASTCALL calling convention, it was common to create an internal cached tuple to call PyObject_Call(), since the cost of creating/destroying the tuple can be significant in fast functions (around 100 ns). Sometimes, this tuple was tracked by the GC but was not always valid. property_descr_get() is an example of such issue.

Other crashes related to GC introspection features:

• 2021: There is a way to access an underlying mapping in MappingProxyType python/cpython#88004

[scoder]

So, do I understand correctly that the only problem with PyTuple_New() is that it immediately tracks the object in GC, rather than leaving it to the user to do that after populating the tuple? That could be solved with a PyTuple_FinishNew() function that enables the tracking, right? Similar for PyUnicode etc.

I actually like the fact that CPython allows filling data directly into the object containers.

[encukou]

That could be solved with a PyTuple_FinishNew() function that enables the tracking, right?

Well, with that you also need a PyTuple_NewUnfinished that allocates a tuple but doesn't enable tracking yet. Here's my draft of what this line of thinking leads to: capi-workgroup/api-evolution#20 (comment)
That draft also allows filling tuples in implementations where they aren't actually backed by a PyObject* array -- in a way that adds no cost on CPython.

[encukou]

Proposed guideline issue: capi-workgroup/api-evolution#36