Skip to content

Commit 524d72b

Browse files
authored
fix: strdup "self" arg in def_property_static, partially revert #6010 (gh-5976) (#6015)
* fix: strdup args added after initialize_generic in def_property_static (gh-5976) `def_property_static` calls `process_attributes::init` on already-initialized function records (after `initialize_generic`'s strdup loop has run). Args added at this stage (e.g. "self" via `append_self_arg_if_needed`) remain as string literals, so `destruct()` would call `free()` on them. Fix by strdup'ing name/descr of any args appended by the late `process_attributes::init` call. Root cause introduced by gh-5486. Made-with: Cursor * Partially revert gh-6010: remove py_is_finalizing() workarounds Now that the root cause (free of string literals in def_property_static, gh-5976) is fixed in the previous commit, the py_is_finalizing() guards introduced in gh-6010 are no longer needed: - tp_dealloc_impl: remove early return during finalization (was leaking all function records instead of properly destroying them) - destruct(): remove guard around arg.value.dec_ref() - common.h: remove py_is_finalizing() helper (no remaining callers) The genuine fix from gh-6010 (PyObject_Free + Py_DECREF ordering in tp_dealloc_impl) is retained. Made-with: Cursor * test: add embedding test for py::enum_ across interpreter restart (gh-5976) py::enum_ is the primary trigger for gh-5976 because its constructor creates properties via def_property_static / def_property_readonly_static, which call process_attributes::init on already-initialized function records. Yet none of the existing embedding tests used py::enum_ at all. Add an PYBIND11_EMBEDDED_MODULE with py::enum_ and a test case that imports it, finalize/reinitializes the interpreter, and re-imports it. This exercises the def_property_static code path that was fixed in the preceding commit. Note: on Python 3.14.2 (and likely 3.12+), tp_dealloc_impl is not called during Py_FinalizeEx for function record PyObjects — they simply leak because types are effectively immortalized. As a result, this test cannot trigger the original free()-on-string-literal crash on this Python version. However, it remains valuable as a regression guard: on Python builds where finalization does clean up function records (or if CPython changes this behavior), the test would catch the crash. It also verifies that py::enum_ survives interpreter restart correctly, which was previously untested. Made-with: Cursor * test: skip enum restart test on Python 3.12 (pre-existing crash) Made-with: Cursor * Add test_standalone_enum_module.py, standalone_enum_module.cpp * Make standalone_enum_module.cpp more similar to #5976 reproducer. Also fix clang-tidy error. * This crashes when testing locally: ( cd /wrk/forked/pybind11/tests && PYTHONPATH=/wrk/bld/pybind11_gcc_v3.14.2_df793163d58_default/lib /wrk/bld/pybind11_gcc_v3.14.2_df793163d58_default/TestVenv/bin/python3 -m pytest test_standalone_enum_module.py ) ============================= test session starts ============================== platform linux -- Python 3.14.2, pytest-9.0.2, pluggy-1.6.0 installed packages of interest: build==1.4.2 numpy==2.4.3 scipy==1.17.1 C++ Info: 13.3.0 C++20 __pybind11_internals_v12_system_libstdcpp_gxx_abi_1xxx_use_cxx11_abi_1__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False rootdir: /wrk/forked/pybind11/tests configfile: pytest.ini plugins: timeout-2.4.0, xdist-3.8.0 collected 1 item test_standalone_enum_module.py F [100%] =================================== FAILURES =================================== ________________________ test_enum_import_exit_no_crash ________________________ def test_enum_import_exit_no_crash(): # Modeled after reproducer under issue #5976 > env.check_script_success_in_subprocess( f""" import sys sys.path.insert(0, {os.path.dirname(env.__file__)!r}) import standalone_enum_module as m assert m.SomeEnum.__class__.__name__ == "pybind11_type" """, rerun=1, ) test_standalone_enum_module.py:10: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ code = 'import sys\nsys.path.insert(0, \'/wrk/forked/pybind11/tests\')\nimport standalone_enum_module as m\nassert m.SomeEnum.__class__.__name__ == "pybind11_type"' def check_script_success_in_subprocess(code: str, *, rerun: int = 8) -> None: """Runs the given code in a subprocess.""" import os import subprocess import sys import textwrap if ANDROID or IOS or sys.platform.startswith("emscripten"): pytest.skip("Requires subprocess support") code = textwrap.dedent(code).strip() try: for _ in range(rerun): # run flakily failing test multiple times subprocess.check_output( [sys.executable, "-c", code], cwd=os.getcwd(), stderr=subprocess.STDOUT, text=True, ) except subprocess.CalledProcessError as ex: > raise RuntimeError( f"Subprocess failed with exit code {ex.returncode}.\n\n" f"Code:\n" f"```python\n" f"{code}\n" f"```\n\n" f"Output:\n" f"{ex.output}" ) from None E RuntimeError: Subprocess failed with exit code -6. E E Code: E ```python E import sys E sys.path.insert(0, '/wrk/forked/pybind11/tests') E import standalone_enum_module as m E assert m.SomeEnum.__class__.__name__ == "pybind11_type" E ``` E E Output: E munmap_chunk(): invalid pointer _ = 0 code = 'import sys\nsys.path.insert(0, \'/wrk/forked/pybind11/tests\')\nimport standalone_enum_module as m\nassert m.SomeEnum.__class__.__name__ == "pybind11_type"' os = <module 'os' (frozen)> rerun = 1 subprocess = <module 'subprocess' from '/wrk/cpython_installs/v3.14.2_df793163d58_default/lib/python3.14/subprocess.py'> sys = <module 'sys' (built-in)> textwrap = <module 'textwrap' from '/wrk/cpython_installs/v3.14.2_df793163d58_default/lib/python3.14/textwrap.py'> env.py:68: RuntimeError =========================== short test summary info ============================ FAILED test_standalone_enum_module.py::test_enum_import_exit_no_crash - Runti... ============================== 1 failed in 0.23s =============================== ERROR: completed_process.returncode=1 * Add "Added in PR #6015" comments, for easy reference back to this PR * test: use PYBIND11_CATCH2_SKIP_IF for Python 3.12 enum restart skip Replace #if/#else/#endif preprocessor guard with runtime PYBIND11_CATCH2_SKIP_IF so the test is always compiled and shows [ SKIPPED ] in output on Python 3.12. Made-with: Cursor * fix: suppress MSVC C4127 in PYBIND11_CATCH2_SKIP_IF macro The constant condition in PYBIND11_CATCH2_SKIP_IF triggers MSVC warning C4127 (conditional expression is constant), which becomes a build error under /WX. Made-with: Cursor
1 parent e8cead1 commit 524d72b

File tree

7 files changed

+90
-20
lines changed

7 files changed

+90
-20
lines changed

include/pybind11/detail/common.h

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -606,15 +606,6 @@ enum class return_value_policy : uint8_t {
606606

607607
PYBIND11_NAMESPACE_BEGIN(detail)
608608

609-
// Py_IsFinalizing() is a public API since 3.13; before that use _Py_IsFinalizing().
610-
inline bool py_is_finalizing() {
611-
#if PY_VERSION_HEX >= 0x030D0000
612-
return Py_IsFinalizing() != 0;
613-
#else
614-
return _Py_IsFinalizing() != 0;
615-
#endif
616-
}
617-
618609
static constexpr int log2(size_t n, int k = 0) { return (n <= 1) ? k : log2(n >> 1, k + 1); }
619610

620611
// Returns the size as a multiple of sizeof(void *), rounded up.

include/pybind11/pybind11.h

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -936,11 +936,8 @@ class cpp_function : public function {
936936
std::free(const_cast<char *>(arg.descr));
937937
}
938938
}
939-
// During finalization, default arg values may already be freed by GC.
940-
if (!detail::py_is_finalizing()) {
941-
for (auto &arg : rec->args) {
942-
arg.value.dec_ref();
943-
}
939+
for (auto &arg : rec->args) {
940+
arg.value.dec_ref();
944941
}
945942
if (rec->def) {
946943
std::free(const_cast<char *>(rec->def->ml_doc));
@@ -1435,12 +1432,6 @@ PYBIND11_NAMESPACE_BEGIN(function_record_PyTypeObject_methods)
14351432

14361433
// This implementation needs the definition of `class cpp_function`.
14371434
inline void tp_dealloc_impl(PyObject *self) {
1438-
// Skip dealloc during finalization — GC may have already freed objects
1439-
// reachable from the function record (e.g. default arg values), causing
1440-
// use-after-free in destruct().
1441-
if (detail::py_is_finalizing()) {
1442-
return;
1443-
}
14441435
// Save type before PyObject_Free invalidates self.
14451436
auto *type = Py_TYPE(self);
14461437
auto *py_func_rec = reinterpret_cast<function_record_PyObject *>(self);
@@ -2687,19 +2678,41 @@ class class_ : public detail::generic_type {
26872678
if (rec_fget) {
26882679
char *doc_prev = rec_fget->doc; /* 'extra' field may include a property-specific
26892680
documentation string */
2681+
auto args_before = rec_fget->args.size();
26902682
detail::process_attributes<Extra...>::init(extra..., rec_fget);
26912683
if (rec_fget->doc && rec_fget->doc != doc_prev) {
26922684
std::free(doc_prev);
26932685
rec_fget->doc = PYBIND11_COMPAT_STRDUP(rec_fget->doc);
26942686
}
2687+
// Args added by process_attributes (e.g. "self" via is_method + pos_only/kw_only)
2688+
// need their strings strdup'd: initialize_generic's strdup loop already ran during
2689+
// cpp_function construction, so it won't process these late additions. Without this,
2690+
// destruct() would call free() on string literals. See gh-5976.
2691+
for (auto i = args_before; i < rec_fget->args.size(); ++i) {
2692+
if (rec_fget->args[i].name) {
2693+
rec_fget->args[i].name = PYBIND11_COMPAT_STRDUP(rec_fget->args[i].name);
2694+
}
2695+
if (rec_fget->args[i].descr) {
2696+
rec_fget->args[i].descr = PYBIND11_COMPAT_STRDUP(rec_fget->args[i].descr);
2697+
}
2698+
}
26952699
}
26962700
if (rec_fset) {
26972701
char *doc_prev = rec_fset->doc;
2702+
auto args_before = rec_fset->args.size();
26982703
detail::process_attributes<Extra...>::init(extra..., rec_fset);
26992704
if (rec_fset->doc && rec_fset->doc != doc_prev) {
27002705
std::free(doc_prev);
27012706
rec_fset->doc = PYBIND11_COMPAT_STRDUP(rec_fset->doc);
27022707
}
2708+
for (auto i = args_before; i < rec_fset->args.size(); ++i) {
2709+
if (rec_fset->args[i].name) {
2710+
rec_fset->args[i].name = PYBIND11_COMPAT_STRDUP(rec_fset->args[i].name);
2711+
}
2712+
if (rec_fset->args[i].descr) {
2713+
rec_fset->args[i].descr = PYBIND11_COMPAT_STRDUP(rec_fset->args[i].descr);
2714+
}
2715+
}
27032716
if (!rec_active) {
27042717
rec_active = rec_fset;
27052718
}

tests/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ set(PYBIND11_TEST_FILES
172172
test_scoped_critical_section
173173
test_sequences_and_iterators
174174
test_smart_ptr
175+
test_standalone_enum_module.py
175176
test_stl
176177
test_stl_binders
177178
test_tagbased_polymorphic
@@ -249,6 +250,7 @@ tests_extra_targets("test_exceptions.py" "cross_module_interleaved_error_already
249250
tests_extra_targets("test_gil_scoped.py" "cross_module_gil_utils")
250251
tests_extra_targets("test_cpp_conduit.py"
251252
"exo_planet_pybind11;exo_planet_c_api;home_planet_very_lonely_traveler")
253+
tests_extra_targets("test_standalone_enum_module.py" "standalone_enum_module")
252254

253255
set(PYBIND11_EIGEN_REPO
254256
"https://gitlab.com/libeigen/eigen.git"

tests/standalone_enum_module.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Copyright (c) 2026 The pybind Community.
2+
3+
#include <pybind11/pybind11.h>
4+
5+
namespace standalone_enum_module_ns {
6+
enum SomeEnum {};
7+
} // namespace standalone_enum_module_ns
8+
9+
using namespace standalone_enum_module_ns;
10+
11+
PYBIND11_MODULE(standalone_enum_module, m) { // Added in PR #6015
12+
pybind11::enum_<SomeEnum> some_enum_wrapper(m, "SomeEnum");
13+
}
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
from __future__ import annotations
2+
3+
import os
4+
5+
import env
6+
7+
8+
def test_enum_import_exit_no_crash():
9+
# Added in PR #6015. Modeled after reproducer under issue #5976
10+
env.check_script_success_in_subprocess(
11+
f"""
12+
import sys
13+
sys.path.insert(0, {os.path.dirname(env.__file__)!r})
14+
import standalone_enum_module as m
15+
assert m.SomeEnum.__class__.__name__ == "pybind11_type"
16+
""",
17+
rerun=1,
18+
)

tests/test_with_catch/catch_skip.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,18 @@
44

55
#pragma once
66

7+
#include <pybind11/detail/pybind11_namespace_macros.h>
8+
79
#include <catch.hpp>
810

911
#define PYBIND11_CATCH2_SKIP_IF(condition, reason) \
1012
do { \
13+
PYBIND11_WARNING_PUSH \
14+
PYBIND11_WARNING_DISABLE_MSVC(4127) \
1115
if (condition) { \
1216
Catch::cout() << "[ SKIPPED ] " << (reason) << '\n'; \
1317
Catch::cout().flush(); \
1418
return; \
1519
} \
20+
PYBIND11_WARNING_POP \
1621
} while (0)

tests/test_with_catch/test_interpreter.cpp

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
// catch 2.0.1; this should be fixed in the next catch release after 2.0.1).
66
PYBIND11_WARNING_DISABLE_MSVC(4996)
77

8+
#include "catch_skip.h"
9+
810
#include <catch.hpp>
911
#include <cstdlib>
1012
#include <fstream>
@@ -84,6 +86,14 @@ PYBIND11_EMBEDDED_MODULE(trampoline_module, m) {
8486
.def("func", &test_override_cache_helper::func);
8587
}
8688

89+
enum class SomeEnum { value1, value2 }; // Added in PR #6015
90+
91+
PYBIND11_EMBEDDED_MODULE(enum_module, m, py::multiple_interpreters::per_interpreter_gil()) {
92+
py::enum_<SomeEnum>(m, "SomeEnum")
93+
.value("value1", SomeEnum::value1)
94+
.value("value2", SomeEnum::value2);
95+
}
96+
8797
PYBIND11_EMBEDDED_MODULE(throw_exception, ) { throw std::runtime_error("C++ Error"); }
8898

8999
PYBIND11_EMBEDDED_MODULE(throw_error_already_set, ) {
@@ -343,6 +353,24 @@ TEST_CASE("Restart the interpreter") {
343353
REQUIRE(py_widget.attr("the_message").cast<std::string>() == "Hello after restart");
344354
}
345355

356+
TEST_CASE("Enum module survives restart") { // Added in PR #6015
357+
// Regression test for gh-5976: py::enum_ uses def_property_static, which
358+
// calls process_attributes::init after initialize_generic's strdup loop,
359+
// leaving arg names as string literals. Without the fix, destruct() would
360+
// call free() on those literals during interpreter finalization.
361+
PYBIND11_CATCH2_SKIP_IF(PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION == 12,
362+
"Pre-existing crash in enum cleanup during finalize on Python 3.12");
363+
364+
auto enum_mod = py::module_::import("enum_module");
365+
REQUIRE(enum_mod.attr("SomeEnum").attr("value1").attr("name").cast<std::string>() == "value1");
366+
367+
py::finalize_interpreter();
368+
py::initialize_interpreter();
369+
370+
enum_mod = py::module_::import("enum_module");
371+
REQUIRE(enum_mod.attr("SomeEnum").attr("value2").attr("name").cast<std::string>() == "value2");
372+
}
373+
346374
TEST_CASE("Execution frame") {
347375
// When the interpreter is embedded, there is no execution frame, but `py::exec`
348376
// should still function by using reasonable globals: `__main__.__dict__`.

0 commit comments

Comments
 (0)