Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
colesbury opened this issue May 10, 2024 · 2 comments
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented May 10, 2024

Bug report

In #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

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.

cc @erlend-aasland

@colesbury colesbury added type-bug An unexpected behavior, bug, or error 3.13 bugs and security fixes topic-free-threading 3.14 new features, bugs and security fixes labels May 10, 2024
@colesbury
Copy link
Contributor Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants