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

YannickJadoul · 2020-12-30T20:00:34Z

Description

Five reasonably trivial fixes for leaks in the tests, commits carved out of #2746:

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

Suggested changelog entry:

Fixed various minor memory leaks in pybind11's test suite.

…support

…d for a read-only object, and enable test_buffer.py

…ble those tests

…ests

YannickJadoul · 2020-12-30T20:10:29Z

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;


Comment from #2746:

@YannickJadoul: This was the actual leak; The fragment was just moved up to not do futile work and such that view->obj = nullptr; isn't necessary anymore.

YannickJadoul · 2020-12-30T20:12:02Z

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);


Comments from #2746:

@bstaletic: These rvps look bogus. The return is by value.

@YannickJadoul: Huh? I thought it returned a pointer to the container?

@bstaletic: I was just entirely unable to figure out why/how the reference rvp is here. This test was added in Fix stl_bind to support movable, non-copyable value types #490 (commit 617fbcf). The PR involves returning elements by reference, but I don't see why these factory methods return the container by reference.

@YannickJadoul: Huh? I thought it returned a pointer to the container?

@bstaletic: That's what happens when you (read: I) do a review while really really tired. Listen to @YannickJadoul, everyone!

@YannickJadoul: Hah 😄 No worries, we've all been there.

@bstaletic: I'm not resolving this discussion, just so it doesn't get collapsed before others take a look.

bstaletic · 2020-12-31T10:26:09Z

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


PyCFunction_NewEx() just calls PyCMethod_new() with the last parameter set to NULL.

What this changes is m_self from NULL to def_capsule.ptr().

Beyond that, I have no idea how this works. PyCFunction* aren't documented, which is discussed in bpo-16776.

What this changes is m_self from NULL to def_capsule.ptr().

We already do this in the actual pybind11 code, btw.

Alternatively, if you prefer, we could play the "weakref trick", here? But I thought this would be easier, since AFAICT m_self being NULL wasn't the point of this test.

I'm just trying to understand how does setting m_self work to our benefit in this case. Does it steal the reference or borrow it? Is our reference counting right in this case? If it appeased valgrind, I guess it's correct, I'd just like a better understanding of how it works.

It does borrow, yes, and keeps it alive as some kind of CPython-style void *data, passed as first argument to our cpp_function::dispatcher:

pybind11/include/pybind11/pybind11.h

Line 492 in d587a2f

static PyObject *dispatcher(PyObject *self, PyObject *args_in, PyObject *kwargs_in) {

See here for the other case where CPython uses it:

pybind11/include/pybind11/pybind11.h

Line 372 in d587a2f

m_ptr = PyCFunction_NewEx(rec->def, rec_capsule.ptr(), scope_module.ptr());

This mechanism was also our problem with Python 3.9.0, m_self being decref'ed a few lines too early while pybind11 was still accessing it, in #2576.

bstaletic · 2020-12-31T10:28:21Z

tests/test_copy_move.cpp

+    // #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);


Just to make things clear, this is avoiding the issue reported in #2742, but not proposing a definite solution.

Moreover, the issue that was a precursor to adding this tests doesn't seem to depend on get_moveissue1() returning a raw owning pointer.

No, but if we decide to have a different approach to #2742, we'll add new tests for those, anyway? So this change could be semi-permanent?

bstaletic

This looks good to me.

henryiii

Small fix then mostly test changes to be tidy, looks good to me.

YannickJadoul · 2021-01-01T15:29:45Z

@henryiii, you can't think why these test_stl_binders tests were returning with return_value_policy::reference, in #490 / 378debb?
If not, I think we can merge; the other fixes should be uncontroversial enough :-)

henryiii · 2021-01-01T15:41:55Z

Looks like a (natural) mistake to me.

YannickJadoul · 2021-01-01T16:04:51Z

OK, then I'll merge and rebase! Thanks for checking!

YannickJadoul added 5 commits December 30, 2020 20:52

Fix leak in the test_copy_move::test_move_fallback

b14891c

Fix leaking PyMethodDef in test_class::test_implicit_conversion_life_…

16bece7

…support

Plumb leak in test_buffer, occuring when a mutable buffer is requeste…

aaeddc3

…d for a read-only object, and enable test_buffer.py

Fix weird return_value_policy::reference in test_stl_binders, and ena…

26d048a

…ble those tests

Cleanup nodelete holder objects in test_smart_ptr, and enable those t…

10635e1

…ests

YannickJadoul commented Dec 30, 2020

View reviewed changes

YannickJadoul mentioned this pull request Dec 30, 2020

Add a Valgrind build on debug Python 3.9 #2746

Merged

10 tasks

bstaletic reviewed Dec 31, 2020

View reviewed changes

henryiii approved these changes Jan 1, 2021

View reviewed changes

YannickJadoul merged commit e57dd47 into pybind:master Jan 1, 2021

YannickJadoul deleted the various-leaks branch January 1, 2021 16:05

github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 1, 2021

YannickJadoul added this to the v2.6.2 milestone Jan 1, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Jan 25, 2021

henryiii mentioned this pull request Jan 26, 2021

chore: prepare for the 2.6.2 release #2821

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YannickJadoul commented Dec 30, 2020 •

edited

Loading

YannickJadoul Dec 30, 2020

YannickJadoul Dec 30, 2020

bstaletic Dec 31, 2020

YannickJadoul Dec 31, 2020

bstaletic Dec 31, 2020

YannickJadoul Dec 31, 2020

bstaletic Dec 31, 2020

YannickJadoul Dec 31, 2020

bstaletic left a comment

henryiii left a comment

YannickJadoul commented Jan 1, 2021

henryiii commented Jan 1, 2021

YannickJadoul commented Jan 1, 2021

		// #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);

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

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

Conversation

YannickJadoul commented Dec 30, 2020 • edited Loading

Description

Suggested changelog entry:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

henryiii left a comment

Choose a reason for hiding this comment

YannickJadoul commented Jan 1, 2021

henryiii commented Jan 1, 2021

YannickJadoul commented Jan 1, 2021

YannickJadoul commented Dec 30, 2020 •

edited

Loading