From 748ae2270b333d7024d9ef7ffb7f2ca947b2491f Mon Sep 17 00:00:00 2001
From: "Ralf W. Grosse-Kunstleve" <rwgk@google.com>
Date: Sat, 28 May 2022 16:40:57 -0700
Subject: [PATCH] Add missing error handling to `module_::def_submodule`
 (#3973)

* Add missing error handling to module_::def_submodule

* Add test_def_submodule_failures

* PyPy only: Skip test with trigger for PyModule_GetName() failure.

* Reapply minor fix that accidentally got lost in transfer from PR #3964
---
 include/pybind11/pybind11.h | 13 ++++++++++---
 tests/test_modules.cpp      |  2 ++
 tests/test_modules.py       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 4c392713c..ea97ed072 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1173,9 +1173,16 @@ public:
             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<module_>(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<module_>(submodule);
         if (doc && options::show_user_defined_docstrings()) {
             result.attr("__doc__") = pybind11::str(doc);
         }
diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp
index 8aa2a4bdb..18a7ec74c 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 15db8355e..e11d68e78 100644
--- a/tests/test_modules.py
+++ b/tests/test_modules.py
@@ -1,3 +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
@@ -89,3 +92,30 @@ def test_builtin_key_type():
         keys = __builtins__.__dict__.keys()
 
     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"
+    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)