From 7b1de1e5515498679477ac4af7caff28fbe60638 Mon Sep 17 00:00:00 2001 From: Dean Moldovan Date: Sun, 3 Sep 2017 01:31:47 +0200 Subject: [PATCH] Fix nullptr dereference when loading an external-only module_local type --- docs/changelog.rst | 4 ++++ include/pybind11/cast.h | 8 +++++--- tests/local_bindings.h | 10 +++++++--- tests/pybind11_cross_module_tests.cpp | 4 ++++ tests/test_local_bindings.cpp | 4 ++++ tests/test_local_bindings.py | 16 ++++++++++++++++ 6 files changed, 40 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index dcab3b118..612a0a66d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -28,6 +28,10 @@ v2.2.1 (Not yet released) * Relax overly strict ``py::picke()`` check for matching get and set types. `#1064 `_. +* Fixed a nullptr dereference when loading a ``py::module_local`` type + that's only registered in an external module. + `#1058 `_. + v2.2.0 (August 31, 2017) ----------------------------------------------------- diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 3e0f92082..3bd1b929b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -482,9 +482,10 @@ inline PyObject *make_new_instance(PyTypeObject *type); class type_caster_generic { public: PYBIND11_NOINLINE type_caster_generic(const std::type_info &type_info) - : typeinfo(get_type_info(type_info)) { } + : typeinfo(get_type_info(type_info)), cpptype(&type_info) { } - type_caster_generic(const type_info *typeinfo) : typeinfo(typeinfo) { } + type_caster_generic(const type_info *typeinfo) + : typeinfo(typeinfo), cpptype(typeinfo ? typeinfo->cpptype : nullptr) { } bool load(handle src, bool convert) { return load_impl(src, convert); @@ -610,7 +611,7 @@ public: type_info *foreign_typeinfo = reinterpret_borrow(getattr(pytype, local_key)); // Only consider this foreign loader if actually foreign and is a loader of the correct cpp type if (foreign_typeinfo->module_local_load == &local_load - || !same_type(*typeinfo->cpptype, *foreign_typeinfo->cpptype)) + || (cpptype && !same_type(*cpptype, *foreign_typeinfo->cpptype))) return false; if (auto result = foreign_typeinfo->module_local_load(src.ptr(), foreign_typeinfo)) { @@ -722,6 +723,7 @@ public: } const type_info *typeinfo = nullptr; + const std::type_info *cpptype = nullptr; void *value = nullptr; }; diff --git a/tests/local_bindings.h b/tests/local_bindings.h index 8f48ed9eb..b6afb8086 100644 --- a/tests/local_bindings.h +++ b/tests/local_bindings.h @@ -8,9 +8,9 @@ public: int i = -1; }; -/// Registered with py::local in both main and secondary modules: +/// Registered with py::module_local in both main and secondary modules: using LocalType = LocalBase<0>; -/// Registered without py::local in both modules: +/// Registered without py::module_local in both modules: using NonLocalType = LocalBase<1>; /// A second non-local type (for stl_bind tests): using NonLocal2 = LocalBase<2>; @@ -21,6 +21,10 @@ using MixedLocalGlobal = LocalBase<4>; /// Mixed: global first, then local using MixedGlobalLocal = LocalBase<5>; +/// Registered with py::module_local only in the secondary module: +using ExternalType1 = LocalBase<6>; +using ExternalType2 = LocalBase<7>; + using LocalVec = std::vector; using LocalVec2 = std::vector; using LocalMap = std::unordered_map; @@ -39,7 +43,7 @@ PYBIND11_MAKE_OPAQUE(NonLocalMap2); // Simple bindings (used with the above): -template +template py::class_ bind_local(Args && ...args) { return py::class_(std::forward(args)...) .def(py::init()) diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 7dd70e0ac..928b25586 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -20,6 +20,10 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) { // Definitions here are tested by importing both this module and the // relevant pybind11_tests submodule from a test_whatever.py + // test_load_external + bind_local(m, "ExternalType1", py::module_local()); + bind_local(m, "ExternalType2", py::module_local()); + // test_exceptions.py m.def("raise_runtime_error", []() { PyErr_SetString(PyExc_RuntimeError, "My runtime error"); throw py::error_already_set(); }); m.def("raise_value_error", []() { PyErr_SetString(PyExc_ValueError, "My value error"); throw py::error_already_set(); }); diff --git a/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index 359d6c61c..97c02dbeb 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -15,6 +15,10 @@ #include TEST_SUBMODULE(local_bindings, m) { + // test_load_external + m.def("load_external1", [](ExternalType1 &e) { return e.i; }); + m.def("load_external2", [](ExternalType2 &e) { return e.i; }); + // test_local_bindings // Register a class with py::module_local: bind_local(m, "LocalType", py::module_local()) diff --git a/tests/test_local_bindings.py b/tests/test_local_bindings.py index e9a63ca0f..b3dc3619c 100644 --- a/tests/test_local_bindings.py +++ b/tests/test_local_bindings.py @@ -3,6 +3,22 @@ import pytest from pybind11_tests import local_bindings as m +def test_load_external(): + """Load a `py::module_local` type that's only registered in an external module""" + import pybind11_cross_module_tests as cm + + assert m.load_external1(cm.ExternalType1(11)) == 11 + assert m.load_external2(cm.ExternalType2(22)) == 22 + + with pytest.raises(TypeError) as excinfo: + assert m.load_external2(cm.ExternalType1(21)) == 21 + assert "incompatible function arguments" in str(excinfo.value) + + with pytest.raises(TypeError) as excinfo: + assert m.load_external1(cm.ExternalType2(12)) == 12 + assert "incompatible function arguments" in str(excinfo.value) + + def test_local_bindings(): """Tests that duplicate `py::module_local` class bindings work across modules"""