From 1da47592919539412796ee765645bc159457bc37 Mon Sep 17 00:00:00 2001 From: Daniel Galvez Date: Thu, 6 Oct 2022 21:58:56 -0700 Subject: [PATCH 1/4] fix: PyCapsule_GetDestructor is allowed to return a nullptr destructor Previously, this code would error out if the destructor happened to be a nullptr. This is incorrect. nullptrs are allowed for capsule destructors. "It is legal for a capsule to have a NULL destructor. This makes a NULL return code somewhat ambiguous; use PyCapsule_IsValid() or PyErr_Occurred() to disambiguate." See: https://docs.python.org/3/c-api/capsule.html#c.PyCapsule_GetDestructor I noticed this while working on a type caster related to #3858 DLPack happens to allow the destructor not to be defined on a capsule, and I encountered such a case. See: https://github.com/dmlc/dlpack/blob/e2bdd3bee8cb6501558042633fa59144cc8b7f5f/include/dlpack/dlpack.h#L219 --- include/pybind11/pytypes.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 565df4375b..29506b7eaf 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1829,18 +1829,18 @@ class capsule : public object { // guard if destructor called while err indicator is set error_scope error_guard; auto destructor = reinterpret_cast(PyCapsule_GetContext(o)); - if (destructor == nullptr) { - if (PyErr_Occurred()) { - throw error_already_set(); - } - pybind11_fail("Unable to get capsule context"); + if (PyErr_Occurred()) { + throw error_already_set(); } const char *name = get_name_in_error_scope(o); void *ptr = PyCapsule_GetPointer(o, name); if (ptr == nullptr) { throw error_already_set(); } - destructor(ptr); + + if (destructor != nullptr) { + destructor(ptr); + } }); if (!m_ptr || PyCapsule_SetContext(m_ptr, (void *) destructor) != 0) { From f626d7d22a8babfba4247c1f2f78d03d56baac5f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Oct 2022 07:05:51 -0700 Subject: [PATCH 2/4] Add test for the fix. --- tests/test_pytypes.cpp | 5 +++++ tests/test_pytypes.py | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index da0dc8f6ba..9e4adf45c8 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -289,6 +289,11 @@ TEST_SUBMODULE(pytypes, m) { return capsule; }); + m.def("return_capsule_with_explicit_nullptr_dtor", []() { + py::print("creating capsule with explicit nullptr dtor"); + return py::capsule((void *) 1234, (void (*)(void *)) nullptr); // PR #4221 + }); + // test_accessors m.def("accessor_api", [](const py::object &o) { auto d = py::dict(); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index a34eaa59e8..070849fc55 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -299,6 +299,17 @@ def test_capsule(capture): """ ) + with capture: + a = m.return_capsule_with_explicit_nullptr_dtor() + del a + pytest.gc_collect() + assert ( + capture.unordered + == """ + creating capsule with explicit nullptr dtor + """ + ) + def test_accessors(): class SubTestObject: From 015828a154361b2f6a365e127eff3551a9af72c7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 7 Oct 2022 09:11:47 -0700 Subject: [PATCH 3/4] Update tests/test_pytypes.cpp I tried this locally and it works! I never knew that there are cases where `reinterpret_cast` does not work but `static_cast` does. Let's see if all compilers are happy with this. Co-authored-by: Aaron Gokaslan --- tests/test_pytypes.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 9e4adf45c8..62805ffc4a 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -291,7 +291,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("return_capsule_with_explicit_nullptr_dtor", []() { py::print("creating capsule with explicit nullptr dtor"); - return py::capsule((void *) 1234, (void (*)(void *)) nullptr); // PR #4221 + return py::capsule(reinterpret_cast(1234), static_cast(nullptr)); // PR #4221 }); // test_accessors From 4b317507258e4060a1cb1c750af23c991902344c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 7 Oct 2022 16:12:25 +0000 Subject: [PATCH 4/4] style: pre-commit fixes --- tests/test_pytypes.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 62805ffc4a..c95ff8230a 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -291,7 +291,8 @@ TEST_SUBMODULE(pytypes, m) { m.def("return_capsule_with_explicit_nullptr_dtor", []() { py::print("creating capsule with explicit nullptr dtor"); - return py::capsule(reinterpret_cast(1234), static_cast(nullptr)); // PR #4221 + return py::capsule(reinterpret_cast(1234), + static_cast(nullptr)); // PR #4221 }); // test_accessors