Properly translate C++ exception to Python exception when creating Py…

…thon buffer from wrapped object (#5324) * Add test for throwing def_buffer case * Translate C++ -> Python exceptions in buffer creation This required a little refactoring to extract exception translation to a separate method * Fix code formatting * Fix "unused parameter" warning * Refactor per review * style: pre-commit fixes * Address review comments * Address review comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pybind · Sep 2, 2024 · aeda49e · aeda49e
1 parent 66c3774
commit aeda49e
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 54 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -135,6 +135,7 @@ set(PYBIND11_HEADERS
     include/pybind11/detail/type_caster_base.h
     include/pybind11/detail/typeid.h
     include/pybind11/detail/value_and_holder.h
+    include/pybind11/detail/exception_translation.h
     include/pybind11/attr.h
     include/pybind11/buffer_info.h
     include/pybind11/cast.h

diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h
@@ -12,6 +12,8 @@
 #include <pybind11/attr.h>
 #include <pybind11/options.h>
 
+#include "exception_translation.h"
+
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 PYBIND11_NAMESPACE_BEGIN(detail)
 
@@ -581,7 +583,18 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
         return -1;
     }
     std::memset(view, 0, sizeof(Py_buffer));
-    buffer_info *info = tinfo->get_buffer(obj, tinfo->get_buffer_data);
+    buffer_info *info = nullptr;
+    try {
+        info = tinfo->get_buffer(obj, tinfo->get_buffer_data);
+    } catch (...) {
+        try_translate_exceptions();
+        raise_from(PyExc_BufferError, "Error getting buffer");
+        return -1;
+    }
+    if (info == nullptr) {
+        pybind11_fail("FATAL UNEXPECTED SITUATION: tinfo->get_buffer() returned nullptr.");
+    }
+
     if ((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && info->readonly) {
         delete info;
         // view->obj = nullptr;  // Was just memset to 0, so not necessary

diff --git a/include/pybind11/detail/exception_translation.h b/include/pybind11/detail/exception_translation.h
@@ -0,0 +1,71 @@
+/*
+    pybind11/detail/exception_translation.h: means to translate C++ exceptions to Python exceptions
+
+    Copyright (c) 2024 The Pybind Development Team.
+
+    All rights reserved. Use of this source code is governed by a
+    BSD-style license that can be found in the LICENSE file.
+*/
+
+#pragma once
+
+#include "common.h"
+#include "internals.h"
+
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
+PYBIND11_NAMESPACE_BEGIN(detail)
+
+// Apply all the extensions translators from a list
+// Return true if one of the translators completed without raising an exception
+// itself. Return of false indicates that if there are other translators
+// available, they should be tried.
+inline bool apply_exception_translators(std::forward_list<ExceptionTranslator> &translators) {
+    auto last_exception = std::current_exception();
+
+    for (auto &translator : translators) {
+        try {
+            translator(last_exception);
+            return true;
+        } catch (...) {
+            last_exception = std::current_exception();
+        }
+    }
+    return false;
+}
+
+inline void try_translate_exceptions() {
+    /* When an exception is caught, give each registered exception
+        translator a chance to translate it to a Python exception. First
+        all module-local translators will be tried in reverse order of
+        registration. If none of the module-locale translators handle
+        the exception (or there are no module-locale translators) then
+        the global translators will be tried, also in reverse order of
+        registration.
+
+        A translator may choose to do one of the following:
+
+        - catch the exception and call py::set_error()
+            to set a standard (or custom) Python exception, or
+        - do nothing and let the exception fall through to the next translator, or
+        - delegate translation to the next translator by throwing a new type of exception.
+        */
+
+    bool handled = with_internals([&](internals &internals) {
+        auto &local_exception_translators = get_local_internals().registered_exception_translators;
+        if (detail::apply_exception_translators(local_exception_translators)) {
+            return true;
+        }
+        auto &exception_translators = internals.registered_exception_translators;
+        if (detail::apply_exception_translators(exception_translators)) {
+            return true;
+        }
+        return false;
+    });
+
+    if (!handled) {
+        set_error(PyExc_SystemError, "Exception escaped from default exception translator!");
+    }
+}
+
+PYBIND11_NAMESPACE_END(detail)
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -9,8 +9,8 @@
 */
 
 #pragma once
-
 #include "detail/class.h"
+#include "detail/exception_translation.h"
 #include "detail/init.h"
 #include "attr.h"
 #include "gil.h"
@@ -95,24 +95,6 @@ inline std::string replace_newlines_and_squash(const char *text) {
     return result.substr(str_begin, str_range);
 }
 
-// Apply all the extensions translators from a list
-// Return true if one of the translators completed without raising an exception
-// itself. Return of false indicates that if there are other translators
-// available, they should be tried.
-inline bool apply_exception_translators(std::forward_list<ExceptionTranslator> &translators) {
-    auto last_exception = std::current_exception();
-
-    for (auto &translator : translators) {
-        try {
-            translator(last_exception);
-            return true;
-        } catch (...) {
-            last_exception = std::current_exception();
-        }
-    }
-    return false;
-}
-
 #if defined(_MSC_VER)
 #    define PYBIND11_COMPAT_STRDUP _strdup
 #else
@@ -1038,40 +1020,7 @@ class cpp_function : public function {
             throw;
 #endif
         } catch (...) {
-            /* When an exception is caught, give each registered exception
-               translator a chance to translate it to a Python exception. First
-               all module-local translators will be tried in reverse order of
-               registration. If none of the module-locale translators handle
-               the exception (or there are no module-locale translators) then
-               the global translators will be tried, also in reverse order of
-               registration.
-
-               A translator may choose to do one of the following:
-
-                - catch the exception and call py::set_error()
-                  to set a standard (or custom) Python exception, or
-                - do nothing and let the exception fall through to the next translator, or
-                - delegate translation to the next translator by throwing a new type of exception.
-             */
-
-            bool handled = with_internals([&](internals &internals) {
-                auto &local_exception_translators
-                    = get_local_internals().registered_exception_translators;
-                if (detail::apply_exception_translators(local_exception_translators)) {
-                    return true;
-                }
-                auto &exception_translators = internals.registered_exception_translators;
-                if (detail::apply_exception_translators(exception_translators)) {
-                    return true;
-                }
-                return false;
-            });
-
-            if (handled) {
-                return nullptr;
-            }
-
-            set_error(PyExc_SystemError, "Exception escaped from default exception translator!");
+            try_translate_exceptions();
             return nullptr;
         }
 

diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py
@@ -59,6 +59,7 @@
     "include/pybind11/detail/type_caster_base.h",
     "include/pybind11/detail/typeid.h",
     "include/pybind11/detail/value_and_holder.h",
+    "include/pybind11/detail/exception_translation.h",
 }
 
 eigen_headers = {

diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp
@@ -167,6 +167,18 @@ TEST_SUBMODULE(buffers, m) {
                  sizeof(float)});
         });
 
+    class BrokenMatrix : public Matrix {
+    public:
+        BrokenMatrix(py::ssize_t rows, py::ssize_t cols) : Matrix(rows, cols) {}
+        void throw_runtime_error() { throw std::runtime_error("See PR #5324 for context."); }
+    };
+    py::class_<BrokenMatrix>(m, "BrokenMatrix", py::buffer_protocol())
+        .def(py::init<py::ssize_t, py::ssize_t>())
+        .def_buffer([](BrokenMatrix &m) {
+            m.throw_runtime_error();
+            return py::buffer_info();
+        });
+
     // test_inherited_protocol
     class SquareMatrix : public Matrix {
     public:

diff --git a/tests/test_buffers.py b/tests/test_buffers.py
@@ -228,3 +228,10 @@ def test_buffer_docstring():
         m.get_buffer_info.__doc__.strip()
         == "get_buffer_info(arg0: Buffer) -> pybind11_tests.buffers.buffer_info"
     )
+
+
+def test_buffer_exception():
+    with pytest.raises(BufferError, match="Error getting buffer") as excinfo:
+        memoryview(m.BrokenMatrix(1, 1))
+    assert isinstance(excinfo.value.__cause__, RuntimeError)
+    assert "for context" in str(excinfo.value.__cause__)