fix: do not run keep_alive for an overload that failed argument conversion - #6154
Open
jturney wants to merge 1 commit into
Open
fix: do not run keep_alive for an overload that failed argument conversion#6154jturney wants to merge 1 commit into
jturney wants to merge 1 commit into
Conversation
…rsion
Calling an overloaded function bound with `py::keep_alive<0, N>` segfaults
whenever the call is not matched by the first overload tried.
.def("make", &Holder::from_int, py::keep_alive<0, 1>())
.def("make", &Holder::from_string, py::keep_alive<0, 1>());
h.make(1) # fine
h.make("x") # SIGSEGV
The dispatch lambda in `cpp_function::initialize` invokes the post-call hook
unconditionally:
auto result = call_impl<...>(call, ...);
process_attributes<Extra...>::postcall(call, result);
but `call_impl` returns `PYBIND11_TRY_NEXT_OVERLOAD` when `load_args` fails,
and that sentinel is `((PyObject *) 1)` rather than an object. For a
`keep_alive` whose nurse or patient is index 0 the work happens in postcall,
so `keep_alive_impl` receives the sentinel as `ret`, hands it to `get_arg(0)`
and dereferences it in `_Py_TYPE`.
The existing guards do not catch it: the sentinel is neither null nor
`Py_None`, so it passes straight through the checks added in pybind#341.
Guard inside `keep_alive_impl`, next to those checks. Only the
`Nurse == 0 || Patient == 0` specialization does its work in postcall, and
`keep_alive` is the only call policy with a non-trivial postcall, so this
covers every path that can observe the sentinel. `keep_alive<1, 2>` and
friends run in precall against fully populated `call.args` and are unaffected.
The regression test crashes the interpreter without the fix. Reaching the
second overload is what matters: it is the first overload's failed conversion
that produces the sentinel.
jturney
marked this pull request as ready for review
August 26, 2026 19:14
jturney
added a commit
to jturney/Einsums
that referenced
this pull request
Aug 27, 2026
…say so
Three follow-ups to the space-typed declaration, one of which corrects it.
An index space now carries a `dim_symbol` ("no" for "occ"), and a
space-shaped axis takes its symbol from there. The first cut used the space's
NAME, which is wrong twice over. It makes the saved `symbol_ties` table a list
of tautologies, and it asserts a one-to-one relation between spaces and
extents that does not hold: a ragged space has many extents, which is why
raggedness is spelled with a prefix rather than as the bare name, and a plain
symbol "pao" would have claimed a single extent for a space that may not have
one with nothing to detect the contradiction. A space registered without a dim
symbol is now refused rather than having a name invented for it.
The field costs the saved schema nothing, which was checked and not assumed:
the IR writes space NAMES plus the symbol ties, and resolves every other
IndexSpace field from the loading process's registry.
The whole surface reaches Python, which closes a gap that predates this work:
annotate_dims and annotate_ragged_dim had never been exposed, so a Python
caller could save and load a graph but could not make one rebindable, which is
the entire point of symbolic extents. Also exposed: annotate_space_axis,
bind_ragged_extents, tensor_dim_symbols, the extent and tiling accessors,
SpaceDim, SpaceTiling, fixed, tiles, and the space-shaped factories.
Those factories are bound under their own Python names (declare_zero_tensor_over
and friends) rather than sharing the dims-based name. pybind resolves overloads
at runtime by trying each in turn, and a keep_alive<0, N> on an overload that
fails argument conversion is handed the PYBIND11_TRY_NEXT_OVERLOAD sentinel,
(PyObject *) 1, as its return value and dereferences it. A shared name is
therefore a segfault rather than an ambiguity. Found by writing the Python test
below, reported upstream as pybind/pybind11#6154 with a fix and a regression
test. The exposure is wider than these factories: the codegen puts
keep_alive<0, 1> on every returning method, so any generated Python name with
more than one overload has the same latent crash.
Last, create_* takes a space-typed shape too, and deliberately stops short of
writing dim symbols. The axes are sized and annotated with their spaces, but a
tensor allocated when the call returns cannot be resized by a bind, so a symbol
there would promise a rebindability the storage cannot honour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Calling an overloaded function bound with
py::keep_alive<0, N>segfaults whenever the call is not matched by the first overload tried. Minimal reproducer:The crash is
EXC_BAD_ACCESSin_Py_TYPE(ob=0x1). Swapping the registration order swaps which call crashes, so it is always the call that reaches an overload after an earlier one failed argument conversion.return_value_policy::referenceis not required to trigger it, and removingkeep_alivemakes it go away.Cause
The dispatch lambda in
cpp_function::initializeruns the post-call hook unconditionally (pybind11.h:596-603):auto result = call_impl<...>(call, ...); process_attributes<Extra...>::postcall(call, result);call_implbails out atpybind11.h:504when argument loading fails:and that sentinel is
((PyObject *) 1)(detail/common.h:367). For akeep_alivewhose nurse or patient is index 0 the work happens inpostcall, sokeep_alive_implreceives the sentinel asret,get_arg(0)returns it, and_Py_TYPEdereferences it.The existing guards do not catch this. The sentinel is neither null nor
Py_None, so it passes straight through the checks added in #341.Fix
A single guard inside
keep_alive_impl, beside those checks.On scope: only the
Nurse == 0 || Patient == 0specialization does its work inpostcall. The other specialization runs inprecall, against a fully populatedcall.args, and I verified thatkeep_alive<1, 2>on the same setup is unaffected.keep_aliveis also the only call policy with a non-trivialpostcall; the other three inattr.hhave empty bodies. So this guard covers every path that can observe the sentinel.An alternative would be to guard the
postcallcall site so that no policy runs on the bail-out path. That may be more correct in general, since running a post-call hook for a call that never happened is questionable on its own terms, but it changes behaviour for call policies broadly. I went with the narrow fix and am happy to switch if you prefer the other.Tests
Added to
test_call_policies. The new test crashes the interpreter without the fix, which is rather the point: pybind11 currently has no test that exercises a call policy on a failing overload, and that gap is the likely reason this went unnoticed for so long.Full suite on macOS/arm64, clang, Python 3.14, against master
5e9611a:Notes
Reproduces on both v3.1.0 and current master. I could not find an existing issue or PR for this. I searched issues and PRs for
keep_alive,postcall, andTRY_NEXT_OVERLOAD, and grepped the diffs of all 39 open PRs that touchpybind11.h.