search

python / cpython

The Python programming language
https://www.python.org/
Other
61.4k stars 29.57k forks source link

`tp_doc` switch from `PyObject_Malloc` to `PyMem_Malloc` is not backwards compatible #118909

Open colesbury opened 2 months ago

colesbury commented 2 months ago

Bug report

In https://github.com/python/cpython/pull/114574 we switched a number of non-PyObject allocations from PyObject_Malloc to PyMem_Malloc, including tp_doc on PyHeapTypeObjects.

Unfortunately, this isn't backwards compatible because C-API extensions may allocate tp_doc contents, which are then freed by CPython in type_dealloc. For example, pybind11 allocates memory for the docstring using PyObject_MALLOC. This leads to crashes when using pybind11 in debug builds of Python 3.13: the allocation uses PyObject_MALLOC, but the memory is freed using PyMem_Free.

We should consider reverting the change to tp_doc and figure out a way to allocate the doc in a way that's both safe (in the free-threaded build) and doesn't break backwards compatibility (in the default build).

Some example extensions: #### Uses `PyObject_Malloc` * [pybind11](https://github.com/pybind/pybind11/blob/750257797ca5eda6f1561f6c3f7147bcdd92cf52/include/pybind11/detail/class.h#L663-L665) * [nanobind](https://github.com/wjakob/nanobind/blob/c5454462e35f29310df05b412b5c48997d634bdd/src/nb_type.cpp#L700-L706) * [datatable](https://github.com/h2oai/datatable/blob/b1a87103c55433dc2069df28a460f3ad15450025/src/core/python/xobject.cc#L140-L142) #### Uses `strdup` We don't document the `tp_doc` behavior so some extensions use `strdup`, which works fine in release builds (and is thread-safe in the free-threaded build), but probably crashes in debug builds of CPython. * [pyicu](https://github.com/cenkalti/pyicu/blob/675017d10916e14443c10d8c7125671c11c77b57/_icu.cpp#L228) * [pyavroc](https://github.com/tubular/pyavroc/blob/a646a4c1b64c2aa53791f0769e151013b383890a/src/record.c#L261) * [pyclops](https://github.com/kmsmith137/pyclops/blob/00e2325c25aa7250792b3b9ead867293e13cddbd/pyclops/extension_type.hpp#L293)

cc @erlend-aasland

colesbury commented 2 months ago

Some possible ways to address this:

  1. Require C API extensions to use PyMem_Malloc for tp_doc on heap types going forward. Not ideal because it's a non-backwards compatible change.

  2. Revert and require C API extensions to use PyObject_Malloc for tp_doc on heap types. Not ideal because it's not thread-safe in the free-threaded build.

  3. Recommend using PyMem_Malloc, but allow extensions to use either PyObject_Malloc or PyMem_Malloc by adding special logic when we free tp_doc internally. It might not be possible to fully detect which allocation method was used if the allocator was overridden, but we can handle the common cases and fall back to PyObject_Free for backwards compatibility.

  4. Add a new API, e.g., PyMem_DocMalloc/PyMem_DocFree that we recommend. In the default build, it can be implemented as calls to PyObject_Malloc/Free for backwards compatibility. In the free-threaded build, we can do extra work at runtime to make the calls thread-safe.

My inclination would be (3).

erlend-aasland commented 2 months ago

I would prefer 1) but 3) would probably be nicer. Definitely not 2) nor 4). If we go for 3), it should be a temporary workaround coupled with docs that communicate which allocator to use, and that the fallback/workaround will be removed in a future Python version. IMO.