gh-118909: Fix crash freeing tp_doc allocated with PyObject_Malloc() - #156990
gh-118909: Fix crash freeing tp_doc allocated with PyObject_Malloc()#156990NAVEENKUMARKR777 wants to merge 1 commit into
Conversation
…loc() type_dealloc() always freed a heap type's tp_doc with PyMem_Free(), but some C extensions (e.g. older pybind11 and nanobind versions) allocate it directly with PyObject_Malloc() instead, relying on CPython to free it. The two allocator domains share the same underlying allocator in a release build, so this went unnoticed, but a build with debug allocator hooks enabled (Py_DEBUG, or PYTHONMALLOC=debug) tags each domain's blocks and aborts when a block is freed with the mismatched domain. Detect which allocator was actually used, from the tag debug builds write before the returned pointer, and free with the matching function as a backwards-compatibility fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documentation build overview
|
ZeroIntensity
left a comment
There was a problem hiding this comment.
This fix only applies to debug builds, which isn't very useful. The cases that we care about (i.e., in third-party extensions) use release builds.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@ZeroIntensity Thanks for taking a look. I want to make sure I understand the concern correctly before changing anything, since I think release builds are actually fine here today — happy to be corrected. I went through every allocator backend CPython ships to check whether
So in every release-mode configuration I can find, Given that, I want to make sure I'm addressing the right concern:
Let me know which direction you'd like, or if there's a concrete release-build crash scenario I'm missing — happy to dig further. |
|
@NAVEENKUMARKR777 Avoid poor LLM replies. We don't want to converse with an agent. |
|
Yes, please use your own words!
I think this needs more discussion before diving into fixes. |
Summary
Fixes gh-118909.
type_dealloc()always frees a heap type'stp_docwithPyMem_Free(), but some C extensions (older versions of pybind11, nanobind, datatable) allocate it directly withPyObject_Malloc()instead, relying on CPython to free it. In a release build both domains share the same underlying allocator, so this goes unnoticed, but a build with debug allocator hooks enabled (Py_DEBUG, orPYTHONMALLOC=debug) tags each domain's blocks and aborts the process when a block is freed with the wrong domain (Fatal Python error: _PyMem_DebugRawFree: bad ID: Allocated using API 'o', verified using API 'm').This implements the approach discussed by @colesbury and @erlend-aasland on the issue ("option 3"): detect which allocator was actually used, from the one-byte domain tag the debug allocator writes just before the returned pointer, and free with the matching function as a documented, backwards-compatibility fallback (not a stable guarantee).
Changes
Objects/typeobject.c: addtype_free_tp_doc(), used intype_dealloc().Modules/_testcapi/heaptype.c+Lib/test/test_capi/test_mem.py: regression test that builds a heap type with aPyObject_Malloc()-allocatedtp_docand deallocates it under the debug allocator.Doc/c-api/typeobj.rst: document thetp_docallocation contract and the fallback.Misc/NEWS.d: changelog entry.Test plan
test_pyobject_malloc_tp_doc) reproduces the exact reported crash when run against the pre-fix code, and passes with the fix, undermalloc_debug,pymalloc_debug, andmimalloc_debug.test_capi,test_types,test_descr(1,820 tests) pass with no regressions.🤖 Generated with Claude Code