From 3902773a1afbb1f53d2c8783b1dda3cf567021cc Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 21 May 2022 20:46:04 -0700 Subject: [PATCH 1/4] Add missing error handling to module_::def_submodule --- include/pybind11/pybind11.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4c392713c9..c033f129bc 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1173,9 +1173,16 @@ class module_ : public object { py::module_ m3 = m2.def_submodule("subsub", "A submodule of 'example.sub'"); \endrst */ module_ def_submodule(const char *name, const char *doc = nullptr) { - std::string full_name - = std::string(PyModule_GetName(m_ptr)) + std::string(".") + std::string(name); - auto result = reinterpret_borrow(PyImport_AddModule(full_name.c_str())); + const char *this_name = PyModule_GetName(m_ptr); + if (this_name == nullptr) { + throw error_already_set(); + } + std::string full_name = std::string(this_name) + "." + name; + handle submodule = PyImport_AddModule(full_name.c_str()); + if (!submodule) { + throw error_already_set(); + } + auto result = reinterpret_borrow(submodule); if (doc && options::show_user_defined_docstrings()) { result.attr("__doc__") = pybind11::str(doc); } From b7e0969529c1f6deb7729cbdc9d98ae8ea34d339 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 25 May 2022 00:50:00 -0700 Subject: [PATCH 2/4] Add test_def_submodule_failures --- tests/test_modules.cpp | 2 ++ tests/test_modules.py | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp index 8aa2a4bdbd..18a7ec74cc 100644 --- a/tests/test_modules.cpp +++ b/tests/test_modules.cpp @@ -120,4 +120,6 @@ TEST_SUBMODULE(modules, m) { return failures; }); + + m.def("def_submodule", [](py::module_ m, const char *name) { return m.def_submodule(name); }); } diff --git a/tests/test_modules.py b/tests/test_modules.py index 15db8355ef..c6e6c9b488 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -1,3 +1,5 @@ +import pytest + from pybind11_tests import ConstructorStats from pybind11_tests import modules as m from pybind11_tests.modules import subsubmodule as ms @@ -89,3 +91,25 @@ def test_builtin_key_type(): keys = __builtins__.__dict__.keys() assert {type(k) for k in keys} == {str} + + +def test_def_submodule_failures(): + sm = m.def_submodule(m, b"ScratchSubModuleName") # Using bytes to show it works. + assert sm.__name__ == m.__name__ + "." + "ScratchSubModuleName" + malformed_utf8 = b"\x80" + # Meant to exercise PyModule_GetName(): + sm_name_orig = sm.__name__ + sm.__name__ = malformed_utf8 + try: + with pytest.raises(Exception): + # Seen with Python 3.9: SystemError: nameless module + # But we do not want to exercise the internals of PyModule_GetName(), which could + # change in future versions of Python, but a bad __name__ is very likely to cause + # some kind of failure indefinitely. + m.def_submodule(sm, b"SubSubModuleName") + finally: + # Clean up to ensure nothing gets upset by a module with an invalid __name__. + sm.__name__ = sm_name_orig # Purely precautionary. + # Meant to exercise PyImport_AddModule(): + with pytest.raises(UnicodeDecodeError): + m.def_submodule(sm, malformed_utf8) From 4dc4a4f7faffa8f9b33763ed93f99c34275167d2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 25 May 2022 23:09:56 -0700 Subject: [PATCH 3/4] PyPy only: Skip test with trigger for PyModule_GetName() failure. --- tests/test_modules.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/test_modules.py b/tests/test_modules.py index c6e6c9b488..e11d68e78e 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -1,5 +1,6 @@ import pytest +import env from pybind11_tests import ConstructorStats from pybind11_tests import modules as m from pybind11_tests.modules import subsubmodule as ms @@ -93,23 +94,28 @@ def test_builtin_key_type(): assert {type(k) for k in keys} == {str} +@pytest.mark.xfail("env.PYPY", reason="PyModule_GetName()") def test_def_submodule_failures(): sm = m.def_submodule(m, b"ScratchSubModuleName") # Using bytes to show it works. assert sm.__name__ == m.__name__ + "." + "ScratchSubModuleName" malformed_utf8 = b"\x80" - # Meant to exercise PyModule_GetName(): - sm_name_orig = sm.__name__ - sm.__name__ = malformed_utf8 - try: - with pytest.raises(Exception): - # Seen with Python 3.9: SystemError: nameless module - # But we do not want to exercise the internals of PyModule_GetName(), which could - # change in future versions of Python, but a bad __name__ is very likely to cause - # some kind of failure indefinitely. - m.def_submodule(sm, b"SubSubModuleName") - finally: - # Clean up to ensure nothing gets upset by a module with an invalid __name__. - sm.__name__ = sm_name_orig # Purely precautionary. - # Meant to exercise PyImport_AddModule(): + if env.PYPY: + # It is not worth the effort finding a trigger for a failure when running with PyPy. + pytest.skip("Sufficiently exercised on platforms other than PyPy.") + else: + # Meant to trigger PyModule_GetName() failure: + sm_name_orig = sm.__name__ + sm.__name__ = malformed_utf8 + try: + with pytest.raises(Exception): + # Seen with Python 3.9: SystemError: nameless module + # But we do not want to exercise the internals of PyModule_GetName(), which could + # change in future versions of Python, but a bad __name__ is very likely to cause + # some kind of failure indefinitely. + m.def_submodule(sm, b"SubSubModuleName") + finally: + # Clean up to ensure nothing gets upset by a module with an invalid __name__. + sm.__name__ = sm_name_orig # Purely precautionary. + # Meant to trigger PyImport_AddModule() failure: with pytest.raises(UnicodeDecodeError): m.def_submodule(sm, malformed_utf8) From dbbe9d1cb3dd2271fdc83e087829ea7f863c499f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 28 May 2022 12:04:09 -0700 Subject: [PATCH 4/4] Reapply minor fix that accidentally got lost in transfer from PR #3964 --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c033f129bc..ea97ed072f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1177,7 +1177,7 @@ class module_ : public object { if (this_name == nullptr) { throw error_already_set(); } - std::string full_name = std::string(this_name) + "." + name; + std::string full_name = std::string(this_name) + '.' + name; handle submodule = PyImport_AddModule(full_name.c_str()); if (!submodule) { throw error_already_set();