From e7761e338313ae649b8a694f1d6dee93b3b72a73 Mon Sep 17 00:00:00 2001 From: oremanj Date: Tue, 25 Sep 2018 14:55:18 -0700 Subject: [PATCH] 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 --- include/pybind11/attr.h | 2 +- include/pybind11/cast.h | 2 +- include/pybind11/pybind11.h | 17 +++++++++++------ tests/test_class.cpp | 13 +++++++++++++ tests/test_class.py | 7 +++++++ 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index dce875a6b..985bc5794 100644 --- a/include/pybind11/attr.h +++ b/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); diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 4084d42a1..5529768b3 100644 --- a/include/pybind11/cast.h +++ b/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; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 84577e975..14ea8dc39 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -420,8 +420,8 @@ protected: 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 @@ protected: 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 @@ protected: // 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 @@ protected: 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 @@ protected: " 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 9265e2e37..9ed1f50bb 100644 --- a/tests/test_class.cpp +++ b/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_(m, "StringWrapper").def(py::init()); + py::implicitly_convertible(); } template class BreaksBase { public: virtual ~BreaksBase() = default; }; diff --git a/tests/test_class.py b/tests/test_class.py index 8cf4757cb..4a488ab65 100644 --- a/tests/test_class.py +++ b/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!")