`_PyList_AsTupleAndClear` does not reset `allocated` to 0.

[KowalskiThomas]

Description

While working on our C extension, I noticed what I believe is a bug/crash in the list implementation when using _PyList_AsTupleAndClear (which creates a list from a tuple then clears the list), sort of live std::move-ing a Python list into a tuple.

Long story short, the function sets self->ob_item = NULL and Py_SET_SIZE(self, 0) ("clear the list and disown the memory") but does NOT reset self->allocated to 0.

After this, the list "believes" it has some memory allocated but it actually has none.

If code then tries to append something to the list (through PyList_Append),
it fails because it directly writes to self->ob_item[len] -- which is null.

Currently, it seems like there's only one caller to _PyList_AsTupleAndClear, and it discards the list right after gettings its tuple, so the bug has been flying under the radar I believe...

Testing

Reproducer

#ifndef Py_BUILD_CORE_MODULE
#  define Py_BUILD_CORE_MODULE
#endif
#include "Python.h"
#include "pycore_list.h"

static PyObject*
test(PyObject* module, PyObject* ignored)
{
    PyObject* list = PyList_New(0);
    if (list == NULL) return NULL;

    /* Add items to get allocated > 0 */
    for (int i = 0; i < 10; i++) {
        PyObject* val = PyLong_FromLong(i);
        if (val == NULL) { Py_DECREF(list); return NULL; }
        if (PyList_Append(list, val) < 0) {
            Py_DECREF(val); Py_DECREF(list); return NULL;
        }
        Py_DECREF(val);
    }

    PyListObject* lst = (PyListObject* )list;

    PyObject* tup = _PyList_AsTupleAndClear(lst);
    if (tup == NULL) { Py_DECREF(list); return NULL; }

    if (lst->ob_item == NULL && lst->allocated != 0) {
        printf("Attempting append...\n");

        PyObject *newitem = PyLong_FromLong(123123123);
        if (newitem == NULL) {
            Py_DECREF(tup); Py_DECREF(list); return NULL;
        }

        if (PyList_Append((PyObject*)lst, newitem) < 0) {
            Py_DECREF(newitem);
            Py_DECREF(tup); Py_DECREF(list); return NULL;
        }
        Py_DECREF(newitem);
    }

    Py_DECREF(tup);
    Py_DECREF(list);
    Py_RETURN_NONE;
}

static PyMethodDef methods[] = {
    {"test", test, METH_NOARGS, ""},
    {NULL, NULL, 0, NULL}
};

static struct PyModuleDef module = {
    PyModuleDef_HEAD_INIT,
    "ext",
    "ext",
    -1,
    methods
};

PyMODINIT_FUNC
PyInit_ext(void)
{
    return PyModule_Create(&module);
}

from setuptools import setup, Extension
import os

cpython_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))

ext = Extension(
    "ext",
    sources=["ext.c"],
    include_dirs=[
        os.path.join(cpython_root, "Include"),
        os.path.join(cpython_root, "Include", "internal"),
        cpython_root,
    ],
    define_macros=[("Py_BUILD_CORE_MODULE", "1")],
)

setup(
    name="ext",
    ext_modules=[ext],
)

To build the reproducer (from a subdir):

../python.exe setup.py build_ext --inplace

And to trigger it:

../python.exe -c "import ext; ext.test()"

gives (I have a TSan build but it does give me what I expect...)

<frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'ext', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.
Attempting append...
ThreadSanitizer:DEADLYSIGNAL
==35116==ERROR: ThreadSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000101639e50 bp 0x00016f72d570 sp 0x00016f72d530 T25592900)
==35116==The signal is caused by a WRITE memory access.
==35116==Hint: address points to the zero page.
^Z
zsh: suspended  ../python.exe -c "import ext; ext.test()"

Next steps

I have a branch/PR ready with a fix for that: #145680.

Linked PRs

• gh-145681: reset list allocated to 0 #145680

[encukou]

_PyList_AsTupleAndClear is only used internally in CPython, so it's OK for it to only do enough to convince list_dealloc do the right thing.

That said, a bit of defensive programming shouldn't hurt.
I don't think we need to add an adjustment for self->allocated, though. We can instead skip freeing the memory & setting self->ob_item to NULL, and leave deallocation to list_dealloc.

[KowalskiThomas]

I don't think we need to add an adjustment for self->allocated, though. We can instead skip freeing the memory & setting self->ob_item to NULL, and leave deallocation to list_dealloc.

Ah, yeah, that makes sense to me as well.

EDIT: Now that I think about it, won't that be a problem (when we deallocate the list) since the items in the list will still "be" in ob_item but now belong to the resulting tuple?

[encukou]

The bits of the pointers will “be” in ob_item, but they'll be treated as uninitialized memory, reserved so that the next few appends don't need to reallocate the array.
The deallocator should not look at them, much less DECREF them.

[KowalskiThomas]

Ah, I think I get it now. Sorry for the confusion -- what you mean is to remove setting ob_item to NULL, remove allocated = 0 but keep the Py_SET_SIZE so that the list deallocator considers everything inside ob_item as uninitialised memory / not something it needs to clear. Correct? (Sorry for the back and forth.)

[KowalskiThomas]

I updated #145680 to do what you suggested.