Repository navigation
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.
…ersion Move keep_alive activation entirely to postcall, so an argument-to-argument keep_alive no longer fires in precall for an overload that later fails argument conversion. Extend the postcall guard to also skip a null return handle, so a failed return-value conversion raises its own error instead of "Could not activate keep_alive!". Document that postcall can receive the PYBIND11_TRY_NEXT_OVERLOAD sentinel or a null handle. Tests cover the sentinel as nurse and as patient, the argument-to-argument case, and the failed return conversion, and no longer make the test object its own patient. Assisted-by: ClaudeCode:claude-fable-5 Claude-Session: https://claude.ai/code/session_01WRPhwYhrJKvFXU2NWe1p3W
Moving keep_alive<N, P> (N, P != 0) to postcall meant a keep_alive error was raised after the function ran, so its side effects stayed. Run the precall hook in call_impl after load_args succeeds instead, so a failed overload still does not trigger it and an error still stops the call. Also release the return value when a postcall keep_alive throws, as the dispatcher drops it. Assisted-by: ClaudeCode:claude-opus-5-5
Assisted-by: ClaudeCode:claude-opus-5-5
| extract_guard_t<Extra...>, | ||
| cast_in>(call, detail::function_ref<Return(Args...)>(cap->f)); | ||
| cast_in>(call, | ||
| detail::function_ref<Return(Args...)>(cap->f), | ||
| &process_attributes<Extra...>::precall); | ||
|
|
||
| /* Invoke call policy post-call hook */ | ||
| process_attributes<Extra...>::postcall(call, result); |
There was a problem hiding this comment.
now that precall runs only for the matched overload, should postcall skip PYBIND11_TRY_NEXT_OVERLOAD too?
| /// The handle is not always a valid object: it is the PYBIND11_TRY_NEXT_OVERLOAD sentinel | ||
| /// if argument conversion failed, and null if the call or return-value conversion failed. |
There was a problem hiding this comment.
nit: a throwing call never reaches postcall, so null only comes from a failed return conversion. Maybe "…and null if return-value conversion failed"?
| // With index 0, this runs in postcall, which the dispatcher runs even when the overload | ||
| // produced no value: `ret` is the PYBIND11_TRY_NEXT_OVERLOAD sentinel ((PyObject *) 1) if | ||
| // the overload bailed out of `load_args`, and null if the return-value conversion failed | ||
| // (with the real error already set). There is no relationship to establish then. The | ||
| // sentinel would pass the guards below and be dereferenced, and a null `ret` would be | ||
| // reported as "Could not activate keep_alive!", masking the real error. | ||
| const bool uses_ret = Nurse == 0 || Patient == 0; | ||
| if (uses_ret && (!ret || ret.ptr() == PYBIND11_TRY_NEXT_OVERLOAD)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: maybe note that uses_ret is needed because the precall path passes handle()? Otherwise it looks simplifiable
| assert m.without_gil() == "GIL released" | ||
|
|
||
|
|
||
| def test_keep_alive_failed_overload(): |
There was a problem hiding this comment.
could we cover a rejected sole candidate too? It also crashed on 3.1.0
with pytest.raises(TypeError):
m.keep_alive_single(obj, "x")
m.def(
"keep_alive_single",
[](const KeepAliveOverload &, int) { return KeepAliveOverload(); },
py::keep_alive<0, 1>());The dispatcher now runs postcall only for the overload that matched, like precall. Add a test for a rejected sole candidate, and clarify the postcall and keep_alive_impl comments. Assisted-by: ClaudeCode:claude-opus-5-5
pybind11 3.1.0 runs keep_alive's postcall on the try-next-overload sentinel, so any binding with keep_alive<0, N> segfaults when called with arguments that match no overload -- Node.addObject(1.0) in SofaPython3. Fixed upstream by pybind/pybind11#6183. The runtime test now checks that such a call raises TypeError. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Downstream confirmation from SofaPython3 (SOFA's bindings), which hits this through SOFA master + SofaPython3 master built from source on CPython 3.14.7 (linux-64, conda-forge toolchain):
A minimal |
|
I just started a codex session for merging master and reviewing this PR. It'll probably run for a while, add commits, and run the CI. I'll post another comment when I'm done reviewing the new work myself. |
Run precall after cast_op and parameter conversion without adding copies or moves, and keep it before guard construction. Protect the owned return throughout postcall so any throwing hook releases it. Extend regression coverage for reference rejection, converting overloads, constructors, return retention, custom hooks and return conversion errors.
|
codex GPT-6.1-Sol ultra Work report for pybind11 PR #6183, requested October 7, 2026. Reviewed the original PR head a7e62c8 after reading both maintenance notes, the PR description, downstream confirmation, and existing review discussions. The review covered dispatcher overload resolution, argument and return conversion, Merging master was warranted because upstream had moved the non-template implementations into Two remaining correctness issues were reproduced and fixed in commit 5dec644:
The regression run against the merged PR, before production fixes, had 3 failures and 19 passes: late-reference patient retention, an unexpected precall event for that rejected reference, and a missing child destructor after a throwing custom postcall. All three pass with the fixes. Expanded call-policy tests also cover the converting overload pass, rejected constructor overloads, actual retention with the return as nurse and patient, both failed-return-conversion directions and their underlying unregistered-type cause, hook behavior for successful and throwing calls, null postcall results, and GIL-release guard ordering. Replaced the error-call global counter with caller-owned event lists. Local validation used GCC 13.3.0, C++20, and CPython 3.14.4 at
The corrected focused call-policy/copy-move run passed all 32 cases. The full suite caught extra tuple/pair moves in an intermediate wrapper; the final invocation method preserves the original counts, with the existing assertions unchanged. Separate checks against the actual Pushed both commits with a normal fast-forward push to Final CI verification at 2026-10-07 08:45:13 UTC confirmed GitHub still points to
External statuses also passed: pre-commit.ci - pr, docs/readthedocs.org:pybind11, continuous-integration/appveyor/pr. All four precompiled-mode jobs passed, covering Ubuntu Python 3.13, Ubuntu free-threaded Python 3.14, Windows Python 3.13, and macOS Python 3.13. The macOS C++11 job, PyPy and GraalPy configurations, free-threaded configurations, and wheel jobs also passed. The Windows regular Python 3.14/C++20 environment associated with #6174 passed on its first attempt. No CI failures occurred, so no deflaking reruns or additional CI-fix commits were needed. Final handoff verification: the worktree is clean, and the local branch, its existing upstream, and the remote PR branch all point to the tested commit. No ABI data layout or platform ABI ID changes were introduced by the review fixes. Custom attribute hooks follow the documented timing: precall after argument conversion, postcall only for a matched call, and a null postcall handle when return conversion fails. No findings remain unresolved from this review. |
|
This is an independent Fable 5 review: Review of PR #6183 (local branch Verdict: approve, with a few non-blocking comments. The fix is correct, the design keeps the code-sharing property of the outlined What I checked:
Comments, in rough priority order:
Nothing I found blocks merging. The two items I would actually ask for before merge are the changelog mention of the |
Remove unused argument-loader invocation overloads, standardize precall naming, and explain the function_ref parameter boundary and copy elision. Document guard ordering and test caster rejection, guarded calls, by-value parameter destruction, and extraction before GIL release.
This branch has not been deployed
5 participants
🤖 AI text below 🤖
Description
This PR contains the commit from, and closes #6154 by @jturney. It adds more commits on top.
#6154: a
py::keep_alivewith index 0 on an overloaded function causes a segfault if an earlier overload fails argument conversion. The dispatcher callspostcallwith thePYBIND11_TRY_NEXT_OVERLOADsentinel ((PyObject *) 1), andkeep_alive_impldereferences it.The added commits extend that fix:
postcallonly for the overload that matched. Before, it also ranpostcallwith the sentinel for an overload that failed argument conversion.precallhook now runs after argument loading (load_args) and final caster extraction (cast_op), before guard construction and the C++ call. An argument-to-argumentkeep_alive(for examplekeep_alive<1, 2>) therefore cannot retain a patient for an overload rejected during conversion, including a late C++ reference-conversion failure. Akeep_aliveerror still stops the call before it has side effects.call_guardconstruction now follows caster extraction for every binding, including those withoutkeep_alive. Conversion rejected afterload_argsconstructs no guard. Python-dependent extraction therefore runs before agil_scoped_releaseguard releases the GIL. The invocation wrapper's by-value parameters are initialized before its local guard and destroyed after it; forwarding into the actual callable remains inside the guard scope.keep_alive_implskips a null return handle. A failed return-value conversion now raises its own error, not "Could not activate keep_alive!".postcall. An exception from any postcall hook, including an index-0keep_aliveor a custom attribute, releases that value.precallandpostcalldocumentation now says when each runs, and that thepostcallhandle is null if return-value conversion failed.Custom
process_attributespecializations follow the same ordering:precallafter successful argument conversion, andpostcallonly for the overload that matched.keep_aliveis the only built-in attribute that usesprecall.Tests cover the return value as nurse and as patient, rejected sole and constructor candidates, the argument-to-argument case, late reference-conversion failures, failed return conversion, throwing precall/postcall hooks, GIL-release ordering, and guard construction and destruction relative to argument extraction and invocation parameters.
The shared per-signature dispatch path receives
precallthrough a function pointer. An indirect hook call can remain when the compiler retains the outlined helper; optimization can eliminate an empty hook call. A GCC 13.3-O2probe eliminated it without LTO. This is not a runtime latency measurement.Closes #6154
Suggested changelog entry:
py::keep_alivefails argument conversion.py::keep_aliveand customprocess_attribute::precallhooks now run after successful argument loading and caster extraction;postcallruns only for a matched overload, and a throwing postcall hook releases the converted return value. Call guards now start after argument conversion, including caster extraction, so a candidate rejected during conversion does not construct a guard.📚 Documentation preview 📚: https://pybind11--6183.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--6183.org.readthedocs.build/