This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Use-after-free (of a heap type) during finalization
Type: behavior Stage: resolved
Components: C API Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: YannickJadoul, bstaletic, petr.viktorin, vstinner
Priority: normal Keywords:

Created on 2021-01-18 21:16 by bstaletic, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
heap_type_base_use_after_free.cpp bstaletic, 2021-01-18 21:15
Pull Requests
URL Status Linked Edit
PR 26274 vstinner, 2021-10-21 18:09
Messages (3)
msg385230 - (view) Author: Boris Staletic (bstaletic) Date: 2021-01-18 21:15
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:

https://github.com/pybind/pybind11/pull/2797

[1]: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L1362
[2]: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L1368
msg385259 - (view) Author: Boris Staletic (bstaletic) Date: 2021-01-19 13:27
Oops... I uploaded (and pasted) the wrong file. The /correct/ example can be found here:

https://github.com/pybind/pybind11/pull/2797/#pullrequestreview-570541151

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();
}
msg404629 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-21 17:59
as far as I can see, this had the same root cause as bpo-44184, and was fixed in GH-26274.


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 :)
History
Date User Action Args
2022-04-11 14:59:40adminsetgithub: 87127
2021-10-21 18:09:22vstinnersetnosy: + vstinner

pull_requests: + pull_request27403
2021-10-21 17:59:20petr.viktorinsetstatus: open -> closed

nosy: + petr.viktorin
messages: + msg404629

resolution: fixed
stage: resolved
2021-01-19 13:27:19bstaleticsetmessages: + msg385259
2021-01-18 21:23:22YannickJadoulsetnosy: + YannickJadoul
2021-01-18 21:16:00bstaleticcreate