From b14891ce201c371eca771b71b90ec52db8aa11c8 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sun, 27 Dec 2020 00:34:56 +0100 Subject: [PATCH 1/5] Fix leak in the test_copy_move::test_move_fallback --- tests/test_copy_move.cpp | 3 ++- tests/test_copy_move.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 2704217aa6..322e9bb85d 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -214,6 +214,7 @@ TEST_SUBMODULE(copy_move_policies, m) { }; py::class_(m, "MoveIssue2").def(py::init()).def_readwrite("value", &MoveIssue2::v); - m.def("get_moveissue1", [](int i) { return new MoveIssue1(i); }, py::return_value_policy::move); + // #2742: Don't expect ownership of raw pointer to `new`ed object to be transferred with `py::return_value_policy::move` + m.def("get_moveissue1", [](int i) { return std::unique_ptr(new MoveIssue1(i)); }, py::return_value_policy::move); m.def("get_moveissue2", [](int i) { return MoveIssue2(i); }, py::return_value_policy::move); } diff --git a/tests/test_copy_move.py b/tests/test_copy_move.py index 7e3cc168ca..1d98952200 100644 --- a/tests/test_copy_move.py +++ b/tests/test_copy_move.py @@ -119,7 +119,7 @@ def test_private_op_new(): def test_move_fallback(): """#389: rvp::move should fall-through to copy on non-movable objects""" - m2 = m.get_moveissue2(2) - assert m2.value == 2 m1 = m.get_moveissue1(1) assert m1.value == 1 + m2 = m.get_moveissue2(2) + assert m2.value == 2 From 16bece7a2feda2f1d06c8c3c02e7122b9b0043f3 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Wed, 30 Dec 2020 20:54:47 +0100 Subject: [PATCH 2/5] Fix leaking PyMethodDef in test_class::test_implicit_conversion_life_support --- tests/test_class.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 890fab736f..523d954c52 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -231,7 +231,8 @@ TEST_SUBMODULE(class_, m) { }; auto def = new PyMethodDef{"f", f, METH_VARARGS, nullptr}; - return py::reinterpret_steal(PyCFunction_NewEx(def, nullptr, m.ptr())); + py::capsule def_capsule(def, [](void *ptr) { delete reinterpret_cast(ptr); }); + return py::reinterpret_steal(PyCFunction_NewEx(def, def_capsule.ptr(), m.ptr())); }()); // test_operator_new_delete From aaeddc3d1516ab8ab59139747df3da45417bc74a Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sun, 27 Dec 2020 14:34:01 +0100 Subject: [PATCH 3/5] Plumb leak in test_buffer, occuring when a mutable buffer is requested for a read-only object, and enable test_buffer.py --- include/pybind11/detail/class.h | 12 ++++++------ tests/test_buffers.py | 2 ++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 7fd74d42a9..2f414e5c7c 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -550,6 +550,12 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla } std::memset(view, 0, sizeof(Py_buffer)); buffer_info *info = tinfo->get_buffer(obj, tinfo->get_buffer_data); + if ((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && info->readonly) { + delete info; + // view->obj = nullptr; // Was just memset to 0, so not necessary + PyErr_SetString(PyExc_BufferError, "Writable buffer requested for readonly storage"); + return -1; + } view->obj = obj; view->ndim = 1; view->internal = info; @@ -559,12 +565,6 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla for (auto s : info->shape) view->len *= s; view->readonly = info->readonly; - if ((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && info->readonly) { - if (view) - view->obj = nullptr; - PyErr_SetString(PyExc_BufferError, "Writable buffer requested for readonly storage"); - return -1; - } if ((flags & PyBUF_FORMAT) == PyBUF_FORMAT) view->format = const_cast(info->format.c_str()); if ((flags & PyBUF_STRIDES) == PyBUF_STRIDES) { diff --git a/tests/test_buffers.py b/tests/test_buffers.py index f0f370844b..50845758b8 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -92,6 +92,8 @@ def test_readonly_buffer(): view = memoryview(buf) assert view[0] == b"d" if env.PY2 else 0x64 assert view.readonly + with pytest.raises(TypeError): + view[0] = b"\0" if env.PY2 else 0 def test_selective_readonly_buffer(): From 26d048aecc802033d29b881b662fb3626a90cb5f Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sun, 27 Dec 2020 22:00:26 +0100 Subject: [PATCH 4/5] Fix weird return_value_policy::reference in test_stl_binders, and enable those tests --- tests/test_stl_binders.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index 1c0df984d8..c791477e3e 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -86,13 +86,13 @@ TEST_SUBMODULE(stl_binders, m) { // test_noncopyable_containers py::bind_vector>(m, "VectorENC"); - m.def("get_vnc", &one_to_n>, py::return_value_policy::reference); + m.def("get_vnc", &one_to_n>); py::bind_vector>(m, "DequeENC"); - m.def("get_dnc", &one_to_n>, py::return_value_policy::reference); + m.def("get_dnc", &one_to_n>); py::bind_map>(m, "MapENC"); - m.def("get_mnc", ×_ten>, py::return_value_policy::reference); + m.def("get_mnc", ×_ten>); py::bind_map>(m, "UmapENC"); - m.def("get_umnc", ×_ten>, py::return_value_policy::reference); + m.def("get_umnc", ×_ten>); // Issue #1885: binding nested std::map> with E non-copyable py::bind_map>>(m, "MapVecENC"); m.def("get_nvnc", [](int n) @@ -102,11 +102,11 @@ TEST_SUBMODULE(stl_binders, m) { for (int j = 1; j <= n; j++) (*m)[i].emplace_back(j); return m; - }, py::return_value_policy::reference); + }); py::bind_map>>(m, "MapMapENC"); - m.def("get_nmnc", ×_hundred>>, py::return_value_policy::reference); + m.def("get_nmnc", ×_hundred>>); py::bind_map>>(m, "UmapUmapENC"); - m.def("get_numnc", ×_hundred>>, py::return_value_policy::reference); + m.def("get_numnc", ×_hundred>>); // test_vector_buffer py::bind_vector>(m, "VectorUChar", py::buffer_protocol()); From 10635e12e1308242080890135b754f5d60613f1f Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sun, 27 Dec 2020 23:04:42 +0100 Subject: [PATCH 5/5] Cleanup nodelete holder objects in test_smart_ptr, and enable those tests --- tests/test_smart_ptr.cpp | 40 +++++++++++++++++++++++++++++++++++----- tests/test_smart_ptr.py | 16 ++++++++++++---- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 812b8e24c5..59996edeb4 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -176,33 +176,63 @@ TEST_SUBMODULE(smart_ptr, m) { // test_unique_nodelete // Object with a private destructor + class MyObject4; + static std::unordered_set myobject4_instances; class MyObject4 { public: - MyObject4(int value) : value{value} { print_created(this); } + MyObject4(int value) : value{value} { + print_created(this); + myobject4_instances.insert(this); + } int value; + + static void cleanupAllInstances() { + auto tmp = std::move(myobject4_instances); + myobject4_instances.clear(); + for (auto o : tmp) + delete o; + } private: - ~MyObject4() { print_destroyed(this); } + ~MyObject4() { + myobject4_instances.erase(this); + print_destroyed(this); + } }; py::class_>(m, "MyObject4") .def(py::init()) - .def_readwrite("value", &MyObject4::value); + .def_readwrite("value", &MyObject4::value) + .def_static("cleanup_all_instances", &MyObject4::cleanupAllInstances); // test_unique_deleter // Object with std::unique_ptr where D is not matching the base class // Object with a protected destructor + class MyObject4a; + static std::unordered_set myobject4a_instances; class MyObject4a { public: MyObject4a(int i) { value = i; print_created(this); + myobject4a_instances.insert(this); }; int value; + + static void cleanupAllInstances() { + auto tmp = std::move(myobject4a_instances); + myobject4a_instances.clear(); + for (auto o : tmp) + delete o; + } protected: - virtual ~MyObject4a() { print_destroyed(this); } + virtual ~MyObject4a() { + myobject4a_instances.erase(this); + print_destroyed(this); + } }; py::class_>(m, "MyObject4a") .def(py::init()) - .def_readwrite("value", &MyObject4a::value); + .def_readwrite("value", &MyObject4a::value) + .def_static("cleanup_all_instances", &MyObject4a::cleanupAllInstances); // Object derived but with public destructor and no Deleter in default holder class MyObject4b : public MyObject4a { diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index c55bffba07..85f61a3223 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -125,7 +125,9 @@ def test_unique_nodelete(): cstats = ConstructorStats.get(m.MyObject4) assert cstats.alive() == 1 del o - assert cstats.alive() == 1 # Leak, but that's intentional + assert cstats.alive() == 1 + m.MyObject4.cleanup_all_instances() + assert cstats.alive() == 0 def test_unique_nodelete4a(): @@ -134,19 +136,25 @@ def test_unique_nodelete4a(): cstats = ConstructorStats.get(m.MyObject4a) assert cstats.alive() == 1 del o - assert cstats.alive() == 1 # Leak, but that's intentional + assert cstats.alive() == 1 + m.MyObject4a.cleanup_all_instances() + assert cstats.alive() == 0 def test_unique_deleter(): + m.MyObject4a(0) o = m.MyObject4b(23) assert o.value == 23 cstats4a = ConstructorStats.get(m.MyObject4a) - assert cstats4a.alive() == 2 # Two because of previous test + assert cstats4a.alive() == 2 cstats4b = ConstructorStats.get(m.MyObject4b) assert cstats4b.alive() == 1 del o - assert cstats4a.alive() == 1 # Should now only be one leftover from previous test + assert cstats4a.alive() == 1 # Should now only be one leftover assert cstats4b.alive() == 0 # Should be deleted + m.MyObject4a.cleanup_all_instances() + assert cstats4a.alive() == 0 + assert cstats4b.alive() == 0 def test_large_holder():