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 |