Fix various minor memory leaks in the tests (found by Valgrind in #2746) (#2758)

* Fix leak in the test_copy_move::test_move_fallback

* Fix leaking PyMethodDef in test_class::test_implicit_conversion_life_support

* Plumb leak in test_buffer, occuring when a mutable buffer is requested for a read-only object, and enable test_buffer.py

* Fix weird return_value_policy::reference in test_stl_binders, and enable those tests

* Cleanup nodelete holder objects in test_smart_ptr, and enable those tests

Author: YannickJadoul

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<char *>(info->format.c_str());
     if ((flags & PyBUF_STRIDES) == PyBUF_STRIDES) {

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():

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<py::object>(PyCFunction_NewEx(def, nullptr, m.ptr()));
+        py::capsule def_capsule(def, [](void *ptr) { delete reinterpret_cast<PyMethodDef *>(ptr); });
+        return py::reinterpret_steal<py::object>(PyCFunction_NewEx(def, def_capsule.ptr(), m.ptr()));
     }());
 
     // test_operator_new_delete

tests/test_copy_move.cpp

@@ -214,6 +214,7 @@ TEST_SUBMODULE(copy_move_policies, m) {
     };
     py::class_<MoveIssue2>(m, "MoveIssue2").def(py::init<int>()).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<MoveIssue1>(new MoveIssue1(i)); }, py::return_value_policy::move);
     m.def("get_moveissue2", [](int i) { return MoveIssue2(i); }, py::return_value_policy::move);
 }

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

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 *> 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_<MyObject4, std::unique_ptr<MyObject4, py::nodelete>>(m, "MyObject4")
         .def(py::init<int>())
-        .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<T, D> where D is not matching the base class
     // Object with a protected destructor
+    class MyObject4a;
+    static std::unordered_set<MyObject4a *> 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_<MyObject4a, std::unique_ptr<MyObject4a, py::nodelete>>(m, "MyObject4a")
         .def(py::init<int>())
-        .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 {

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():

tests/test_stl_binders.cpp

@@ -86,13 +86,13 @@ TEST_SUBMODULE(stl_binders, m) {
 
     // test_noncopyable_containers
     py::bind_vector<std::vector<E_nc>>(m, "VectorENC");
-    m.def("get_vnc", &one_to_n<std::vector<E_nc>>, py::return_value_policy::reference);
+    m.def("get_vnc", &one_to_n<std::vector<E_nc>>);
     py::bind_vector<std::deque<E_nc>>(m, "DequeENC");
-    m.def("get_dnc", &one_to_n<std::deque<E_nc>>, py::return_value_policy::reference);
+    m.def("get_dnc", &one_to_n<std::deque<E_nc>>);
     py::bind_map<std::map<int, E_nc>>(m, "MapENC");
-    m.def("get_mnc", &times_ten<std::map<int, E_nc>>, py::return_value_policy::reference);
+    m.def("get_mnc", &times_ten<std::map<int, E_nc>>);
     py::bind_map<std::unordered_map<int, E_nc>>(m, "UmapENC");
-    m.def("get_umnc", &times_ten<std::unordered_map<int, E_nc>>, py::return_value_policy::reference);
+    m.def("get_umnc", &times_ten<std::unordered_map<int, E_nc>>);
     // Issue #1885: binding nested std::map<X, Container<E>> with E non-copyable
     py::bind_map<std::map<int, std::vector<E_nc>>>(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<std::map<int, std::map<int, E_nc>>>(m, "MapMapENC");
-    m.def("get_nmnc", &times_hundred<std::map<int, std::map<int, E_nc>>>, py::return_value_policy::reference);
+    m.def("get_nmnc", &times_hundred<std::map<int, std::map<int, E_nc>>>);
     py::bind_map<std::unordered_map<int, std::unordered_map<int, E_nc>>>(m, "UmapUmapENC");
-    m.def("get_numnc", &times_hundred<std::unordered_map<int, std::unordered_map<int, E_nc>>>, py::return_value_policy::reference);
+    m.def("get_numnc", &times_hundred<std::unordered_map<int, std::unordered_map<int, E_nc>>>);
 
     // test_vector_buffer
     py::bind_vector<std::vector<unsigned char>>(m, "VectorUChar", py::buffer_protocol());