Fix potential crash when calling an overloaded function (#1327)

* Fix potential crash when calling an overloaded function

The crash would occur if:
- dispatcher() uses two-pass logic (because the target is overloaded and some arguments support conversions)
- the first pass (with conversions disabled) doesn't find any matching overload
- the second pass does find a matching overload, but its return value can't be converted to Python

The code for formatting the error message assumed `it` still pointed to the selected overload,
but during the second-pass loop `it` was nullptr. Fix by setting `it` correctly if a second-pass
call returns a nullptr `handle`. Add a new test that segfaults without this fix.

* Make overload iteration const-correct so we don't have to iterate again on second-pass error

* Change test_error_after_conversions dependencies to local classes/variables

Author: oremanj

include/pybind11/attr.h

@@ -278,7 +278,7 @@ struct type_record {
     }
 };
 
-inline function_call::function_call(function_record &f, handle p) :
+inline function_call::function_call(const function_record &f, handle p) :
         func(f), parent(p) {
     args.reserve(f.nargs);
     args_convert.reserve(f.nargs);

include/pybind11/cast.h

@@ -1843,7 +1843,7 @@ struct function_record;
 
 /// Internal data associated with a single function call
 struct function_call {
-    function_call(function_record &f, handle p); // Implementation in attr.h
+    function_call(const function_record &f, handle p); // Implementation in attr.h
 
     /// The function data:
     const function_record &func;

include/pybind11/pybind11.h

@@ -420,8 +420,8 @@ class cpp_function : public function {
         using namespace detail;
 
         /* Iterator over the list of potentially admissible overloads */
-        function_record *overloads = (function_record *) PyCapsule_GetPointer(self, nullptr),
-                        *it = overloads;
+        const function_record *overloads = (function_record *) PyCapsule_GetPointer(self, nullptr),
+                              *it = overloads;
 
         /* Need to know how many arguments + keyword arguments there are to pick the right overload */
         const size_t n_args_in = (size_t) PyTuple_GET_SIZE(args_in);
@@ -477,7 +477,7 @@ class cpp_function : public function {
                    result other than PYBIND11_TRY_NEXT_OVERLOAD.
                  */
 
-                function_record &func = *it;
+                const function_record &func = *it;
                 size_t pos_args = func.nargs;    // Number of positional arguments that we need
                 if (func.has_args) --pos_args;   // (but don't count py::args
                 if (func.has_kwargs) --pos_args; //  or py::kwargs)
@@ -509,7 +509,7 @@ class cpp_function : public function {
                 // 1. Copy any position arguments given.
                 bool bad_arg = false;
                 for (; args_copied < args_to_copy; ++args_copied) {
-                    argument_record *arg_rec = args_copied < func.args.size() ? &func.args[args_copied] : nullptr;
+                    const argument_record *arg_rec = args_copied < func.args.size() ? &func.args[args_copied] : nullptr;
                     if (kwargs_in && arg_rec && arg_rec->name && PyDict_GetItemString(kwargs_in, arg_rec->name)) {
                         bad_arg = true;
                         break;
@@ -650,8 +650,13 @@ class cpp_function : public function {
                         result = PYBIND11_TRY_NEXT_OVERLOAD;
                     }
 
-                    if (result.ptr() != PYBIND11_TRY_NEXT_OVERLOAD)
+                    if (result.ptr() != PYBIND11_TRY_NEXT_OVERLOAD) {
+                        // The error reporting logic below expects 'it' to be valid, as it would be
+                        // if we'd encountered this failure in the first-pass loop.
+                        if (!result)
+                            it = &call.func;
                         break;
+                    }
                 }
             }
         } catch (error_already_set &e) {
@@ -703,7 +708,7 @@ class cpp_function : public function {
                 " arguments. The following argument types are supported:\n";
 
             int ctr = 0;
-            for (function_record *it2 = overloads; it2 != nullptr; it2 = it2->next) {
+            for (const function_record *it2 = overloads; it2 != nullptr; it2 = it2->next) {
                 msg += "    "+ std::to_string(++ctr) + ". ";
 
                 bool wrote_sig = false;

tests/test_class.cpp

@@ -341,6 +341,19 @@ TEST_SUBMODULE(class_, m) {
                 "a"_a, "b"_a, "c"_a);
     base.def("g", [](NestBase &, Nested &) {});
     base.def("h", []() { return NestBase(); });
+
+    // test_error_after_conversion
+    // The second-pass path through dispatcher() previously didn't
+    // remember which overload was used, and would crash trying to
+    // generate a useful error message
+
+    struct NotRegistered {};
+    struct StringWrapper { std::string str; };
+    m.def("test_error_after_conversions", [](int) {});
+    m.def("test_error_after_conversions",
+          [](StringWrapper) -> NotRegistered { return {}; });
+    py::class_<StringWrapper>(m, "StringWrapper").def(py::init<std::string>());
+    py::implicitly_convertible<std::string, StringWrapper>();
 }
 
 template <int N> class BreaksBase { public: virtual ~BreaksBase() = default; };

tests/test_class.py

@@ -266,3 +266,10 @@ def test_reentrant_implicit_conversion_failure(msg):
 
         Invoked with: 0
     '''
+
+
+def test_error_after_conversions():
+    with pytest.raises(TypeError) as exc_info:
+        m.test_error_after_conversions("hello")
+    assert str(exc_info.value).startswith(
+        "Unable to convert function return value to a Python type!")