fix: never run keep_alive for a failed overload or failed return conversion (#6183)

* fix: do not run keep_alive for an overload that failed argument conversion

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

* fix: never run keep_alive for a failed overload or failed return conversion

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

* fix: run argument-to-argument keep_alive before the call again

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

* test: collect the child before capturing output on PyPy

Assisted-by: ClaudeCode:claude-opus-5-5

* fix: skip postcall for an overload that failed argument conversion

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

* fix: complete call policy conversion and result cleanup

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.

* refactor: clarify call guard conversion boundaries

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.

* test: make call guard caster extraction const

* test: explain moved-from lifetime logging [skip ci]

---------

Co-authored-by: Justin Turney <justin.turney@gmail.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>

Author: 3 people

docs/advanced/functions.rst

@@ -233,6 +233,12 @@ is equivalent to the following pseudocode:
         return foo(args...); // forwarded arguments
     });
 
+Argument conversion, including extraction from type casters, finishes before
+the guards are constructed. A candidate rejected during argument conversion
+does not construct its guards. Call-policy ``precall`` hooks also run before
+guard construction, and the guards are destroyed before Python return-value
+conversion.
+
 The only requirement is that ``T`` is default-constructible, but otherwise any
 scope guard will work. This is very useful in combination with ``gil_scoped_release``.
 See :ref:`gil`.

include/pybind11/attr.h

@@ -409,7 +409,9 @@ struct process_attribute_default {
     /// Default implementation: do nothing
     static void init(const T &, function_record *) {}
     static void init(const T &, type_record *) {}
+    /// Runs after argument conversion succeeded, before the call.
     static void precall(function_call &) {}
+    /// Runs after the call succeeded. The handle is null if return-value conversion failed.
     static void postcall(function_call &, handle) {}
 };
 
@@ -650,8 +652,8 @@ struct process_attribute<call_guard<Ts...>> : process_attribute_default<call_gua
 
 /**
  * Process a keep_alive call policy -- invokes keep_alive_impl during the
- * pre-call handler if both Nurse, Patient != 0 and use the post-call handler
- * otherwise
+ * pre-call handler (after argument conversion succeeded) if both Nurse,
+ * Patient != 0 and use the post-call handler otherwise
  */
 template <size_t Nurse, size_t Patient>
 struct process_attribute<keep_alive<Nurse, Patient>>

include/pybind11/cast.h

@@ -2162,17 +2162,17 @@ class argument_loader {
 
     bool load_args(function_call &call) { return load_impl_sequence(call, indices{}); }
 
-    template <typename Return, typename Guard, typename Func>
+    template <typename Return, typename Guard, typename Func, typename Precall>
     // NOLINTNEXTLINE(readability-const-return-type)
-    enable_if_t<!std::is_void<Return>::value, Return> call(Func &&f) && {
-        return std::move(*this).template call_impl<remove_cv_t<Return>>(
-            std::forward<Func>(f), indices{}, Guard{});
+    enable_if_t<!std::is_void<Return>::value, Return> call(Func &&f, Precall &&precall) && {
+        return std::move(*this).template call_impl<remove_cv_t<Return>, Guard>(
+            std::forward<Func>(f), std::forward<Precall>(precall), indices{});
     }
 
-    template <typename Return, typename Guard, typename Func>
-    enable_if_t<std::is_void<Return>::value, void_type> call(Func &&f) && {
-        std::move(*this).template call_impl<remove_cv_t<Return>>(
-            std::forward<Func>(f), indices{}, Guard{});
+    template <typename Return, typename Guard, typename Func, typename Precall>
+    enable_if_t<std::is_void<Return>::value, void_type> call(Func &&f, Precall &&precall) && {
+        std::move(*this).template call_impl<remove_cv_t<Return>, Guard>(
+            std::forward<Func>(f), std::forward<Precall>(precall), indices{});
         return void_type();
     }
 
@@ -2201,9 +2201,12 @@ class argument_loader {
         return true;
     }
 
-    template <typename Return, typename Func, size_t... Is, typename Guard>
-    Return call_impl(Func &&f, index_sequence<Is...>, Guard &&) && {
-        return std::forward<Func>(f)(cast_op<Args>(std::move(std::get<Is>(argcasters)))...);
+    template <typename Return, typename Guard, typename Func, typename Precall, size_t... Is>
+    Return call_impl(Func &&f, Precall &&precall, index_sequence<Is...>) && {
+        // Func provides function_ref::invoke_with_guard, whose typed parameters finish conversion
+        // before precall. Direct casts and returns preserve copy elision (see issue #6142).
+        return std::forward<Func>(f).template invoke_with_guard<Guard>(
+            std::forward<Precall>(precall), cast_op<Args>(std::move(std::get<Is>(argcasters)))...);
     }
 
     std::tuple<make_caster<Args>...> argcasters;

include/pybind11/detail/function_ref.h

@@ -104,6 +104,17 @@ class function_ref<Ret(Params...)> {
         return callback(callable, std::forward<Params>(params)...);
     }
 
+    template <typename Guard, typename Precall>
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
+    Ret invoke_with_guard(Precall &&precall, Params... params) const {
+        // Keep the same parameter boundary as operator() so prvalue arguments can be elided.
+        // Argument conversion must finish before the hook, which runs before the guard.
+        std::forward<Precall>(precall)();
+        Guard guard{};
+        (void) guard;
+        return callback(callable, std::forward<Params>(params)...);
+    }
+
     explicit operator bool() const { return callback; }
 
     bool operator==(const function_ref<Ret(Params...)> &Other) const {

include/pybind11/pybind11-inl.h

@@ -309,6 +309,14 @@ PYBIND11_NOINLINE_ATTR PYBIND11_INLINE void keep_alive_impl(handle nurse, handle
 
 PYBIND11_NOINLINE_ATTR PYBIND11_INLINE void
 keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
+    // With index 0, this runs in postcall, where a null `ret` means the return-value conversion
+    // failed with the real error already set. Report that error, not "Could not activate
+    // keep_alive!". Without index 0, this runs in precall, where `ret` is always null.
+    const bool uses_ret = Nurse == 0 || Patient == 0;
+    if (uses_ret && !ret) {
+        return;
+    }
+
     auto get_arg = [&](size_t n) {
         if (n == 0) {
             return ret;

include/pybind11/pybind11.h

@@ -319,7 +319,9 @@ class cpp_function : public function {
     // actual function lambda so that we can get code reuse for
     // functions with the same Return, Args, and Guard.
     template <typename Return, typename Guard, typename ArgsConverter, typename... Args>
-    static handle call_impl(detail::function_call &call, detail::function_ref<Return(Args...)> f) {
+    static handle call_impl(detail::function_call &call,
+                            detail::function_ref<Return(Args...)> f,
+                            void (*precall)(detail::function_call &)) {
         using namespace detail;
         // Static assertion: function_ref must be trivially copyable to ensure safe pass-by-value.
         // Lifetime safety: The function_ref is created from cap->f which lives in the capture
@@ -335,18 +337,24 @@ class cpp_function : public function {
             return PYBIND11_TRY_NEXT_OVERLOAD;
         }
 
+        // cast_op can reject an argument after load_args succeeds. Run the hook after those
+        // conversions, but before constructing the guard, which may release the GIL.
+        auto precall_hook = [&] { precall(call); };
+
         /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */
         return_value_policy policy
             = return_value_policy_override<Return>::policy(call.func.policy);
 
         /* Perform the function call */
         handle result;
         if (call.func.is_setter) {
-            (void) std::move(args_converter).template call<Return, Guard>(f);
+            (void) std::move(args_converter).template call<Return, Guard>(f, precall_hook);
             result = none().release();
         } else {
             result = cast_out::cast(
-                std::move(args_converter).template call<Return, Guard>(f), policy, call.parent);
+                std::move(args_converter).template call<Return, Guard>(f, precall_hook),
+                policy,
+                call.parent);
         }
 
         return result;
@@ -415,9 +423,6 @@ class cpp_function : public function {
 
         /* Dispatch code which converts function arguments and performs the actual function call */
         rec->impl = [](function_call &call) -> handle {
-            /* Invoke call policy pre-call hook */
-            process_attributes<Extra...>::precall(call);
-
             /* Get a pointer to the capture object */
             const auto *data = (sizeof(capture) <= sizeof(call.func.data) ? &call.func.data
                                                                           : call.func.data[0]);
@@ -427,12 +432,18 @@ class cpp_function : public function {
                                     /* Function scope guard -- defaults to the compile-to-nothing
                                        `void_type` */
                                     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);
+            if (result.ptr() == PYBIND11_TRY_NEXT_OVERLOAD) {
+                return result;
+            }
 
-            return result;
+            // Own the result while running postcall so any throwing hook releases it.
+            auto result_guard = reinterpret_steal<object>(result);
+            process_attributes<Extra...>::postcall(call, result_guard);
+            return result_guard.release();
         };
 
         rec->nargs_pos = cast_in::args_pos >= 0