From aeda49ed0b4e6e8abba7abc265ace86a6c26ba66 Mon Sep 17 00:00:00 2001 From: Vasily Litvinov Date: Mon, 2 Sep 2024 19:01:59 +0300 Subject: [PATCH] Properly translate C++ exception to Python exception when creating Python 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> --- CMakeLists.txt | 1 + include/pybind11/detail/class.h | 15 +++- .../pybind11/detail/exception_translation.h | 71 +++++++++++++++++++ include/pybind11/pybind11.h | 55 +------------- tests/extra_python_package/test_files.py | 1 + tests/test_buffers.cpp | 12 ++++ tests/test_buffers.py | 7 ++ 7 files changed, 108 insertions(+), 54 deletions(-) create mode 100644 include/pybind11/detail/exception_translation.h diff --git a/CMakeLists.txt b/CMakeLists.txt index a641925e9..2b6b564e5 100644 --- 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 index 454c186b9..6c353eb09 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -12,6 +12,8 @@ #include #include +#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 new file mode 100644 index 000000000..2764180bb --- /dev/null +++ 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 &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 index 74919a7d5..1806583e6 100644 --- 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 &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 @@ protected: 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 index 7ad9b8062..18299bf5a 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -59,6 +59,7 @@ detail_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", } eigen_headers = { diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index b5b8c355b..a6c527c10 100644 --- 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_(m, "BrokenMatrix", py::buffer_protocol()) + .def(py::init()) + .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 index 84a301e25..535f33c2d 100644 --- 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__)