Use-after-free (of a heap type) during finalization

[bstaletic]

BPO	42961
Nosy	@vstinner, @encukou, @YannickJadoul, @bstaletic
PRs	bpo-44184: Fix subtype_dealloc() for freed type #26274
Files	heap_type_base_use_after_free.cpp

^{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:

assignee = None
closed_at = <Date 2021-10-21.17:59:20.826>
created_at = <Date 2021-01-18.21:16:00.169>
labels = ['expert-C-API', 'type-bug', '3.9', '3.10']
title = 'Use-after-free (of a heap type) during finalization'
updated_at = <Date 2021-10-21.18:09:22.120>
user = 'https://github.com/bstaletic'

bugs.python.org fields:

activity = <Date 2021-10-21.18:09:22.120>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-10-21.17:59:20.826>
closer = 'petr.viktorin'
components = ['C API']
creation = <Date 2021-01-18.21:16:00.169>
creator = 'bstaletic'
dependencies = []
files = ['49749']
hgrepos = []
issue_num = 42961
keywords = []
message_count = 3.0
messages = ['385230', '385259', '404629']
nosy_count = 4.0
nosy_names = ['vstinner', 'petr.viktorin', 'YannickJadoul', 'bstaletic']
pr_nums = ['26274']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42961'
versions = ['Python 3.9', 'Python 3.10']

[bstaletic]

When a C extension module exposes a heap-type that is used as a base class for a "pure" python class, that gets deallocated during Py_Finalize(), subtype_dealloc() runs into a use-after-free error.

That is a hell of a run-on sentence. The thing is that every part of the sentence needs to happen, to trigger the error, at least according to valgrind. The whole thing took a very long time to debug and I finally have a concise and self-contained repro:

///////////////////////////////////////////////////////////
#include <Python.h>

static void pybind11_object_dealloc(PyObject *self) {
        auto type = Py_TYPE(self);
        type->tp_free(self);
        Py_DECREF(type);
}

static PyModuleDef pybind11_module_def_m{
.m_base = PyModuleDef_HEAD_INIT,
.m_name = "m",
.m_size = -1,
};

static PyObject *pybind11_init_impl_m() {
        auto m = PyModule_Create(&pybind11_module_def_m);
        PyType_Slot slots[] = {
                {Py_tp_dealloc, (void*)pybind11_object_dealloc},
                {0, nullptr}
        };
        PyType_Spec spec{"m.B", sizeof(PyObject), 0, Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HEAPTYPE, slots};
        auto type = PyType_FromSpec(&spec);
        PyObject_SetAttrString(m, "B", (PyObject*)type);
        return m;
}

int main() {
        PyImport_AppendInittab("m", pybind11_init_impl_m);
        Py_InitializeEx(1);
        PyRun_SimpleString("import m\n"
                           "def t():\n"
                           "  class A:\n"
                           "    class B(m.B):pass\n"
                           "    B()\n"
                           "t()\n");
        Py_Finalize();
}
/////////////////////////////////////////////////////////////////////////

Below is the valgrind-produced traceback of the error:

==3768== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info

==3768== Invalid read of size 1 [4/36]
==3768== at 0x4952AD6: subtype_dealloc (typeobject.c:1368)
==3768== by 0x4928B6C: _Py_DECREF (object.h:448)
==3768== by 0x4928B6C: _Py_XDECREF (object.h:514)
==3768== by 0x4928B6C: free_keys_object (dictobject.c:628)
==3768== by 0x492DAC8: dict_tp_clear (dictobject.c:3289)
==3768== by 0x4A365EF: delete_garbage (gcmodule.c:1018)
==3768== by 0x4A365EF: gc_collect_main (gcmodule.c:1301)
==3768== by 0x4A370A4: gc_collect_with_callback (gcmodule.c:1414)
==3768== by 0x4A370A4: PyGC_Collect (gcmodule.c:2066)
==3768== by 0x4A106A6: Py_FinalizeEx (pylifecycle.c:1716)
==3768== by 0x4A106A6: Py_FinalizeEx (pylifecycle.c:1638)
==3768== by 0x109337: main (newtype.cpp:36)
==3768== Address 0x59cc609 is 185 bytes inside a block of size 944 free'd
==3768== at 0x483B9AB: free (vg_replace_malloc.c:538)
==3768== by 0x4951830: type_dealloc (typeobject.c:3593)
==3768== by 0x1091F3: _Py_DECREF (object.h:448)
==3768== by 0x10922E: pybind11_object_dealloc(object*) (newtype.cpp:6)
==3768== by 0x4952AD5: subtype_dealloc (typeobject.c:1362)
==3768== by 0x4928B6C: _Py_DECREF (object.h:448)
==3768== by 0x4928B6C: _Py_XDECREF (object.h:514)
==3768== by 0x4928B6C: free_keys_object (dictobject.c:628)
==3768== by 0x492DAC8: dict_tp_clear (dictobject.c:3289)
==3768== by 0x4A365EF: delete_garbage (gcmodule.c:1018)
==3768== by 0x4A365EF: gc_collect_main (gcmodule.c:1301)
==3768== by 0x4A370A4: gc_collect_with_callback (gcmodule.c:1414)
==3768== by 0x4A370A4: PyGC_Collect (gcmodule.c:2066)
==3768== by 0x4A106A6: Py_FinalizeEx (pylifecycle.c:1716)
==3768== by 0x4A106A6: Py_FinalizeEx (pylifecycle.c:1638)
==3768== by 0x109337: main (newtype.cpp:36)
==3768== Block was alloc'd at
==3768== at 0x483A77F: malloc (vg_replace_malloc.c:307)
==3768== by 0x4A37402: _PyObject_GC_Alloc (gcmodule.c:2217)
==3768== by 0x4A37402: _PyObject_GC_Malloc (gcmodule.c:2244)
==3768== by 0x49523B5: PyType_GenericAlloc (typeobject.c:1072)
==3768== by 0x495F2B4: type_new (typeobject.c:2622)
==3768== by 0x49550F5: type_call (typeobject.c:1039)
==3768== by 0x48F0EC7: _PyObject_MakeTpCall (call.c:191)
==3768== by 0x49CD6CF: builtin___build_class_ (bltinmodule.c:232)
==3768== by 0x493B674: cfunction_vectorcall_FASTCALL_KEYWORDS (methodobject.c:442)
==3768== by 0x48B0040: _PyObject_VectorcallTstate (abstract.h:114)
==3768== by 0x48B0040: PyObject_Vectorcall (abstract.h:123)
==3768== by 0x48B0040: call_function (ceval.c:5379)
==3768== by 0x48B0040: _PyEval_EvalFrameDefault (ceval.c:3803)
==3768== by 0x49D383A: _PyEval_EvalFrame (pycore_ceval.h:40)
==3768== by 0x49D383A: _PyEval_EvalCode (ceval.c:4625)
==3768== by 0x49D3B9D: _PyEval_EvalCodeWithName (ceval.c:4657)
==3768== by 0x49D3BED: PyEval_EvalCodeEx (ceval.c:4673)

FWIW, the valgrind command doesn't need any extra flags/switches. Looking at the output, it seems to me that the following happens:

1. 1 Objects/typeobject.c#L1362 calls the user provided (see the attached file) tp_dealloc(p).
2. 2 Immediately after, Objects/typeobject.c#L1368 dereferences the, now dangling, pointer.

The provided tp_dealloc looks like this:

static void pybind11_object_dealloc(PyObject *self) {
        auto type = Py_TYPE(self);
        type->tp_free(self);
        Py_DECREF(type);
}

Which seems correct to me, given the "whatsnew" page for python 3.8:

https://docs.python.org/3.8/whatsnew/3.8.html#changes-in-the-c-api

Finally, this found while running an innocent-looking pybind11 test inside valgrind:

pybind/pybind11#2797

[bstaletic]

Oops... I uploaded (and pasted) the wrong file. The /correct/ example can be found here:

pybind/pybind11#2797

However, I have just realized that the example doesn't really need the embedded module. The following also shows the use-after-free:

#include <Python.h>

static void pybind11_object_dealloc(PyObject *self) {
	auto type = Py_TYPE(self);
	type->tp_free(self);
	Py_DECREF(type);
}
static PyType_Slot base_slots[] = {{Py_tp_dealloc, (void*)pybind11_object_dealloc}, {0, nullptr}};
static PyType_Spec base_spec{"B", sizeof(PyObject), 0, Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HEAPTYPE, base_slots};
int main() {
	Py_InitializeEx(1);
	auto base_type = PyType_FromSpec(&base_spec);
	auto globals = PyDict_New();
	PyDict_SetItemString(globals, "B", base_type);
	auto derived_t = PyRun_String("def f():\n"
                                      "  class C:\n"
                                      "    class D(B):pass\n"
                                      "    b=D()\n"
                                      "f()", Py_file_input, globals, nullptr);
	Py_DECREF(globals);
	Py_DECREF(derived_t);
	Py_Finalize();
}

[encukou]

as far as I can see, this had the same root cause as bpo-44184, and was fixed in #70462.

FWIW, the reproducers are missing a Py_DECREF(type) after PyDict_SetItemString/PyObject_SetAttrString; those don't steal the reference. That sent me on an unproductive refcounting chase :)