Merge pull request #30125 from rwgk/pybind11k_merge_sh

git merge smart_holder (pybind/pybind11#5161)
google · Jun 12, 2024 · cf8345e · cf8345e
2 parents c062556 + 9abb0b4
commit cf8345e
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 5 deletions.
diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
@@ -1519,13 +1519,24 @@ enable_if_t<!cast_is_temporary_value_reference<T>::value, T> cast_ref(object &&,
 // static_assert, even though if it's in dead code, so we provide a "trampoline" to pybind11::cast
 // that only does anything in cases where pybind11::cast is valid.
 template <typename T>
-enable_if_t<cast_is_temporary_value_reference<T>::value, T> cast_safe(object &&) {
+enable_if_t<cast_is_temporary_value_reference<T>::value
+                && !detail::is_same_ignoring_cvref<T, PyObject *>::value,
+            T>
+cast_safe(object &&) {
     pybind11_fail("Internal error: cast_safe fallback invoked");
 }
 template <typename T>
 enable_if_t<std::is_void<T>::value, void> cast_safe(object &&) {}
 template <typename T>
-enable_if_t<detail::none_of<cast_is_temporary_value_reference<T>, std::is_void<T>>::value, T>
+enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value, PyObject *>
+cast_safe(object &&o) {
+    return o.release().ptr();
+}
+template <typename T>
+enable_if_t<detail::none_of<cast_is_temporary_value_reference<T>,
+                            detail::is_same_ignoring_cvref<T, PyObject *>,
+                            std::is_void<T>>::value,
+            T>
 cast_safe(object &&o) {
     return pybind11::cast<T>(std::move(o));
 }

diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -3212,10 +3212,14 @@ function get_override(const T *this_ptr, const char *name) {
             = pybind11::get_override(static_cast<const cname *>(this), name);                     \
         if (override) {                                                                           \
             auto o = override_call(__VA_ARGS__);                                                  \
-            if (pybind11::detail::cast_is_temporary_value_reference<ret_type>::value) {           \
+            PYBIND11_WARNING_PUSH                                                                 \
+            PYBIND11_WARNING_DISABLE_MSVC(4127)                                                   \
+            if (pybind11::detail::cast_is_temporary_value_reference<ret_type>::value              \
+                && !pybind11::detail::is_same_ignoring_cvref<ret_type, PyObject *>::value) {      \
                 static pybind11::detail::override_caster_t<ret_type> caster;                      \
                 return pybind11::detail::cast_ref<ret_type>(std::move(o), caster);                \
             }                                                                                     \
+            PYBIND11_WARNING_POP                                                                  \
             return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
         }                                                                                         \
     } while (false)

diff --git a/tests/test_type_caster_pyobject_ptr.cpp b/tests/test_type_caster_pyobject_ptr.cpp
@@ -5,9 +5,10 @@
 #include "pybind11_tests.h"
 
 #include <cstddef>
+#include <string>
 #include <vector>
 
-namespace {
+namespace test_type_caster_pyobject_ptr {
 
 std::vector<PyObject *> make_vector_pyobject_ptr(const py::object &ValueHolder) {
     std::vector<PyObject *> vec_obj;
@@ -18,9 +19,39 @@ std::vector<PyObject *> make_vector_pyobject_ptr(const py::object &ValueHolder)
     return vec_obj;
 }
 
-} // namespace
+struct WithPyObjectPtrReturn {
+#if defined(__clang_major__) && __clang_major__ < 4
+    WithPyObjectPtrReturn() = default;
+    WithPyObjectPtrReturn(const WithPyObjectPtrReturn &) = default;
+#endif
+    virtual ~WithPyObjectPtrReturn() = default;
+    virtual PyObject *return_pyobject_ptr() const = 0;
+};
+
+struct WithPyObjectPtrReturnTrampoline : WithPyObjectPtrReturn {
+    PyObject *return_pyobject_ptr() const override {
+        PYBIND11_OVERRIDE_PURE(PyObject *, WithPyObjectPtrReturn, return_pyobject_ptr,
+                               /* no arguments */);
+    }
+};
+
+std::string call_return_pyobject_ptr(const WithPyObjectPtrReturn *base_class_ptr) {
+    PyObject *returned_obj = base_class_ptr->return_pyobject_ptr();
+#if !defined(PYPY_VERSION) // It is not worth the trouble doing something special for PyPy.
+    if (Py_REFCNT(returned_obj) != 1) {
+        py::pybind11_fail(__FILE__ ":" PYBIND11_TOSTRING(__LINE__));
+    }
+#endif
+    auto ret_val = py::repr(returned_obj).cast<std::string>();
+    Py_DECREF(returned_obj);
+    return ret_val;
+}
+
+} // namespace test_type_caster_pyobject_ptr
 
 TEST_SUBMODULE(type_caster_pyobject_ptr, m) {
+    using namespace test_type_caster_pyobject_ptr;
+
     m.def("cast_from_pyobject_ptr", []() {
         PyObject *ptr = PyLong_FromLongLong(6758L);
         return py::cast(ptr, py::return_value_policy::take_ownership);
@@ -128,6 +159,12 @@ TEST_SUBMODULE(type_caster_pyobject_ptr, m) {
     }
 #endif
 
+    py::class_<WithPyObjectPtrReturn, WithPyObjectPtrReturnTrampoline>(m, "WithPyObjectPtrReturn")
+        .def(py::init<>())
+        .def("return_pyobject_ptr", &WithPyObjectPtrReturn::return_pyobject_ptr);
+
+    m.def("call_return_pyobject_ptr", call_return_pyobject_ptr);
+
     // This test exercises functionality (`py::cast<PyObject *>(handle_nullptr)`)
     // that is needed indirectly below in py_arg_handle_nullptr.
     m.def("pyobject_ptr_from_handle_nullptr", []() {

diff --git a/tests/test_type_caster_pyobject_ptr.py b/tests/test_type_caster_pyobject_ptr.py
@@ -104,6 +104,22 @@ def test_type_caster_name_via_incompatible_function_arguments_type_error():
         m.pass_pyobject_ptr_and_int(ValueHolder(101), ValueHolder(202))
 
 
+def test_trampoline_with_pyobject_ptr_return():
+    class Drvd(m.WithPyObjectPtrReturn):
+        def return_pyobject_ptr(self):
+            return ["11", "22", "33"]
+
+    # Basic health check: First make sure this works as expected.
+    d = Drvd()
+    assert d.return_pyobject_ptr() == ["11", "22", "33"]
+
+    while True:
+        # This failed before PR #5156: AddressSanitizer: heap-use-after-free ... in Py_DECREF
+        d_repr = m.call_return_pyobject_ptr(d)
+        assert d_repr == repr(["11", "22", "33"])
+        break  # Comment out for manual leak checking.
+
+
 def test_pyobject_ptr_from_handle_nullptr():
     assert m.pyobject_ptr_from_handle_nullptr() == "SUCCESS"