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
diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
index dce875a..985bc57 100644
--- a/include/pybind11/attr.h
+++ b/include/pybind11/attr.h
@@ -278,7 +278,7 @@
}
};
-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);
diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 4084d42..5529768 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -1843,7 +1843,7 @@
/// 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;
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 84577e9..14ea8dc 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -420,8 +420,8 @@
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 @@
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 @@
// 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 @@
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 @@
" 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;
diff --git a/tests/test_class.cpp b/tests/test_class.cpp
index 9265e2e..9ed1f50 100644
--- a/tests/test_class.cpp
+++ b/tests/test_class.cpp
@@ -341,6 +341,19 @@
"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; };
diff --git a/tests/test_class.py b/tests/test_class.py
index 8cf4757..4a488ab 100644
--- a/tests/test_class.py
+++ b/tests/test_class.py
@@ -266,3 +266,10 @@
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!")