Issue42961
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.
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) * ![]() |
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:40 | admin | set | github: 87127 |
| 2021-10-21 18:09:22 | vstinner | set | nosy:
+ vstinner pull_requests: + pull_request27403 |
| 2021-10-21 17:59:20 | petr.viktorin | set | status: open -> closed nosy: + petr.viktorin messages: + msg404629 resolution: fixed stage: resolved |
| 2021-01-19 13:27:19 | bstaletic | set | messages: + msg385259 |
| 2021-01-18 21:23:22 | YannickJadoul | set | nosy:
+ YannickJadoul |
| 2021-01-18 21:16:00 | bstaletic | create | |
