Skip to content

Commit 7c6f2f8

Browse files
galvrwgkSkylion007pre-commit-ci[bot]
authored
fix: PyCapsule_GetDestructor is allowed to return a nullptr destructor (#4221)
* 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 * Add test for the fix. * 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 <[email protected]> * style: pre-commit fixes Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]> Co-authored-by: Aaron Gokaslan <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 parent 4a42156 commit 7c6f2f8

File tree

3 files changed

+23
-6
lines changed

3 files changed

+23
-6
lines changed

include/pybind11/pytypes.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1829,18 +1829,18 @@ class capsule : public object {
18291829
// guard if destructor called while err indicator is set
18301830
error_scope error_guard;
18311831
auto destructor = reinterpret_cast<void (*)(void *)>(PyCapsule_GetContext(o));
1832-
if (destructor == nullptr) {
1833-
if (PyErr_Occurred()) {
1834-
throw error_already_set();
1835-
}
1836-
pybind11_fail("Unable to get capsule context");
1832+
if (PyErr_Occurred()) {
1833+
throw error_already_set();
18371834
}
18381835
const char *name = get_name_in_error_scope(o);
18391836
void *ptr = PyCapsule_GetPointer(o, name);
18401837
if (ptr == nullptr) {
18411838
throw error_already_set();
18421839
}
1843-
destructor(ptr);
1840+
1841+
if (destructor != nullptr) {
1842+
destructor(ptr);
1843+
}
18441844
});
18451845

18461846
if (!m_ptr || PyCapsule_SetContext(m_ptr, (void *) destructor) != 0) {

tests/test_pytypes.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,12 @@ TEST_SUBMODULE(pytypes, m) {
289289
return capsule;
290290
});
291291

292+
m.def("return_capsule_with_explicit_nullptr_dtor", []() {
293+
py::print("creating capsule with explicit nullptr dtor");
294+
return py::capsule(reinterpret_cast<void *>(1234),
295+
static_cast<void (*)(void *)>(nullptr)); // PR #4221
296+
});
297+
292298
// test_accessors
293299
m.def("accessor_api", [](const py::object &o) {
294300
auto d = py::dict();

tests/test_pytypes.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,17 @@ def test_capsule(capture):
299299
"""
300300
)
301301

302+
with capture:
303+
a = m.return_capsule_with_explicit_nullptr_dtor()
304+
del a
305+
pytest.gc_collect()
306+
assert (
307+
capture.unordered
308+
== """
309+
creating capsule with explicit nullptr dtor
310+
"""
311+
)
312+
302313

303314
def test_accessors():
304315
class SubTestObject:

0 commit comments

Comments
 (0)